2019-08-09 07:46:10

by Horia Geanta

[permalink] [raw]
Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> On Fri, 9 Aug 2019 at 05:48, Herbert Xu <[email protected]> wrote:
>>
>> On Thu, Aug 08, 2019 at 06:01:49PM +0000, Horia Geanta wrote:
>>>
>>> -- >8 --
>>>
>>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for covering
>>> CTS (part II)
>>
>> Patchwork doesn't like it when you do this and it'll discard
>> your patch. To make it into patchwork you need to put the new
>> Subject in the email headers.
>>
>
> IMO, pretending that your XTS implementation is compliant by only
I've never said that.
Some parts are compliant, some are not.

> providing test vectors with the last 8 bytes of IV cleared is not the
> right fix for this issue. If you want to be compliant, you will need
It's not a fix.
It's adding test vectors which are not provided in the P1619 standard,
where "data unit sequence number" is at most 5B.

> to provide a s/w fallback for these cases.
>
Yes, the plan is to:

-add 16B IV support for caam versions supporting it - caam Era 9+,
currently deployed in lx2160a and ls108a

-remove current 8B IV support and add s/w fallback for affected caam versions
I'd assume this could be done dynamically, i.e. depending on IV provided
in the crypto request to use either the caam engine or s/w fallback.

Horia


2019-08-09 17:50:20

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

On Fri, 9 Aug 2019 at 10:44, Horia Geanta <[email protected]> wrote:
>
> On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > On Fri, 9 Aug 2019 at 05:48, Herbert Xu <[email protected]> wrote:
> >>
> >> On Thu, Aug 08, 2019 at 06:01:49PM +0000, Horia Geanta wrote:
> >>>
> >>> -- >8 --
> >>>
> >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for covering
> >>> CTS (part II)
> >>
> >> Patchwork doesn't like it when you do this and it'll discard
> >> your patch. To make it into patchwork you need to put the new
> >> Subject in the email headers.
> >>
> >
> > IMO, pretending that your XTS implementation is compliant by only
> I've never said that.
> Some parts are compliant, some are not.
>
> > providing test vectors with the last 8 bytes of IV cleared is not the
> > right fix for this issue. If you want to be compliant, you will need
> It's not a fix.
> It's adding test vectors which are not provided in the P1619 standard,
> where "data unit sequence number" is at most 5B.
>

Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
numbers in the test vectors. However, we should obviously not add test
vectors that are known to cause breakages on hardware that works fine
in practice.

> > to provide a s/w fallback for these cases.
> >
> Yes, the plan is to:
>
> -add 16B IV support for caam versions supporting it - caam Era 9+,
> currently deployed in lx2160a and ls108a
>
> -remove current 8B IV support and add s/w fallback for affected caam versions
> I'd assume this could be done dynamically, i.e. depending on IV provided
> in the crypto request to use either the caam engine or s/w fallback.
>

Yes. If the IV received from the caller has bytes 8..15 cleared, you
use the limited XTS h/w implementation, otherwise you fall back to
xts(ecb-aes-caam..).

2019-08-09 20:57:41

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

> -----Original Message-----
> From: Ard Biesheuvel <[email protected]>
> Sent: Friday, August 9, 2019 7:49 PM
> To: Horia Geanta <[email protected]>
> Cc: Herbert Xu <[email protected]>; Pascal Van Leeuwen
> <[email protected]>; Milan Broz <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
>
> On Fri, 9 Aug 2019 at 10:44, Horia Geanta <[email protected]> wrote:
> >
> > On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > > On Fri, 9 Aug 2019 at 05:48, Herbert Xu <[email protected]> wrote:
> > >>
> > >> On Thu, Aug 08, 2019 at 06:01:49PM +0000, Horia Geanta wrote:
> > >>>
> > >>> -- >8 --
> > >>>
> > >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for covering
> > >>> CTS (part II)
> > >>
> > >> Patchwork doesn't like it when you do this and it'll discard
> > >> your patch. To make it into patchwork you need to put the new
> > >> Subject in the email headers.
> > >>
> > >
> > > IMO, pretending that your XTS implementation is compliant by only
> > I've never said that.
> > Some parts are compliant, some are not.
> >
> > > providing test vectors with the last 8 bytes of IV cleared is not the
> > > right fix for this issue. If you want to be compliant, you will need
> > It's not a fix.
> > It's adding test vectors which are not provided in the P1619 standard,
> > where "data unit sequence number" is at most 5B.
> >
>
> Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
> numbers in the test vectors. However, we should obviously not add test
> vectors that are known to cause breakages on hardware that works fine
> in practice.
>
Well, obviously, the full 16 byte sector number vectors fail on existing
CAAM hardware, which I do assume to work fine in practice. And you know
I'm not in favor of building all kinds of workarounds into the drivers.

Fact is, we know there are no current users that need more than 64 bits
of IV. Fact is also that having 64 bits of IV in the vectors is already
an improvement over the 40 bits in the original vectors. And unlike CTS,
I am not aware of any real use case for more than 64 bits.
Finally, another fact is that limiting the *vectors* to 64 bits of IV
does not prohibit anyone from *using* a full 128 bit IV on an
implementation that *does* support this. I would think most users of
XTS, like dmcrypt, would allow you to specify the cra_drivername
explictly anyway, so just don't select legacy CAAM if you need that.
(heck, if it would be reading and writing its own data, and not need
compatibility with other implementations, it wouldn't even matter)

So yes, the specs are quite clear on the sector number being a full
128 bits. But that doesn't prevent us from specifying that the
crypto API implementation currently only supports 64 bits, with the
remaining bits being forced to 0. We can always revisit that when
an actual use case for more than 64 bits arises ...

> > > to provide a s/w fallback for these cases.
> > >
> > Yes, the plan is to:
> >
> > -add 16B IV support for caam versions supporting it - caam Era 9+,
> > currently deployed in lx2160a and ls108a
> >
> > -remove current 8B IV support and add s/w fallback for affected caam versions
> > I'd assume this could be done dynamically, i.e. depending on IV provided
> > in the crypto request to use either the caam engine or s/w fallback.
> >
>
> Yes. If the IV received from the caller has bytes 8..15 cleared, you
> use the limited XTS h/w implementation, otherwise you fall back to
> xts(ecb-aes-caam..).

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-10 04:43:53

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

On Fri, 9 Aug 2019 at 23:57, Pascal Van Leeuwen
<[email protected]> wrote:
>
> > -----Original Message-----
> > From: Ard Biesheuvel <[email protected]>
> > Sent: Friday, August 9, 2019 7:49 PM
> > To: Horia Geanta <[email protected]>
> > Cc: Herbert Xu <[email protected]>; Pascal Van Leeuwen
> > <[email protected]>; Milan Broz <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
> >
> > On Fri, 9 Aug 2019 at 10:44, Horia Geanta <[email protected]> wrote:
> > >
> > > On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > > > On Fri, 9 Aug 2019 at 05:48, Herbert Xu <[email protected]> wrote:
> > > >>
> > > >> On Thu, Aug 08, 2019 at 06:01:49PM +0000, Horia Geanta wrote:
> > > >>>
> > > >>> -- >8 --
> > > >>>
> > > >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for covering
> > > >>> CTS (part II)
> > > >>
> > > >> Patchwork doesn't like it when you do this and it'll discard
> > > >> your patch. To make it into patchwork you need to put the new
> > > >> Subject in the email headers.
> > > >>
> > > >
> > > > IMO, pretending that your XTS implementation is compliant by only
> > > I've never said that.
> > > Some parts are compliant, some are not.
> > >
> > > > providing test vectors with the last 8 bytes of IV cleared is not the
> > > > right fix for this issue. If you want to be compliant, you will need
> > > It's not a fix.
> > > It's adding test vectors which are not provided in the P1619 standard,
> > > where "data unit sequence number" is at most 5B.
> > >
> >
> > Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
> > numbers in the test vectors. However, we should obviously not add test
> > vectors that are known to cause breakages on hardware that works fine
> > in practice.
> >
> Well, obviously, the full 16 byte sector number vectors fail on existing
> CAAM hardware, which I do assume to work fine in practice. And you know
> I'm not in favor of building all kinds of workarounds into the drivers.
>
> Fact is, we know there are no current users that need more than 64 bits
> of IV. Fact is also that having 64 bits of IV in the vectors is already
> an improvement over the 40 bits in the original vectors. And unlike CTS,
> I am not aware of any real use case for more than 64 bits.
> Finally, another fact is that limiting the *vectors* to 64 bits of IV
> does not prohibit anyone from *using* a full 128 bit IV on an
> implementation that *does* support this. I would think most users of
> XTS, like dmcrypt, would allow you to specify the cra_drivername
> explictly anyway, so just don't select legacy CAAM if you need that.
> (heck, if it would be reading and writing its own data, and not need
> compatibility with other implementations, it wouldn't even matter)
>
> So yes, the specs are quite clear on the sector number being a full
> 128 bits. But that doesn't prevent us from specifying that the
> crypto API implementation currently only supports 64 bits, with the
> remaining bits being forced to 0. We can always revisit that when
> an actual use case for more than 64 bits arises ...
>

You have got it completely backwards:

CTS has never worked in any kernel implementation, so regardless of
what the spec says, supporting it in the kernel is not a high priority
issue whichever way you put it. Now is the first time anyone has asked
for it in 12 years, and only because someone spotted a deviation
between a h/w and a s/w implementation, not because anyone tried to
use it and failed. (Note that passing anything other than a multiple
of the block size will cause an error rather than fail silently)

Truncated IVs are a huge issue, since we already expose the correct
API via AF_ALG (without any restrictions on how many of the IV bits
are populated), and apparently, if your AF_ALG request for xts(aes)
happens to be fulfilled by the CAAM driver and your implementation
uses more than 64 bits for the IV, the top bits get truncated silently
and your data might get eaten.

In my experience, users tend to care more about the latter than the former.


> > > > to provide a s/w fallback for these cases.
> > > >
> > > Yes, the plan is to:
> > >
> > > -add 16B IV support for caam versions supporting it - caam Era 9+,
> > > currently deployed in lx2160a and ls108a
> > >
> > > -remove current 8B IV support and add s/w fallback for affected caam versions
> > > I'd assume this could be done dynamically, i.e. depending on IV provided
> > > in the crypto request to use either the caam engine or s/w fallback.
> > >
> >
> > Yes. If the IV received from the caller has bytes 8..15 cleared, you
> > use the limited XTS h/w implementation, otherwise you fall back to
> > xts(ecb-aes-caam..).
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> http://www.insidesecure.com
>

2019-08-11 11:15:55

by Milan Broz

[permalink] [raw]
Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

On 10/08/2019 06:39, Ard Biesheuvel wrote:
> Truncated IVs are a huge issue, since we already expose the correct
> API via AF_ALG (without any restrictions on how many of the IV bits
> are populated), and apparently, if your AF_ALG request for xts(aes)
> happens to be fulfilled by the CAAM driver and your implementation
> uses more than 64 bits for the IV, the top bits get truncated silently
> and your data might get eaten.

Actually, I think we have already serious problem with in in kernel (no AF_ALG needed).

I do not have the hardware, but please could you check that dm-crypt big-endian IV
(plain64be) produces the same output on CAAM?

It is 64bit IV, but big-endian and we use size of cipher block (16bytes) here,
so the first 8 bytes are zero in this case.

I would expect data corruption in comparison to generic implementation,
if it supports only the first 64bit...

Try this:

# create small null device of 8 sectors, we use zeroes as fixed ciphertext
dmsetup create zero --table "0 8 zero"

# create crypt device on top of it (with some key), using plain64be IV
dmsetup create crypt --table "0 8 crypt aes-xts-plain64be e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"

# and compare it with and without your driver, this is what I get here:
# sha256sum /dev/mapper/crypt
532f71198d0d84d823b8e410738c6f43bc3e149d844dd6d37fa5b36d150501e1 /dev/mapper/crypt
# dmsetup remove crypt

You can try little-endian version (plain64), this should always work even with CAAM
dmsetup create crypt --table "0 8 crypt aes-xts-plain64 e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"

# sha256sum /dev/mapper/crypt
f17abd27dedee4e539758eabdb6c15fa619464b509cf55f16433e6a25da42857 /dev/mapper/crypt
# dmsetup remove crypt

# dmsetup remove zero


If you get different plaintext in the first case, your driver is actually creating
data corruption in this configuration and it should be fixed!
(Only the first sector must be the same, because it has IV == 0.)

Milan

p.s.
If you ask why we have this IV, it was added per request to allow map some chipset-based
encrypted drives directly. I guess it is used for some data forensic things.

2019-08-11 20:35:20

by Eric Biggers

[permalink] [raw]
Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

On Sun, Aug 11, 2019 at 01:12:56PM +0200, Milan Broz wrote:
> On 10/08/2019 06:39, Ard Biesheuvel wrote:
> > Truncated IVs are a huge issue, since we already expose the correct
> > API via AF_ALG (without any restrictions on how many of the IV bits
> > are populated), and apparently, if your AF_ALG request for xts(aes)
> > happens to be fulfilled by the CAAM driver and your implementation
> > uses more than 64 bits for the IV, the top bits get truncated silently
> > and your data might get eaten.
>
> Actually, I think we have already serious problem with in in kernel (no AF_ALG needed).
>
> I do not have the hardware, but please could you check that dm-crypt big-endian IV
> (plain64be) produces the same output on CAAM?
>
> It is 64bit IV, but big-endian and we use size of cipher block (16bytes) here,
> so the first 8 bytes are zero in this case.
>
> I would expect data corruption in comparison to generic implementation,
> if it supports only the first 64bit...
>
> Try this:
>
> # create small null device of 8 sectors, we use zeroes as fixed ciphertext
> dmsetup create zero --table "0 8 zero"
>
> # create crypt device on top of it (with some key), using plain64be IV
> dmsetup create crypt --table "0 8 crypt aes-xts-plain64be e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"
>
> # and compare it with and without your driver, this is what I get here:
> # sha256sum /dev/mapper/crypt
> 532f71198d0d84d823b8e410738c6f43bc3e149d844dd6d37fa5b36d150501e1 /dev/mapper/crypt
> # dmsetup remove crypt
>
> You can try little-endian version (plain64), this should always work even with CAAM
> dmsetup create crypt --table "0 8 crypt aes-xts-plain64 e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"
>
> # sha256sum /dev/mapper/crypt
> f17abd27dedee4e539758eabdb6c15fa619464b509cf55f16433e6a25da42857 /dev/mapper/crypt
> # dmsetup remove crypt
>
> # dmsetup remove zero
>
>
> If you get different plaintext in the first case, your driver is actually creating
> data corruption in this configuration and it should be fixed!
> (Only the first sector must be the same, because it has IV == 0.)
>
> Milan
>
> p.s.
> If you ask why we have this IV, it was added per request to allow map some chipset-based
> encrypted drives directly. I guess it is used for some data forensic things.
>

Also, if the CAAM driver is really truncating the IV for "xts(aes)", it should
already be failing the extra crypto self-tests, since the fuzz testing in
test_skcipher_vs_generic_impl() uses random IVs.

- Eric

2019-08-11 21:17:47

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

> -----Original Message-----
> From: Ard Biesheuvel <[email protected]>
> Sent: Saturday, August 10, 2019 6:40 AM
> To: Pascal Van Leeuwen <[email protected]>
> Cc: Horia Geanta <[email protected]>; Herbert Xu <[email protected]>; Milan Broz
> <[email protected]>; [email protected]; [email protected]
> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
>
> On Fri, 9 Aug 2019 at 23:57, Pascal Van Leeuwen
> <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <[email protected]>
> > > Sent: Friday, August 9, 2019 7:49 PM
> > > To: Horia Geanta <[email protected]>
> > > Cc: Herbert Xu <[email protected]>; Pascal Van Leeuwen
> > > <[email protected]>; Milan Broz <[email protected]>; [email protected];
> linux-
> > > [email protected]
> > > Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
> > >
> > > On Fri, 9 Aug 2019 at 10:44, Horia Geanta <[email protected]> wrote:
> > > >
> > > > On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > > > > On Fri, 9 Aug 2019 at 05:48, Herbert Xu <[email protected]> wrote:
> > > > >>
> > > > >> On Thu, Aug 08, 2019 at 06:01:49PM +0000, Horia Geanta wrote:
> > > > >>>
> > > > >>> -- >8 --
> > > > >>>
> > > > >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for covering
> > > > >>> CTS (part II)
> > > > >>
> > > > >> Patchwork doesn't like it when you do this and it'll discard
> > > > >> your patch. To make it into patchwork you need to put the new
> > > > >> Subject in the email headers.
> > > > >>
> > > > >
> > > > > IMO, pretending that your XTS implementation is compliant by only
> > > > I've never said that.
> > > > Some parts are compliant, some are not.
> > > >
> > > > > providing test vectors with the last 8 bytes of IV cleared is not the
> > > > > right fix for this issue. If you want to be compliant, you will need
> > > > It's not a fix.
> > > > It's adding test vectors which are not provided in the P1619 standard,
> > > > where "data unit sequence number" is at most 5B.
> > > >
> > >
> > > Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
> > > numbers in the test vectors. However, we should obviously not add test
> > > vectors that are known to cause breakages on hardware that works fine
> > > in practice.
> > >
> > Well, obviously, the full 16 byte sector number vectors fail on existing
> > CAAM hardware, which I do assume to work fine in practice. And you know
> > I'm not in favor of building all kinds of workarounds into the drivers.
> >
> > Fact is, we know there are no current users that need more than 64 bits
> > of IV. Fact is also that having 64 bits of IV in the vectors is already
> > an improvement over the 40 bits in the original vectors. And unlike CTS,
> > I am not aware of any real use case for more than 64 bits.
> > Finally, another fact is that limiting the *vectors* to 64 bits of IV
> > does not prohibit anyone from *using* a full 128 bit IV on an
> > implementation that *does* support this. I would think most users of
> > XTS, like dmcrypt, would allow you to specify the cra_drivername
> > explictly anyway, so just don't select legacy CAAM if you need that.
> > (heck, if it would be reading and writing its own data, and not need
> > compatibility with other implementations, it wouldn't even matter)
> >
> > So yes, the specs are quite clear on the sector number being a full
> > 128 bits. But that doesn't prevent us from specifying that the
> > crypto API implementation currently only supports 64 bits, with the
> > remaining bits being forced to 0. We can always revisit that when
> > an actual use case for more than 64 bits arises ...
> >
>
> You have got it completely backwards:
>
> CTS has never worked in any kernel implementation, so regardless of
> what the spec says, supporting it in the kernel is not a high priority
> issue whichever way you put it.
>
I never said it was a high priority, I merely pointed out it's not spec
compliant. Apparently, you feel that that's only important insofar that
it matches current kernel use cases?

Anyway, as far as I understand, there are no users that need more than
64 bits of IV in the kernel (i.e. dmcrypt uses only 64 bits), so I see
no fundamental difference with CTS except that most(?) implementations
possibly already "accidentally" (since unverified!) did it correctly.

Not that I have any interest in restricting the IV size: "my" hardware
handles full 128 bit IV's just fine. So why do I even bother ... :-)

> Now is the first time anyone has asked
> for it in 12 years, and only because someone spotted a deviation
> between a h/w and a s/w implementation, not because anyone tried to
> use it and failed. (Note that passing anything other than a multiple
> of the block size will cause an error rather than fail silently)
>
Yes, failing silently is not such a good idea, I think we agree on that.
Although we also need to keep in mind that that's exactly what the CAAM
driver has been doing all those years, and, before my vectors, nobody
noticed or cared. Without my involvement, this would have probably gone
unnoticed for many years to come (so I feel some responsibility ;-).

> Truncated IVs are a huge issue, since we already expose the correct
> API via AF_ALG (without any restrictions on how many of the IV bits
> are populated), and apparently, if your AF_ALG request for xts(aes)
> happens to be fulfilled by the CAAM driver and your implementation
> uses more than 64 bits for the IV, the top bits get truncated silently
> and your data might get eaten.
>
Apparently, not such a "huge" issue at all, see previous remark.

As a precaution, the CAAM driver could return -EINVAL if the upper IV
bytes are non-zero. But then testmgr would have to do less strict error
return code checking so we don't force this upon drivers that CAN do it.

Implementing a full SW fallback for that in the driver just seems like
massive overkill, as you normally specify the driver for dmcrypt explictly
anyway (or at least, you can do that if the default fails).

I don't like the idea of HW drivers doing SW fallbacks because it clouds
the whole picture of what is actually done by a certain HW device.

> In my experience, users tend to care more about the latter than the former.
>
>
> > > > > to provide a s/w fallback for these cases.
> > > > >
> > > > Yes, the plan is to:
> > > >
> > > > -add 16B IV support for caam versions supporting it - caam Era 9+,
> > > > currently deployed in lx2160a and ls108a
> > > >
> > > > -remove current 8B IV support and add s/w fallback for affected caam versions
> > > > I'd assume this could be done dynamically, i.e. depending on IV provided
> > > > in the crypto request to use either the caam engine or s/w fallback.
> > > >
> > >
> > > Yes. If the IV received from the caller has bytes 8..15 cleared, you
> > > use the limited XTS h/w implementation, otherwise you fall back to
> > > xts(ecb-aes-caam..).
> >
> > Regards,
> > Pascal van Leeuwen
> > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> > http://www.insidesecure.com
> >

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-11 21:30:04

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

> -----Original Message-----
> From: Milan Broz <[email protected]>
> Sent: Sunday, August 11, 2019 1:13 PM
> To: Ard Biesheuvel <[email protected]>; Pascal Van Leeuwen
> <[email protected]>
> Cc: Horia Geanta <[email protected]>; Herbert Xu <[email protected]>; dm-
> [email protected]; [email protected]
> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
>
> On 10/08/2019 06:39, Ard Biesheuvel wrote:
> > Truncated IVs are a huge issue, since we already expose the correct
> > API via AF_ALG (without any restrictions on how many of the IV bits
> > are populated), and apparently, if your AF_ALG request for xts(aes)
> > happens to be fulfilled by the CAAM driver and your implementation
> > uses more than 64 bits for the IV, the top bits get truncated silently
> > and your data might get eaten.
>
> Actually, I think we have already serious problem with in in kernel (no AF_ALG needed).
>
> I do not have the hardware, but please could you check that dm-crypt big-endian IV
> (plain64be) produces the same output on CAAM?
>
> It is 64bit IV, but big-endian and we use size of cipher block (16bytes) here,
> so the first 8 bytes are zero in this case.
>
> I would expect data corruption in comparison to generic implementation,
> if it supports only the first 64bit...
>
> Try this:
>
> # create small null device of 8 sectors, we use zeroes as fixed ciphertext
> dmsetup create zero --table "0 8 zero"
>
> # create crypt device on top of it (with some key), using plain64be IV
> dmsetup create crypt --table "0 8 crypt aes-xts-plain64be
> e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"
>
> # and compare it with and without your driver, this is what I get here:
> # sha256sum /dev/mapper/crypt
> 532f71198d0d84d823b8e410738c6f43bc3e149d844dd6d37fa5b36d150501e1 /dev/mapper/crypt
> # dmsetup remove crypt
>
> You can try little-endian version (plain64), this should always work even with CAAM
> dmsetup create crypt --table "0 8 crypt aes-xts-plain64
> e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"
>
> # sha256sum /dev/mapper/crypt
> f17abd27dedee4e539758eabdb6c15fa619464b509cf55f16433e6a25da42857 /dev/mapper/crypt
> # dmsetup remove crypt
>
> # dmsetup remove zero
>
>
> If you get different plaintext in the first case, your driver is actually creating
> data corruption in this configuration and it should be fixed!
> (Only the first sector must be the same, because it has IV == 0.)
>
It will very likely fail with that CAAM h/w, but that only proves that you
should not use plain64be IV's together with CAAM h/w. Which should be
entirely avoidable in real life unless you have some unlikely requirement
to move physical encrypted diks from one system (without CAAM h/w and needing
these plain64be IV's for some reason) to another system (with CAAM h/w) and be
able to decrypt them there.

Formally, these plain64be IV's are actually WRONG, since the XTS specification is
very clear on the byte order of the sector number ("little endian byte array").


> Milan
>
> p.s.
> If you ask why we have this IV, it was added per request to allow map some chipset-based
> encrypted drives directly. I guess it is used for some data forensic things.
>
Sounds like someone got the endianness wrong ;-)

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-11 21:40:12

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

> -----Original Message-----
> From: Eric Biggers <[email protected]>
> Sent: Sunday, August 11, 2019 10:34 PM
> To: Milan Broz <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>; Pascal Van Leeuwen
> <[email protected]>; [email protected]; Herbert Xu <[email protected]>;
> Horia Geanta <[email protected]>; [email protected]
> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
>
> On Sun, Aug 11, 2019 at 01:12:56PM +0200, Milan Broz wrote:
> > On 10/08/2019 06:39, Ard Biesheuvel wrote:
> > > Truncated IVs are a huge issue, since we already expose the correct
> > > API via AF_ALG (without any restrictions on how many of the IV bits
> > > are populated), and apparently, if your AF_ALG request for xts(aes)
> > > happens to be fulfilled by the CAAM driver and your implementation
> > > uses more than 64 bits for the IV, the top bits get truncated silently
> > > and your data might get eaten.
> >
> > Actually, I think we have already serious problem with in in kernel (no AF_ALG needed).
> >
> > I do not have the hardware, but please could you check that dm-crypt big-endian IV
> > (plain64be) produces the same output on CAAM?
> >
> > It is 64bit IV, but big-endian and we use size of cipher block (16bytes) here,
> > so the first 8 bytes are zero in this case.
> >
> > I would expect data corruption in comparison to generic implementation,
> > if it supports only the first 64bit...
> >
> > Try this:
> >
> > # create small null device of 8 sectors, we use zeroes as fixed ciphertext
> > dmsetup create zero --table "0 8 zero"
> >
> > # create crypt device on top of it (with some key), using plain64be IV
> > dmsetup create crypt --table "0 8 crypt aes-xts-plain64be
> e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"
> >
> > # and compare it with and without your driver, this is what I get here:
> > # sha256sum /dev/mapper/crypt
> > 532f71198d0d84d823b8e410738c6f43bc3e149d844dd6d37fa5b36d150501e1 /dev/mapper/crypt
> > # dmsetup remove crypt
> >
> > You can try little-endian version (plain64), this should always work even with CAAM
> > dmsetup create crypt --table "0 8 crypt aes-xts-plain64
> e8cfa3dbfe373b536be43c5637387786c01be00ba5f730aacb039e86f3eb72f3 0 /dev/mapper/zero 0"
> >
> > # sha256sum /dev/mapper/crypt
> > f17abd27dedee4e539758eabdb6c15fa619464b509cf55f16433e6a25da42857 /dev/mapper/crypt
> > # dmsetup remove crypt
> >
> > # dmsetup remove zero
> >
> >
> > If you get different plaintext in the first case, your driver is actually creating
> > data corruption in this configuration and it should be fixed!
> > (Only the first sector must be the same, because it has IV == 0.)
> >
> > Milan
> >
> > p.s.
> > If you ask why we have this IV, it was added per request to allow map some chipset-based
> > encrypted drives directly. I guess it is used for some data forensic things.
> >
>
> Also, if the CAAM driver is really truncating the IV for "xts(aes)", it should
> already be failing the extra crypto self-tests, since the fuzz testing in
> test_skcipher_vs_generic_impl() uses random IVs.
>
> - Eric
>
Yes, good point. Although that is only seen during development and not
during "normal" use ...


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-11 22:27:40

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

On Mon, 12 Aug 2019 at 00:15, Pascal Van Leeuwen
<[email protected]> wrote:
>
> > -----Original Message-----
> > From: Ard Biesheuvel <[email protected]>
> > Sent: Saturday, August 10, 2019 6:40 AM
> > To: Pascal Van Leeuwen <[email protected]>
> > Cc: Horia Geanta <[email protected]>; Herbert Xu <[email protected]>; Milan Broz
> > <[email protected]>; [email protected]; [email protected]
> > Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
> >
> > On Fri, 9 Aug 2019 at 23:57, Pascal Van Leeuwen
> > <[email protected]> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel <[email protected]>
> > > > Sent: Friday, August 9, 2019 7:49 PM
> > > > To: Horia Geanta <[email protected]>
> > > > Cc: Herbert Xu <[email protected]>; Pascal Van Leeuwen
> > > > <[email protected]>; Milan Broz <[email protected]>; [email protected];
> > linux-
> > > > [email protected]
> > > > Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
> > > >
> > > > On Fri, 9 Aug 2019 at 10:44, Horia Geanta <[email protected]> wrote:
> > > > >
> > > > > On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > > > > > On Fri, 9 Aug 2019 at 05:48, Herbert Xu <[email protected]> wrote:
> > > > > >>
> > > > > >> On Thu, Aug 08, 2019 at 06:01:49PM +0000, Horia Geanta wrote:
> > > > > >>>
> > > > > >>> -- >8 --
> > > > > >>>
> > > > > >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for covering
> > > > > >>> CTS (part II)
> > > > > >>
> > > > > >> Patchwork doesn't like it when you do this and it'll discard
> > > > > >> your patch. To make it into patchwork you need to put the new
> > > > > >> Subject in the email headers.
> > > > > >>
> > > > > >
> > > > > > IMO, pretending that your XTS implementation is compliant by only
> > > > > I've never said that.
> > > > > Some parts are compliant, some are not.
> > > > >
> > > > > > providing test vectors with the last 8 bytes of IV cleared is not the
> > > > > > right fix for this issue. If you want to be compliant, you will need
> > > > > It's not a fix.
> > > > > It's adding test vectors which are not provided in the P1619 standard,
> > > > > where "data unit sequence number" is at most 5B.
> > > > >
> > > >
> > > > Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
> > > > numbers in the test vectors. However, we should obviously not add test
> > > > vectors that are known to cause breakages on hardware that works fine
> > > > in practice.
> > > >
> > > Well, obviously, the full 16 byte sector number vectors fail on existing
> > > CAAM hardware, which I do assume to work fine in practice. And you know
> > > I'm not in favor of building all kinds of workarounds into the drivers.
> > >
> > > Fact is, we know there are no current users that need more than 64 bits
> > > of IV. Fact is also that having 64 bits of IV in the vectors is already
> > > an improvement over the 40 bits in the original vectors. And unlike CTS,
> > > I am not aware of any real use case for more than 64 bits.
> > > Finally, another fact is that limiting the *vectors* to 64 bits of IV
> > > does not prohibit anyone from *using* a full 128 bit IV on an
> > > implementation that *does* support this. I would think most users of
> > > XTS, like dmcrypt, would allow you to specify the cra_drivername
> > > explictly anyway, so just don't select legacy CAAM if you need that.
> > > (heck, if it would be reading and writing its own data, and not need
> > > compatibility with other implementations, it wouldn't even matter)
> > >
> > > So yes, the specs are quite clear on the sector number being a full
> > > 128 bits. But that doesn't prevent us from specifying that the
> > > crypto API implementation currently only supports 64 bits, with the
> > > remaining bits being forced to 0. We can always revisit that when
> > > an actual use case for more than 64 bits arises ...
> > >
> >
> > You have got it completely backwards:
> >
> > CTS has never worked in any kernel implementation, so regardless of
> > what the spec says, supporting it in the kernel is not a high priority
> > issue whichever way you put it.
> >
> I never said it was a high priority, I merely pointed out it's not spec
> compliant. Apparently, you feel that that's only important insofar that
> it matches current kernel use cases?
>
> Anyway, as far as I understand, there are no users that need more than
> 64 bits of IV in the kernel (i.e. dmcrypt uses only 64 bits), so I see
> no fundamental difference with CTS except that most(?) implementations
> possibly already "accidentally" (since unverified!) did it correctly.
>

Well, as Milan points out, dm-crypt may use the upper bits as well,
but the point I was making was about the userland interface. If you
care about spec compliance for the sake of it, I don't understand why
you dismiss a broken userland->kernel interface that silently fails
and corrupts your data as something we should just live with, and just
assume nobody will ever be silly enough to use it the way the spec
describes it, and use all the available IV bits.

> Not that I have any interest in restricting the IV size: "my" hardware
> handles full 128 bit IV's just fine. So why do I even bother ... :-)
>
> > Now is the first time anyone has asked
> > for it in 12 years, and only because someone spotted a deviation
> > between a h/w and a s/w implementation, not because anyone tried to
> > use it and failed. (Note that passing anything other than a multiple
> > of the block size will cause an error rather than fail silently)
> >
> Yes, failing silently is not such a good idea, I think we agree on that.
> Although we also need to keep in mind that that's exactly what the CAAM
> driver has been doing all those years, and, before my vectors, nobody
> noticed or cared.

True. But now that the cat is out of the bag, we have an obligation to
our users to ensure that a known problem does not corrupt their data.

> Without my involvement, this would have probably gone
> unnoticed for many years to come (so I feel some responsibility ;-).
>
> > Truncated IVs are a huge issue, since we already expose the correct
> > API via AF_ALG (without any restrictions on how many of the IV bits
> > are populated), and apparently, if your AF_ALG request for xts(aes)
> > happens to be fulfilled by the CAAM driver and your implementation
> > uses more than 64 bits for the IV, the top bits get truncated silently
> > and your data might get eaten.
> >
> Apparently, not such a "huge" issue at all, see previous remark.
>
> As a precaution, the CAAM driver could return -EINVAL if the upper IV
> bytes are non-zero.

Then we'd still have two implementations that behave differently, and
this is not how abstractions work. If a driver implements xts(aes),
then it should produce the same output (and return value) for every
input.

> But then testmgr would have to do less strict error
> return code checking so we don't force this upon drivers that CAN do it.
>
> Implementing a full SW fallback for that in the driver just seems like
> massive overkill, as you normally specify the driver for dmcrypt explictly
> anyway (or at least, you can do that if the default fails).
>

How would you find out if the default fails? Either it corrupts your
data, or it fails with an error that is specific to CAAM, and so we
would have to update the dm-crypt code to deal with en/decryption
failures that can only occur in this particular case. This is
*exactly* the thing I argued against before, when we discussed zero
length inputs: the reason we want the workaround in the driver is
because it is contained, and its peculiarities don't leak into other
layers.

> I don't like the idea of HW drivers doing SW fallbacks because it clouds
> the whole picture of what is actually done by a certain HW device.
>

Well, the reality is that we OS guys get to clean up after the h/w
guys all the time. I agree that explicit failure is better than silent
corruption, but that still means there are corner cases where things
just don't work. So the only acceptable way to me is to fix this with
a s/w workaround, and as I already pointed out, this could simply be
the xts template wrapped around the accelerated ECB implementation
exposed by the driver.


> > In my experience, users tend to care more about the latter than the former.
> >
> >
> > > > > > to provide a s/w fallback for these cases.
> > > > > >
> > > > > Yes, the plan is to:
> > > > >
> > > > > -add 16B IV support for caam versions supporting it - caam Era 9+,
> > > > > currently deployed in lx2160a and ls108a
> > > > >
> > > > > -remove current 8B IV support and add s/w fallback for affected caam versions
> > > > > I'd assume this could be done dynamically, i.e. depending on IV provided
> > > > > in the crypto request to use either the caam engine or s/w fallback.
> > > > >
> > > >
> > > > Yes. If the IV received from the caller has bytes 8..15 cleared, you
> > > > use the limited XTS h/w implementation, otherwise you fall back to
> > > > xts(ecb-aes-caam..).
> > >
> > > Regards,
> > > Pascal van Leeuwen
> > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> > > http://www.insidesecure.com
> > >
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> http://www.insidesecure.com
>

2019-08-12 01:09:27

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

> -----Original Message-----
> From: Ard Biesheuvel <[email protected]>
> Sent: Monday, August 12, 2019 12:24 AM
> To: Pascal Van Leeuwen <[email protected]>
> Cc: Horia Geanta <[email protected]>; Herbert Xu <[email protected]>; Milan Broz
> <[email protected]>; [email protected]; [email protected]
> Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
>
> On Mon, 12 Aug 2019 at 00:15, Pascal Van Leeuwen
> <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: Ard Biesheuvel <[email protected]>
> > > Sent: Saturday, August 10, 2019 6:40 AM
> > > To: Pascal Van Leeuwen <[email protected]>
> > > Cc: Horia Geanta <[email protected]>; Herbert Xu <[email protected]>; Milan
> Broz
> > > <[email protected]>; [email protected]; [email protected]
> > > Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
> > >
> > > On Fri, 9 Aug 2019 at 23:57, Pascal Van Leeuwen
> > > <[email protected]> wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Ard Biesheuvel <[email protected]>
> > > > > Sent: Friday, August 9, 2019 7:49 PM
> > > > > To: Horia Geanta <[email protected]>
> > > > > Cc: Herbert Xu <[email protected]>; Pascal Van Leeuwen
> > > > > <[email protected]>; Milan Broz <[email protected]>; [email protected];
> > > linux-
> > > > > [email protected]
> > > > > Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support
> > > > >
> > > > > On Fri, 9 Aug 2019 at 10:44, Horia Geanta <[email protected]> wrote:
> > > > > >
> > > > > > On 8/9/2019 9:45 AM, Ard Biesheuvel wrote:
> > > > > > > On Fri, 9 Aug 2019 at 05:48, Herbert Xu <[email protected]> wrote:
> > > > > > >>
> > > > > > >> On Thu, Aug 08, 2019 at 06:01:49PM +0000, Horia Geanta wrote:
> > > > > > >>>
> > > > > > >>> -- >8 --
> > > > > > >>>
> > > > > > >>> Subject: [PATCH] crypto: testmgr - Add additional AES-XTS vectors for covering
> > > > > > >>> CTS (part II)
> > > > > > >>
> > > > > > >> Patchwork doesn't like it when you do this and it'll discard
> > > > > > >> your patch. To make it into patchwork you need to put the new
> > > > > > >> Subject in the email headers.
> > > > > > >>
> > > > > > >
> > > > > > > IMO, pretending that your XTS implementation is compliant by only
> > > > > > I've never said that.
> > > > > > Some parts are compliant, some are not.
> > > > > >
> > > > > > > providing test vectors with the last 8 bytes of IV cleared is not the
> > > > > > > right fix for this issue. If you want to be compliant, you will need
> > > > > > It's not a fix.
> > > > > > It's adding test vectors which are not provided in the P1619 standard,
> > > > > > where "data unit sequence number" is at most 5B.
> > > > > >
> > > > >
> > > > > Indeed. But I would prefer not to limit ourselves to 5 bytes of sector
> > > > > numbers in the test vectors. However, we should obviously not add test
> > > > > vectors that are known to cause breakages on hardware that works fine
> > > > > in practice.
> > > > >
> > > > Well, obviously, the full 16 byte sector number vectors fail on existing
> > > > CAAM hardware, which I do assume to work fine in practice. And you know
> > > > I'm not in favor of building all kinds of workarounds into the drivers.
> > > >
> > > > Fact is, we know there are no current users that need more than 64 bits
> > > > of IV. Fact is also that having 64 bits of IV in the vectors is already
> > > > an improvement over the 40 bits in the original vectors. And unlike CTS,
> > > > I am not aware of any real use case for more than 64 bits.
> > > > Finally, another fact is that limiting the *vectors* to 64 bits of IV
> > > > does not prohibit anyone from *using* a full 128 bit IV on an
> > > > implementation that *does* support this. I would think most users of
> > > > XTS, like dmcrypt, would allow you to specify the cra_drivername
> > > > explictly anyway, so just don't select legacy CAAM if you need that.
> > > > (heck, if it would be reading and writing its own data, and not need
> > > > compatibility with other implementations, it wouldn't even matter)
> > > >
> > > > So yes, the specs are quite clear on the sector number being a full
> > > > 128 bits. But that doesn't prevent us from specifying that the
> > > > crypto API implementation currently only supports 64 bits, with the
> > > > remaining bits being forced to 0. We can always revisit that when
> > > > an actual use case for more than 64 bits arises ...
> > > >
> > >
> > > You have got it completely backwards:
> > >
> > > CTS has never worked in any kernel implementation, so regardless of
> > > what the spec says, supporting it in the kernel is not a high priority
> > > issue whichever way you put it.
> > >
> > I never said it was a high priority, I merely pointed out it's not spec
> > compliant. Apparently, you feel that that's only important insofar that
> > it matches current kernel use cases?
> >
> > Anyway, as far as I understand, there are no users that need more than
> > 64 bits of IV in the kernel (i.e. dmcrypt uses only 64 bits), so I see
> > no fundamental difference with CTS except that most(?) implementations
> > possibly already "accidentally" (since unverified!) did it correctly.
> >
>
> Well, as Milan points out, dm-crypt may use the upper bits as well,
> but the point I was making was about the userland interface. If you
> care about spec compliance for the sake of it, I don't understand why
> you dismiss a broken userland->kernel interface that silently fails
> and corrupts your data as something we should just live with, and just
> assume nobody will ever be silly enough to use it the way the spec
> describes it, and use all the available IV bits.
>
Pfff ... taking my words waaay out of context ...

I believe I agreed that such a thing should not be silent.
And my argument was never that everyone should support everything, just
that a driver should be able to expose it's CTS capability if available.
If you don't support CTS, just return -EINVAL on non-16 byte multiples,
but don't FORCE a driver that DOES support it to do so for no reason.

> > Not that I have any interest in restricting the IV size: "my" hardware
> > handles full 128 bit IV's just fine. So why do I even bother ... :-)
> >
> > > Now is the first time anyone has asked
> > > for it in 12 years, and only because someone spotted a deviation
> > > between a h/w and a s/w implementation, not because anyone tried to
> > > use it and failed. (Note that passing anything other than a multiple
> > > of the block size will cause an error rather than fail silently)
> > >
> > Yes, failing silently is not such a good idea, I think we agree on that.
> > Although we also need to keep in mind that that's exactly what the CAAM
> > driver has been doing all those years, and, before my vectors, nobody
> > noticed or cared.
>
> True. But now that the cat is out of the bag, we have an obligation to
> our users to ensure that a known problem does not corrupt their data.
>
Again, it should indeed not fail silently. Returning an error should
prevent users from continuing to actually use it, as just building or
mounting the filesystem will already fail.

> > Without my involvement, this would have probably gone
> > unnoticed for many years to come (so I feel some responsibility ;-).
> >
> > > Truncated IVs are a huge issue, since we already expose the correct
> > > API via AF_ALG (without any restrictions on how many of the IV bits
> > > are populated), and apparently, if your AF_ALG request for xts(aes)
> > > happens to be fulfilled by the CAAM driver and your implementation
> > > uses more than 64 bits for the IV, the top bits get truncated silently
> > > and your data might get eaten.
> > >
> > Apparently, not such a "huge" issue at all, see previous remark.
> >
> > As a precaution, the CAAM driver could return -EINVAL if the upper IV
> > bytes are non-zero.
>
> Then we'd still have two implementations that behave differently, and
> this is not how abstractions work. If a driver implements xts(aes),
> then it should produce the same output (and return value) for every
> input.
>
Well, that's where I will continue to disagree with you.

I'm allergic to dogma, I like bending the rules a little. I believe in
flexibility and providing options, where possible and within reason.
I don't see why crypto drivers can't limit their functionality, so long
as these limitations are clear. I've argued this many times before, but
most HW crypto drivers are NOT selected by default, they are selected
EXPLICITLY (you can control this by keeping the priority relatively low)
by the user through some config file or commandline switch.
You can choose NOT to use them for a certain task (say dmcrypt or ipsec)
if they have known issues for that use case. Vendors can document this.
That's what we do all the time, document use cases and limitations for
our hardware.

But obviously, they should not fail silently, but provide meaningful
error responses instead, so it's immediately clear they don't work.

> > But then testmgr would have to do less strict error
> > return code checking so we don't force this upon drivers that CAN do it.
> >
> > Implementing a full SW fallback for that in the driver just seems like
> > massive overkill, as you normally specify the driver for dmcrypt explictly
> > anyway (or at least, you can do that if the default fails).
> >
>
> How would you find out if the default fails? Either it corrupts your
> data, or it fails with an error that is specific to CAAM, and so we
> would have to update the dm-crypt code to deal with en/decryption
> failures that can only occur in this particular case.
>
It would just return some error and dmcrypt would fail on that, as
it would have to do on any returned error code anyway.
Then you just configure dmcrypt to use some other driver for xts(aes).
It should not need to be more complex than that.

> This is
> *exactly* the thing I argued against before, when we discussed zero
> length inputs: the reason we want the workaround in the driver is
> because it is contained, and its peculiarities don't leak into other
> layers.
>
And I still very fundamentally disagree with you on that ... ;-)

> > I don't like the idea of HW drivers doing SW fallbacks because it clouds
> > the whole picture of what is actually done by a certain HW device.
> >
>
> Well, the reality is that we OS guys get to clean up after the h/w
> guys all the time.
> I agree that explicit failure is better than silent
> corruption, but that still means there are corner cases where things
> just don't work.
>
Corner cases relevant to specific use cases that you could document.
Many of these h/w drivers are only relevant to embedded systems, which
usually come with large stacks of documentation on how to use them.
It's not a problem unless it is the default selection and there is no
override. You could do stricter checks on implementations with the
highest priority that may end up becoming the default. Just a thought.

> So the only acceptable way to me is to fix this with
> a s/w workaround, and as I already pointed out, this could simply be
> the xts template wrapped around the accelerated ECB implementation
> exposed by the driver.
>
You could do that. It's just no fun writing and maintaining all that
code that you fundamentally don't want to be executed in the first
place. It seems like a huge waste of effort.
Also, for every XTS TFM, you always need the software implementation
on standby, consuming memory, with the key configured and the key
scheduler likely executed etc., just in case the upper 8 iv bytes are
nonzero. Which probably never happens anyway.

>
> > > In my experience, users tend to care more about the latter than the former.
> > >
> > >
> > > > > > > to provide a s/w fallback for these cases.
> > > > > > >
> > > > > > Yes, the plan is to:
> > > > > >
> > > > > > -add 16B IV support for caam versions supporting it - caam Era 9+,
> > > > > > currently deployed in lx2160a and ls108a
> > > > > >
> > > > > > -remove current 8B IV support and add s/w fallback for affected caam versions
> > > > > > I'd assume this could be done dynamically, i.e. depending on IV provided
> > > > > > in the crypto request to use either the caam engine or s/w fallback.
> > > > > >
> > > > >
> > > > > Yes. If the IV received from the caller has bytes 8..15 cleared, you
> > > > > use the limited XTS h/w implementation, otherwise you fall back to
> > > > > xts(ecb-aes-caam..).
> > > >
> > > > Regards,
> > > > Pascal van Leeuwen
> > > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> > > > http://www.insidesecure.com
> > > >
> >
> > Regards,
> > Pascal van Leeuwen
> > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> > http://www.insidesecure.com
> >

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com

2019-08-12 04:52:07

by Herbert Xu

[permalink] [raw]
Subject: Re: [dm-devel] xts fuzz testing and lack of ciphertext stealing support

On Sun, Aug 11, 2019 at 09:29:38PM +0000, Pascal Van Leeuwen wrote:
>
> It will very likely fail with that CAAM h/w, but that only proves that you
> should not use plain64be IV's together with CAAM h/w. Which should be

It doesn't matter whether it's wrong or not.

The fact is that this is an interface that we export to user-space
and we must NEVER break that. So if your driver is breaking this
interface then your driver is broken and needs to be fixed.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt