Hi,
I've ran into some problems when enabling the extended crypto tests
after commit 49763fc6b1af ("crypto: testmgr - generate inauthentic
AEAD test vectors").
After looking into the matter, I've found something that seems like a
possible problem with the tests, but I am not sure and would
appreciate your consideration:
include/crypto/aead.h has this piece of wisdom to offer:
"* It is important to note that if multiple scatter gather list entries form
* the input data mentioned above, the first entry must not point to a NULL
* buffer. If there is any potential where the AAD buffer can be NULL, the
* calling code must contain a precaution to ensure that this does not result
* in the first scatter gather list entry pointing to a NULL buffer."
However, in generate_random_aead_testvec() we have:
/* AAD, plaintext, and ciphertext lengths */
total_len = generate_random_length(maxdatasize);
if (prandom_u32() % 4 == 0)
vec->alen = 0;
else
vec->alen = generate_random_length(total_len);
vec->plen = total_len - vec->alen;
vec->clen = vec->plen + authsize;
Which later calls into generate_aead_message() that has:
int i = 0;
struct scatterlist src[2], dst;
u8 iv[MAX_IVLEN];
DECLARE_CRYPTO_WAIT(wait);
/* Generate a random plaintext and encrypt it. */
sg_init_table(src, 2);
if (vec->alen)
sg_set_buf(&src[i++], vec->assoc, vec->alen);
if (vec->plen) {
generate_random_bytes((u8 *)vec->ptext, vec->plen);
sg_set_buf(&src[i++], vec->ptext, vec->plen);
}
sg_init_one(&dst, vec->ctext, vec->alen + vec->clen);
memcpy(iv, vec->iv, ivsize);
aead_request_set_callback(req, 0, crypto_req_done, &wait);
aead_request_set_crypt(req, src, &dst, vec->plen, iv);
aead_request_set_ad(req, vec->alen);
vec->crypt_error = crypto_wait_req(crypto_aead_encrypt(req),
&wait);
When both vec->alen and vec->plen are 0, which can happen as
generate_random_bytes will happily generate zero length from time to
time,
we seem to be getting a scatterlist with the first entry (as well as
the 2nd) being a NULL.
This seems to violate the words of wisdom from aead.h and much more
important to me crashes the ccree driver :-)
Is there anything I am missing or is this a valid concern?
Thanks!
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
On Mon, Jan 27, 2020 at 10:04:26AM +0200, Gilad Ben-Yossef wrote:
>
> When both vec->alen and vec->plen are 0, which can happen as
> generate_random_bytes will happily generate zero length from time to
> time,
> we seem to be getting a scatterlist with the first entry (as well as
> the 2nd) being a NULL.
>
> This seems to violate the words of wisdom from aead.h and much more
> important to me crashes the ccree driver :-)
>
> Is there anything I am missing or is this a valid concern?
>
My understanding is that all crypto API functions that take scatterlists only
forbid zero-length scatterlist elements in the part of the scatterlist that's
actually passed to the API call. The input to these functions is never simply a
scatterlist, but rather a (scatterlist, length) pair. Algorithms shouldn't look
beyond 'length', so in the case of 'length == 0', they shouldn't look at the
scatterlist at all -- which may be just a NULL pointer.
If that's the case, there's no problem with this test code.
I'm not sure the comment in aead.h is relevant here. It sounds like it's
warning about not providing an empty scatterlist element for the AAD when it's
followed by a nonempty scatterlist element for the plaintext. I'm not sure it's
meant to also cover the case where both are empty.
Herbert and Stephan, any thoughts on what was intended?
- Eric
Am Dienstag, 28. Januar 2020, 03:34:55 CET schrieb Eric Biggers:
Hi Eric,
> On Mon, Jan 27, 2020 at 10:04:26AM +0200, Gilad Ben-Yossef wrote:
> > When both vec->alen and vec->plen are 0, which can happen as
> > generate_random_bytes will happily generate zero length from time to
> > time,
> > we seem to be getting a scatterlist with the first entry (as well as
> > the 2nd) being a NULL.
> >
> > This seems to violate the words of wisdom from aead.h and much more
> > important to me crashes the ccree driver :-)
> >
> > Is there anything I am missing or is this a valid concern?
>
> My understanding is that all crypto API functions that take scatterlists
> only forbid zero-length scatterlist elements in the part of the scatterlist
> that's actually passed to the API call. The input to these functions is
> never simply a scatterlist, but rather a (scatterlist, length) pair.
> Algorithms shouldn't look beyond 'length', so in the case of 'length == 0',
> they shouldn't look at the scatterlist at all -- which may be just a NULL
> pointer.
>
> If that's the case, there's no problem with this test code.
I agree with your assessment. Not only when looking at cipher or template
implementations, but also when looking at the scatterwalk API the SGL length
field is processed first. If the length field is insufficient then the SGL is
not processed.
>
> I'm not sure the comment in aead.h is relevant here. It sounds like it's
> warning about not providing an empty scatterlist element for the AAD when
> it's followed by a nonempty scatterlist element for the plaintext. I'm not
> sure it's meant to also cover the case where both are empty.
The statement here (and maybe it could be updated) refers to a valid SGL with
a size > 0, but where the first SGL entry points to a NULL buffer. This is an
invalid use of an SGL.
Specifically for AEAD, the SGL must have the form of (assoc data ||
plaintext). As the AAD is not required for a successful cipher operation, the
caller of the crypto API must guarantee the AAD is either non-NULL or the SGL
must start with the plaintext as the first entry.
>
> Herbert and Stephan, any thoughts on what was intended?
>
> - Eric
Ciao
Stephan
On Mon, Jan 27, 2020 at 06:34:55PM -0800, Eric Biggers wrote:
>
> My understanding is that all crypto API functions that take scatterlists only
> forbid zero-length scatterlist elements in the part of the scatterlist that's
> actually passed to the API call. The input to these functions is never simply a
> scatterlist, but rather a (scatterlist, length) pair. Algorithms shouldn't look
> beyond 'length', so in the case of 'length == 0', they shouldn't look at the
> scatterlist at all -- which may be just a NULL pointer.
>
> If that's the case, there's no problem with this test code.
>
> I'm not sure the comment in aead.h is relevant here. It sounds like it's
> warning about not providing an empty scatterlist element for the AAD when it's
> followed by a nonempty scatterlist element for the plaintext. I'm not sure it's
> meant to also cover the case where both are empty.
>
> Herbert and Stephan, any thoughts on what was intended?
I agree. I think this is a bug in the driver.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, Jan 28, 2020 at 5:39 AM Herbert Xu <[email protected]> wrote:
>
> On Mon, Jan 27, 2020 at 06:34:55PM -0800, Eric Biggers wrote:
> >
> > My understanding is that all crypto API functions that take scatterlists only
> > forbid zero-length scatterlist elements in the part of the scatterlist that's
> > actually passed to the API call. The input to these functions is never simply a
> > scatterlist, but rather a (scatterlist, length) pair. Algorithms shouldn't look
> > beyond 'length', so in the case of 'length == 0', they shouldn't look at the
> > scatterlist at all -- which may be just a NULL pointer.
> >
> > If that's the case, there's no problem with this test code.
> >
> > I'm not sure the comment in aead.h is relevant here. It sounds like it's
> > warning about not providing an empty scatterlist element for the AAD when it's
> > followed by a nonempty scatterlist element for the plaintext. I'm not sure it's
> > meant to also cover the case where both are empty.
> >
> > Herbert and Stephan, any thoughts on what was intended?
>
> I agree. I think this is a bug in the driver.
>
Yes, I agree. After debugging it yesterday along with a similar but
not identical issue with the help of Geert it's a bug in the driver
and will send a fix to the root cause shortly.
<rant>
However while working on debugging this it became obvious to me how
convoluted are the requirements for what to expect from the source
scatterlist of an AEAD request from the transformation provider driver
point of view:
- The source is presumed to have enough room for both the associated
data and the plaintext.
- Unless it's in-place encryption, in which case, you also presume to
have room for the authentication tag
- The only way to tell if this is in-place encryption or not is to
compare the pointers to the source and destination - there is no flag.
- Also, if we happen to be dealing with RFC 4106, you also need to
presume to have room for the IV.
- You can count on the scattergather list not having a first NULL
buffer, *unless* the plaintext and associated data length are both
zero AND it's not in place encryption.
- You can count on not getting NULL as a scatterlist point, *unless*
the plaintext and associated data length are both zero AND it's not in
place encryption. (I'm actually unsure of this one?)
- The behavior of mapping scattergather lists is dependent on the
architecture, platform and configuration - e.g. even turning on
scatterlist DMA mapping debug option did not detect the issue that
Geert is seeing on his arm64 board that do not appear in mine...
So it's no wonder in a sense we got it wrong and judging from some of
the commits for the other driver maintainer I'm not the only one.
I'm not sure there is something actionable here, maybe just clearer
documentation' but it is feel a somewhat brittle API to implement from
a security hardware driver perspective.
Oh well...
</rant>
Thank you all for your help!
Gilad
--
> Email: Herbert Xu <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> - The source is presumed to have enough room for both the associated
> data and the plaintext.
> - Unless it's in-place encryption, in which case, you also presume to
> have room for the authentication tag
The authentication tag is part of the ciphertext, not the plaintext. So the
rule is just that the ciphertext buffer needs to have room for it, not the
plaintext.
Of course, when doing in-place encryption/decryption, the two buffers are the
same, so both will have room for it, even though the tag is only meaningful on
the ciphertext side. That's just the logical consequence of "in-place".
> - The only way to tell if this is in-place encryption or not is to
> compare the pointers to the source and destination - there is no flag.
Requiring users to remember to provide a flag to indicate in-place
encryption/decryption, in addition to passing the same scatterlist, would make
the API more complex.
> - You can count on the scattergather list not having a first NULL
> buffer, *unless* the plaintext and associated data length are both
> zero AND it's not in place encryption.
> - You can count on not getting NULL as a scatterlist point, *unless*
> the plaintext and associated data length are both zero AND it's not in
> place encryption. (I'm actually unsure of this one?)
If we consider that the input is not just a scatterlist, but rather a
scatterlist and a length, then these observations are really just "you can
access the first byte, unless the length is 0" -- which is sort of obvious. And
requiring a dereferencable pointer for length = 0 is generally considered to be
bad API design; see the memcpy() fiasco
(https://www.imperialviolet.org/2016/06/26/nonnull.html).
The API could be simplified by only supporting full scatterlists, but it seems
that users are currently relying on being able to encrypt/decrypt just a prefix.
IMO, the biggest problems with the AEAD API are actually things you didn't
mention, such as the fact that the AAD isn't given in a separate scatterlist,
and that the API only supports scatterlists and not virtual addresses (which
makes it difficult to use in some cases).
In any case we do need much better documentation. I'm planning to improve some
of the crypto API documentation, but I'll probably do the hash and skcipher
algorithm types first before getting to AEAD. So if you want to improve the
AEAD documentation in the mean time, please go ahead.
- Eric
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Eric Biggers
> Sent: Tuesday, January 28, 2020 10:13 PM
> To: Gilad Ben-Yossef <[email protected]>
> Cc: Herbert Xu <[email protected]>; Stephan Mueller <[email protected]>; Linux Crypto Mailing List <linux-
> [email protected]>; Geert Uytterhoeven <[email protected]>; David Miller <[email protected]>; Ofir Drang
> <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> > - The source is presumed to have enough room for both the associated
> > data and the plaintext.
> > - Unless it's in-place encryption, in which case, you also presume to
> > have room for the authentication tag
>
> The authentication tag is part of the ciphertext, not the plaintext. So the
> rule is just that the ciphertext buffer needs to have room for it, not the
> plaintext.
>
> Of course, when doing in-place encryption/decryption, the two buffers are the
> same, so both will have room for it, even though the tag is only meaningful on
> the ciphertext side. That's just the logical consequence of "in-place".
>
> > - The only way to tell if this is in-place encryption or not is to
> > compare the pointers to the source and destination - there is no flag.
>
> Requiring users to remember to provide a flag to indicate in-place
> encryption/decryption, in addition to passing the same scatterlist, would make
> the API more complex.
>
Also, what would the benefit? You'd still have to compare the flag. The performance
difference of comparing the flag vs comparing 2 pointers (that you need to read anyway)
is likely completely negligible on most modern CPU architectures ...
> > - You can count on the scattergather list not having a first NULL
> > buffer, *unless* the plaintext and associated data length are both
> > zero AND it's not in place encryption.
> > - You can count on not getting NULL as a scatterlist point, *unless*
> > the plaintext and associated data length are both zero AND it's not in
> > place encryption. (I'm actually unsure of this one?)
>
> If we consider that the input is not just a scatterlist, but rather a
> scatterlist and a length, then these observations are really just "you can
> access the first byte, unless the length is 0" -- which is sort of obvious. And
> requiring a dereferencable pointer for length = 0 is generally considered to be
> bad API design; see the memcpy() fiasco
> (https://www.imperialviolet.org/2016/06/26/nonnull.html).
>
> The API could be simplified by only supporting full scatterlists, but it seems
> that users are currently relying on being able to encrypt/decrypt just a prefix.
>
> IMO, the biggest problems with the AEAD API are actually things you didn't
> mention, such as the fact that the AAD isn't given in a separate scatterlist,
>
While I can understand this may be beneficial in some cases, I believe they do not
outweigh the downsides:
- In many use cases, AAD+cipher text are stored as one contiguous string. Requiring this
string to be spit into seperate particles for AAD and ciphertext would be a burden.
- For hardware accelerators, there is a cost associated with each additional particle, in
terms of either bandwidth or performance or both. So less particles = better, generally.
The only thing that I find odd is that if you do a non-inplace operation you have this
undefined(?) gap in the output data where the AAD would be for inplace. That makes
little sense to me and requires extra effort to skip over in the driver.
> and that the API only supports scatterlists and not virtual addresses (which
> makes it difficult to use in some cases).
>
While I can understand that this is difficult if the API user just got this virtual address
provided from somewhere else and needs to do the translation, the other side of the
medal is that any hardware driver would otherwise have to do address translation and
scatterlist building on the fly (as hardware needs to access contiguous physical memory),
which would be real burden there. While many API users_are_ able to provide a nice
scatterlist at negligible extra cost. So why burden those?
> In any case we do need much better documentation. I'm planning to improve some
> of the crypto API documentation, but I'll probably do the hash and skcipher
> algorithm types first before getting to AEAD. So if you want to improve the
> AEAD documentation in the mean time, please go ahead.
>
> - Eric
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
Am Mittwoch, 29. Januar 2020, 01:18:29 CET schrieb Van Leeuwen, Pascal:
Hi Pascal,
> > -----Original Message-----
> > From: [email protected]
> > <[email protected]> On Behalf Of Eric Biggers
Sent:
> > Tuesday, January 28, 2020 10:13 PM
> > To: Gilad Ben-Yossef <[email protected]>
> > Cc: Herbert Xu <[email protected]>; Stephan Mueller
> > <[email protected]>; Linux Crypto Mailing List <linux-
> > [email protected]>; Geert Uytterhoeven <[email protected]>; David
> > Miller <[email protected]>; Ofir Drang <[email protected]>
> > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto
> > tests
>
> >
> >
> > <<< External Email >>>
> > CAUTION: This email originated from outside of the organization. Do not
> > click links or open attachments unless you recognize the
sender/sender
> > address and know the content is safe.
> >
> >
> >
> >
> > On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> >
> > > - The source is presumed to have enough room for both the associated
> > > data and the plaintext.
> > > - Unless it's in-place encryption, in which case, you also presume to
> > > have room for the authentication tag
> >
> >
> >
> > The authentication tag is part of the ciphertext, not the plaintext. So
> > the
rule is just that the ciphertext buffer needs to have room for it,
> > not the plaintext.
> >
> >
> >
> > Of course, when doing in-place encryption/decryption, the two buffers are
> > the
same, so both will have room for it, even though the tag is only
> > meaningful on the ciphertext side. That's just the logical consequence
> > of "in-place".>
> >
> >
> > > - The only way to tell if this is in-place encryption or not is to
> > > compare the pointers to the source and destination - there is no flag.
> >
> >
> >
> > Requiring users to remember to provide a flag to indicate in-place
> > encryption/decryption, in addition to passing the same scatterlist, would
> > make
the API more complex.
> >
> >
>
> Also, what would the benefit? You'd still have to compare the flag. The
> performance
difference of comparing the flag vs comparing 2 pointers (that
> you need to read anyway) is likely completely negligible on most modern CPU
> architectures ...
>
> > > - You can count on the scattergather list not having a first NULL
> > > buffer, *unless* the plaintext and associated data length are both
> > > zero AND it's not in place encryption.
> > > - You can count on not getting NULL as a scatterlist point, *unless*
> > > the plaintext and associated data length are both zero AND it's not in
> > > place encryption. (I'm actually unsure of this one?)
> >
> >
> >
> > If we consider that the input is not just a scatterlist, but rather a
> > scatterlist and a length, then these observations are really just "you
> > can
> > access the first byte, unless the length is 0" -- which is sort of
> > obvious. And requiring a dereferencable pointer for length = 0 is
> > generally considered to be bad API design; see the memcpy() fiasco
> > (https://www.imperialviolet.org/2016/06/26/nonnull.html).
> >
> >
> >
> > The API could be simplified by only supporting full scatterlists, but it
> > seems that users are currently relying on being able to encrypt/decrypt
> > just a prefix.>
> >
> >
> > IMO, the biggest problems with the AEAD API are actually things you
> > didn't
> > mention, such as the fact that the AAD isn't given in a separate
> > scatterlist,
>
> >
>
> While I can understand this may be beneficial in some cases, I believe they
> do not
> outweigh the downsides:
> - In many use cases, AAD+cipher text are stored as one contiguous string.
Then refer to that one linear buffer with one SGL entry.
> Requiring this
> string to be spit into seperate particles for AAD and
> ciphertext would be a burden.
There is no need to split a string. All that is said is that the SGL needs to
point to memory that is AAD||PT or AAD||CT||TAG. There is no statement about
the number of SGL entries to point to these buffer(s). So you could have one
linear buffer for these components pointing to it with an SGL holding one
entry.
> - For hardware accelerators, there is a cost
> associated with each additional particle, in terms of either bandwidth or
> performance or both. So less particles = better, generally.
> The only thing that I find odd is that if you do a non-inplace operation you
> have this
> undefined(?) gap in the output data where the AAD would be for
> inplace. That makes little sense to me and requires extra effort to skip
> over in the driver.
>
> > and that the API only supports scatterlists and not virtual addresses
> > (which makes it difficult to use in some cases).
> >
> >
>
> While I can understand that this is difficult if the API user just got this
> virtual address
provided from somewhere else and needs to do the
> translation, the other side of the medal is that any hardware driver would
> otherwise have to do address translation and scatterlist building on the
> fly (as hardware needs to access contiguous physical memory), which would
> be real burden there. While many API users_are_ able to provide a nice
> scatterlist at negligible extra cost. So why burden those?
>
>
> > In any case we do need much better documentation. I'm planning to improve
> > some
of the crypto API documentation, but I'll probably do the hash and
> > skcipher algorithm types first before getting to AEAD. So if you want to
> > improve the AEAD documentation in the mean time, please go ahead.
> >
> >
> >
> > - Eric
>
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect Multi-Protocol Engines, Rambus Security
> Rambus ROTW Holding BV
> +31-73 6581953
>
> Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by
> Rambus.
Please be so kind to update your e-mail address book with my new
> e-mail address.
>
> ** This message and any attachments are for the sole use of the intended
> recipient(s). It may contain information that is confidential and
> privileged. If you are not the intended recipient of this message, you are
> prohibited from printing, copying, forwarding or saving it. Please delete
> the message and attachments and notify the sender immediately. **
> Rambus Inc.<http://www.rambus.com>
Ciao
Stephan
Hi Stephan,
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Stephan Mueller
> Sent: Wednesday, January 29, 2020 2:27 AM
> To: Van Leeuwen, Pascal <[email protected]>
> Cc: Eric Biggers <[email protected]>; Gilad Ben-Yossef <[email protected]>; Herbert Xu <[email protected]>;
> Linux Crypto Mailing List <[email protected]>; Geert Uytterhoeven <[email protected]>; David Miller
> <[email protected]>; Ofir Drang <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> Am Mittwoch, 29. Januar 2020, 01:18:29 CET schrieb Van Leeuwen, Pascal:
>
> Hi Pascal,
>
> > > -----Original Message-----
> > > From: [email protected]
> > > <[email protected]> On Behalf Of Eric Biggers
> Sent:
> > > Tuesday, January 28, 2020 10:13 PM
> > > To: Gilad Ben-Yossef <[email protected]>
> > > Cc: Herbert Xu <[email protected]>; Stephan Mueller
> > > <[email protected]>; Linux Crypto Mailing List <linux-
> > > [email protected]>; Geert Uytterhoeven <[email protected]>; David
> > > Miller <[email protected]>; Ofir Drang <[email protected]>
> > > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto
> > > tests
> >
> > >
> > >
> > > <<< External Email >>>
> > > CAUTION: This email originated from outside of the organization. Do not
> > > click links or open attachments unless you recognize the
> sender/sender
> > > address and know the content is safe.
> > >
> > >
> > >
> > >
> > > On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> > >
> > > > - The source is presumed to have enough room for both the associated
> > > > data and the plaintext.
> > > > - Unless it's in-place encryption, in which case, you also presume to
> > > > have room for the authentication tag
> > >
> > >
> > >
> > > The authentication tag is part of the ciphertext, not the plaintext. So
> > > the
> rule is just that the ciphertext buffer needs to have room for it,
> > > not the plaintext.
> > >
> > >
> > >
> > > Of course, when doing in-place encryption/decryption, the two buffers are
> > > the
> same, so both will have room for it, even though the tag is only
> > > meaningful on the ciphertext side. That's just the logical consequence
> > > of "in-place".>
> > >
> > >
> > > > - The only way to tell if this is in-place encryption or not is to
> > > > compare the pointers to the source and destination - there is no flag.
> > >
> > >
> > >
> > > Requiring users to remember to provide a flag to indicate in-place
> > > encryption/decryption, in addition to passing the same scatterlist, would
> > > make
> the API more complex.
> > >
> > >
> >
> > Also, what would the benefit? You'd still have to compare the flag. The
> > performance
> difference of comparing the flag vs comparing 2 pointers (that
> > you need to read anyway) is likely completely negligible on most modern CPU
> > architectures ...
> >
> > > > - You can count on the scattergather list not having a first NULL
> > > > buffer, *unless* the plaintext and associated data length are both
> > > > zero AND it's not in place encryption.
> > > > - You can count on not getting NULL as a scatterlist point, *unless*
> > > > the plaintext and associated data length are both zero AND it's not in
> > > > place encryption. (I'm actually unsure of this one?)
> > >
> > >
> > >
> > > If we consider that the input is not just a scatterlist, but rather a
> > > scatterlist and a length, then these observations are really just "you
> > > can
> > > access the first byte, unless the length is 0" -- which is sort of
> > > obvious. And requiring a dereferencable pointer for length = 0 is
> > > generally considered to be bad API design; see the memcpy() fiasco
> > > (https://www.imperialviolet.org/2016/06/26/nonnull.html).
> > >
> > >
> > >
> > > The API could be simplified by only supporting full scatterlists, but it
> > > seems that users are currently relying on being able to encrypt/decrypt
> > > just a prefix.>
> > >
> > >
> > > IMO, the biggest problems with the AEAD API are actually things you
> > > didn't
> > > mention, such as the fact that the AAD isn't given in a separate
> > > scatterlist,
> >
> > >
> >
> > While I can understand this may be beneficial in some cases, I believe they
> > do not
> > outweigh the downsides:
> > - In many use cases, AAD+cipher text are stored as one contiguous string.
>
> Then refer to that one linear buffer with one SGL entry.
>
Hmm ... I believe having a seperate scatter list for AAD would imply that you have
seperate scatter entries for AAD (in that list) and Crypto[+TAG] (in the other list).
So you still have the burden of constructing 2 scatterlists instead of one, figuring
out where the second one starts. Plus the burden of any hardware accelerator
having to handle 2 particles instead of one.
Note that even with one scatterlist you can still have the AAD data coming from
some specific AAD-only buffer(s). Just put it it its own (set of) particle(s), seperate
from the crypto data particles. So that is not a reason to have seperate *lists*.
The only advantage of having AAD seperate I can think of is for software
crypto implementations, not having to skip over the AAD for the scatterlist they
send to the parallel encryption part. Which IMHO is only a minor inconvenience
that you shouldn't push to all the users of the API.
> > Requiring this
> > string to be spit into seperate particles for AAD and
> > ciphertext would be a burden.
>
> There is no need to split a string. All that is said is that the SGL needs to
> point to memory that is AAD||PT or AAD||CT||TAG. There is no statement about
> the number of SGL entries to point to these buffer(s). So you could have one
> linear buffer for these components pointing to it with an SGL holding one
> entry.
>
The remark I responded to was about having a seperate scatterlist for AAD data.
Which, in my world, implies that the *other* scatterlist does NOT include the AAD
data. So that one would then need to be only PT or CT||TAG. Which does require
"splitting the string" (virtually, anyway) between AAD and PT/CT.
It's not about splitting the data physically (i.e. moving it). It's about splitting the
particles, creating 2 particles (in 2 lists) where you would now only need 1.
> > - For hardware accelerators, there is a cost
> > associated with each additional particle, in terms of either bandwidth or
> > performance or both. So less particles = better, generally.
> > The only thing that I find odd is that if you do a non-inplace operation you
> > have this
> > undefined(?) gap in the output data where the AAD would be for
> > inplace. That makes little sense to me and requires extra effort to skip
> > over in the driver.
> >
> > > and that the API only supports scatterlists and not virtual addresses
> > > (which makes it difficult to use in some cases).
> > >
> > >
> >
> > While I can understand that this is difficult if the API user just got this
> > virtual address
> provided from somewhere else and needs to do the
> > translation, the other side of the medal is that any hardware driver would
> > otherwise have to do address translation and scatterlist building on the
> > fly (as hardware needs to access contiguous physical memory), which would
> > be real burden there. While many API users_are_ able to provide a nice
> > scatterlist at negligible extra cost. So why burden those?
> >
> >
> > > In any case we do need much better documentation. I'm planning to improve
> > > some
> of the crypto API documentation, but I'll probably do the hash and
> > > skcipher algorithm types first before getting to AEAD. So if you want to
> > > improve the AEAD documentation in the mean time, please go ahead.
> > >
> > >
> > >
> > > - Eric
> >
> >
> > Regards,
> > Pascal van Leeuwen
> > Silicon IP Architect Multi-Protocol Engines, Rambus Security
> > Rambus ROTW Holding BV
> > +31-73 6581953
> >
> > Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by
> > Rambus.
> Please be so kind to update your e-mail address book with my new
> > e-mail address.
> >
> > ** This message and any attachments are for the sole use of the intended
> > recipient(s). It may contain information that is confidential and
> > privileged. If you are not the intended recipient of this message, you are
> > prohibited from printing, copying, forwarding or saving it. Please delete
> > the message and attachments and notify the sender immediately. **
>
> > Rambus Inc.<http://www.rambus.com>
>
>
>
> Ciao
> Stephan
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
On Tue, Jan 28, 2020 at 11:12 PM Eric Biggers <[email protected]> wrote:
>
> On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> > - The source is presumed to have enough room for both the associated
> > data and the plaintext.
> > - Unless it's in-place encryption, in which case, you also presume to
> > have room for the authentication tag
>
> The authentication tag is part of the ciphertext, not the plaintext. So the
> rule is just that the ciphertext buffer needs to have room for it, not the
> plaintext.
>
> Of course, when doing in-place encryption/decryption, the two buffers are the
> same, so both will have room for it, even though the tag is only meaningful on
> the ciphertext side. That's just the logical consequence of "in-place".
Yes, of course. I understand the purpose all of this serves.
>
> > - The only way to tell if this is in-place encryption or not is to
> > compare the pointers to the source and destination - there is no flag.
>
> Requiring users to remember to provide a flag to indicate in-place
> encryption/decryption, in addition to passing the same scatterlist, would make
> the API more complex.
>
Asking the user to provide the flag is throwing the problem at the user -
so indeed, not a good idea. But that still doesn't mean we need to have
"rea->src == req->dst" in every driver. We can have the API framework
do this.
> > - You can count on the scattergather list not having a first NULL
> > buffer, *unless* the plaintext and associated data length are both
> > zero AND it's not in place encryption.
> > - You can count on not getting NULL as a scatterlist point, *unless*
> > the plaintext and associated data length are both zero AND it's not in
> > place encryption. (I'm actually unsure of this one?)
>
> If we consider that the input is not just a scatterlist, but rather a
> scatterlist and a length, then these observations are really just "you can
> access the first byte, unless the length is 0" -- which is sort of obvious. And
Yes, if it is indeed a scatterlist and length. In fact it isn't - it's
a scatterlist
and four different lengths: plaintext, associated data, IV and auth tag.
Some of them are used in various scenarios and some aren't.
Which is exactly my point.
> requiring a dereferencable pointer for length = 0 is generally considered to be
> bad API design; see the memcpy() fiasco
> (https://www.imperialviolet.org/2016/06/26/nonnull.html).
Yes, that's not a good option - but neither is having a comment that
can be read to imply
that the API requires it if it doesn't :-)
Thinking about it, I'm wondering if having something like this will
save boilerplate code in many drivers:
static inline bool crypto_aead_inplace(struct aead_request req)
{
return (req->src == req->dst);
}
unsigned int crypto_aead_sg_len(struct aead_request req, bool enc, bool src,
int authsize, bool need_iv)
{
struct crypto_aead *tfm = crypto_aead_reqtfm(req);
unsigned int len = req->assoclen + req->cryptlen;
if (need_iv)
len += crypto_aead_ivsize(tfm);
if (src && !enc) || (!src && enc) || crypto_aead_inplace(req))
len += authsize;
return len;
}
It would be better even if we can put the authsize and need_iv into the tfv
at registration time and not have to pass them as parameters at all.
<snip>
Anyways, thanks for entertaining my ramblings... :-)
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
Am Mittwoch, 29. Januar 2020, 09:40:28 CET schrieb Van Leeuwen, Pascal:
Hi Pascal,
> Hi Stephan,
>
>
> > -----Original Message-----
> > From: [email protected]
> > <[email protected]> On Behalf Of Stephan Mueller
Sent:
> > Wednesday, January 29, 2020 2:27 AM
> > To: Van Leeuwen, Pascal <[email protected]>
> > Cc: Eric Biggers <[email protected]>; Gilad Ben-Yossef
> > <[email protected]>; Herbert Xu <[email protected]>;
Linux
> > Crypto Mailing List <[email protected]>; Geert Uytterhoeven
> > <[email protected]>; David Miller <[email protected]>; Ofir Drang
> > <[email protected]>
> > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto
> > tests
>
> >
> >
> > <<< External Email >>>
> > CAUTION: This email originated from outside of the organization. Do not
> > click links or open attachments unless you recognize the
sender/sender
> > address and know the content is safe.
> >
> >
> >
> >
> > Am Mittwoch, 29. Januar 2020, 01:18:29 CET schrieb Van Leeuwen, Pascal:
> >
> >
> >
> > Hi Pascal,
> >
> >
> >
> > > > -----Original Message-----
> > > > From: [email protected]
> > > > <[email protected]> On Behalf Of Eric Biggers
> >
> > Sent:
> >
> > > > Tuesday, January 28, 2020 10:13 PM
> > > > To: Gilad Ben-Yossef <[email protected]>
> > > > Cc: Herbert Xu <[email protected]>; Stephan Mueller
> > > > <[email protected]>; Linux Crypto Mailing List <linux-
> > > > [email protected]>; Geert Uytterhoeven <[email protected]>;
> > > > David
> > > > Miller <[email protected]>; Ofir Drang <[email protected]>
> > > > Subject: Re: Possible issue with new inauthentic AEAD in extended
> > > > crypto
> > > > tests
> > >
> > >
> > >
> > > >
> > > >
> > > >
> > > > <<< External Email >>>
> > > > CAUTION: This email originated from outside of the organization. Do
> > > > not
> > > > click links or open attachments unless you recognize the
> >
> > sender/sender
> >
> > > > address and know the content is safe.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> > > >
> > > >
> > > >
> > > > > - The source is presumed to have enough room for both the
> > > > > associated
> > > > > data and the plaintext.
> > > > > - Unless it's in-place encryption, in which case, you also presume
> > > > > to
> > > > > have room for the authentication tag
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > The authentication tag is part of the ciphertext, not the plaintext.
> > > > So
> > > > the
> >
> > rule is just that the ciphertext buffer needs to have room for it,
> >
> > > > not the plaintext.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > Of course, when doing in-place encryption/decryption, the two buffers
> > > > are
> > > > the
> >
> > same, so both will have room for it, even though the tag is only
> >
> > > > meaningful on the ciphertext side. That's just the logical
> > > > consequence
> > > > of "in-place".>
> > > >
> > > >
> > > >
> > > >
> > > > > - The only way to tell if this is in-place encryption or not is to
> > > > > compare the pointers to the source and destination - there is no
> > > > > flag.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > Requiring users to remember to provide a flag to indicate in-place
> > > > encryption/decryption, in addition to passing the same scatterlist,
> > > > would
> > > > make
> >
> > the API more complex.
> >
> > > >
> > > >
> > >
> > >
> > >
> > > Also, what would the benefit? You'd still have to compare the flag. The
> > > performance
> >
> > difference of comparing the flag vs comparing 2 pointers (that
> >
> > > you need to read anyway) is likely completely negligible on most modern
> > > CPU
architectures ...
> > >
> > >
> > >
> > > > > - You can count on the scattergather list not having a first NULL
> > > > > buffer, *unless* the plaintext and associated data length are both
> > > > > zero AND it's not in place encryption.
> > > > > - You can count on not getting NULL as a scatterlist point,
> > > > > *unless*
> > > > > the plaintext and associated data length are both zero AND it's not
> > > > > in
> > > > > place encryption. (I'm actually unsure of this one?)
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > If we consider that the input is not just a scatterlist, but rather a
> > > > scatterlist and a length, then these observations are really just
> > > > "you
> > > > can
> > > > access the first byte, unless the length is 0" -- which is sort of
> > > > obvious. And requiring a dereferencable pointer for length = 0 is
> > > > generally considered to be bad API design; see the memcpy() fiasco
> > > > (https://www.imperialviolet.org/2016/06/26/nonnull.html).
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > The API could be simplified by only supporting full scatterlists, but
> > > > it
> > > > seems that users are currently relying on being able to
> > > > encrypt/decrypt
> > > > just a prefix.>
> > > >
> > > >
> > > >
> > > >
> > > > IMO, the biggest problems with the AEAD API are actually things you
> > > > didn't
> > > > mention, such as the fact that the AAD isn't given in a separate
> > > > scatterlist,
> > >
> > >
> > >
> > > >
> > >
> > >
> > >
> > > While I can understand this may be beneficial in some cases, I believe
> > > they
do not
> > > outweigh the downsides:
> > > - In many use cases, AAD+cipher text are stored as one contiguous
> > > string.
> >
> >
> >
> > Then refer to that one linear buffer with one SGL entry.
> >
> >
>
> Hmm ... I believe having a seperate scatter list for AAD would imply that
> you have
seperate scatter entries for AAD (in that list) and Crypto[+TAG]
> (in the other list).
Who says that we need a separate SGL entry for the AAD?
> So you still have the burden of constructing 2
> scatterlists instead of one, figuring out where the second one starts.
I do not see the requirement that the caller must have at least two SGL
entries.
In fact, for the AF_ALG interface, af_alg_get_rsgl creates the destination SGL
and creates one SGL entry per user-space IOVEC. If user space provides a
linear buffer with one IOVEC holding the AAD, CT, Tag, only one SGL entry is
created.
For the source SGL, af_alg_sendmsg tries to be efficient to put as much as
possible into one page referenced by one SGL entry. So, if user space provides
AAD||PT which is less than a page in size, you get one SGL entry for the
entire input data.
> Plus
> the burden of any hardware accelerator having to handle 2 particles instead
> of one.
Well, the cipher implementation must be capable of processing any SGL
structure. It is not given that the SGL with the source data has exactly 2
entries. It can have one entry with AAD||PT. It can have two entries where the
split is between AAD and PT. But it can have 2 entries where the split is in
the middle of, say, AAD. Or it can have more SGL entries.
Please do not mix up the structure of the data to be contained in the SGL
(say, AAD||PT) with the physical memory structure (e.g. how many SGL entries
there are).
>
> Note that even with one scatterlist you can still have the AAD data coming
> from
some specific AAD-only buffer(s). Just put it it its own (set of)
> particle(s), seperate from the crypto data particles. So that is not a
> reason to have seperate *lists*.
> The only advantage of having AAD seperate I can think of is for software
> crypto implementations, not having to skip over the AAD for the scatterlist
> they
send to the parallel encryption part. Which IMHO is only a minor
> inconvenience that you shouldn't push to all the users of the API.
>
>
> > > Requiring this
> > > string to be spit into seperate particles for AAD and
> > > ciphertext would be a burden.
> >
> >
> >
> > There is no need to split a string. All that is said is that the SGL needs
> > to
point to memory that is AAD||PT or AAD||CT||TAG. There is no
> > statement about the number of SGL entries to point to these buffer(s). So
> > you could have one linear buffer for these components pointing to it with
> > an SGL holding one entry.
> >
> >
>
> The remark I responded to was about having a seperate scatterlist for AAD
> data.
Which, in my world, implies that the *other* scatterlist does NOT
> include the AAD data. So that one would then need to be only PT or CT||TAG.
> Which does require "splitting the string" (virtually, anyway) between AAD
> and PT/CT.
> It's not about splitting the data physically (i.e. moving it). It's about
> splitting the
particles, creating 2 particles (in 2 lists) where you would
> now only need 1.
>
> > > - For hardware accelerators, there is a cost
> > > associated with each additional particle, in terms of either bandwidth
> > > or
> > > performance or both. So less particles = better, generally.
> > > The only thing that I find odd is that if you do a non-inplace operation
> > > you
have this
> > > undefined(?) gap in the output data where the AAD would be for
> > > inplace. That makes little sense to me and requires extra effort to
> > > skip
> > > over in the driver.
> > >
> > >
> > >
> > > > and that the API only supports scatterlists and not virtual addresses
> > > > (which makes it difficult to use in some cases).
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > > While I can understand that this is difficult if the API user just got
> > > this
virtual address
> >
> > provided from somewhere else and needs to do the
> >
> > > translation, the other side of the medal is that any hardware driver
> > > would
> > > otherwise have to do address translation and scatterlist building on
> > > the
> > > fly (as hardware needs to access contiguous physical memory), which
> > > would
> > > be real burden there. While many API users_are_ able to provide a nice
> > > scatterlist at negligible extra cost. So why burden those?
> > >
> > >
> > >
> > >
> > > > In any case we do need much better documentation. I'm planning to
> > > > improve
> > > > some
> >
> > of the crypto API documentation, but I'll probably do the hash and
> >
> > > > skcipher algorithm types first before getting to AEAD. So if you want
> > > > to
> > > > improve the AEAD documentation in the mean time, please go ahead.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > - Eric
> > >
> > >
> > >
> > >
> > > Regards,
> > > Pascal van Leeuwen
> > > Silicon IP Architect Multi-Protocol Engines, Rambus Security
> > > Rambus ROTW Holding BV
> > > +31-73 6581953
> > >
> > >
> > >
> > > Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired
> > > by
Rambus.
> >
> > Please be so kind to update your e-mail address book with my new
> >
> > > e-mail address.
> > >
> > >
> > >
> > > ** This message and any attachments are for the sole use of the
> > > intended
> > > recipient(s). It may contain information that is confidential and
> > > privileged. If you are not the intended recipient of this message, you
> > > are
> > > prohibited from printing, copying, forwarding or saving it. Please
> > > delete
> > > the message and attachments and notify the sender immediately. **
> >
> >
> >
> > > Rambus Inc.<http://www.rambus.com>
> >
> >
> >
> >
> >
> > Ciao
> > Stephan
>
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect Multi-Protocol Engines, Rambus Security
> Rambus ROTW Holding BV
> +31-73 6581953
>
> Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by
> Rambus.
Please be so kind to update your e-mail address book with my new
> e-mail address.
>
> ** This message and any attachments are for the sole use of the intended
> recipient(s). It may contain information that is confidential and
> privileged. If you are not the intended recipient of this message, you are
> prohibited from printing, copying, forwarding or saving it. Please delete
> the message and attachments and notify the sender immediately. **
> Rambus Inc.<http://www.rambus.com>
Ciao
Stephan
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Gilad Ben-Yossef
> Sent: Wednesday, January 29, 2020 12:28 PM
> To: Eric Biggers <[email protected]>
> Cc: Herbert Xu <[email protected]>; Stephan Mueller <[email protected]>; Linux Crypto Mailing List <linux-
> [email protected]>; Geert Uytterhoeven <[email protected]>; David Miller <[email protected]>; Ofir Drang
> <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> On Tue, Jan 28, 2020 at 11:12 PM Eric Biggers <[email protected]> wrote:
> >
> > On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> > > - The source is presumed to have enough room for both the associated
> > > data and the plaintext.
> > > - Unless it's in-place encryption, in which case, you also presume to
> > > have room for the authentication tag
> >
> > The authentication tag is part of the ciphertext, not the plaintext. So the
> > rule is just that the ciphertext buffer needs to have room for it, not the
> > plaintext.
> >
> > Of course, when doing in-place encryption/decryption, the two buffers are the
> > same, so both will have room for it, even though the tag is only meaningful on
> > the ciphertext side. That's just the logical consequence of "in-place".
>
> Yes, of course. I understand the purpose all of this serves.
>
> >
> > > - The only way to tell if this is in-place encryption or not is to
> > > compare the pointers to the source and destination - there is no flag.
> >
> > Requiring users to remember to provide a flag to indicate in-place
> > encryption/decryption, in addition to passing the same scatterlist, would make
> > the API more complex.
> >
>
> Asking the user to provide the flag is throwing the problem at the user -
> so indeed, not a good idea. But that still doesn't mean we need to have
> "rea->src == req->dst" in every driver. We can have the API framework
> do this.
>
Which would mean the framework would do the pointer compare, set
the flag appropriately and then, on top of that, the driver still has to
check/compare that flag as well, i.e.
"if (inplace) { map bidirectional } else { map unidirectional };"
How would that be an improvement of any sort? It just adds overhead.
Especially for SW implementations that may not even need to know.
It's not like that single pointer compare is terribly complicated to do
or difficult to understand ...
> > > - Yo
u can count on the scattergather list not having a first NULL
> > > buffer, *unless* the plaintext and associated data length are both
> > > zero AND it's not in place encryption.
> > > - You can count on not getting NULL as a scatterlist point, *unless*
> > > the plaintext and associated data length are both zero AND it's not in
> > > place encryption. (I'm actually unsure of this one?)
> >
> > If we consider that the input is not just a scatterlist, but rather a
> > scatterlist and a length, then these observations are really just "you can
> > access the first byte, unless the length is 0" -- which is sort of obvious. And
>
> Yes, if it is indeed a scatterlist and length. In fact it isn't - it's
> a scatterlist
> and four different lengths: plaintext, associated data, IV and auth tag.
> Some of them are used in various scenarios and some aren't.
> Which is exactly my point.
>
Agreed that what is included in cryptlen is not consistent or obvious.
Either make it include ONLY the PT/CT data (as the name implies!), or
make it the full input length or something. (but it's too late for that now)
> > requiring a dereferencable pointer for length = 0 is generally considered to be
> > bad API design; see the memcpy() fiasco
> > (https://www.imperialviolet.org/2016/06/26/nonnull.html).
>
> Yes, that's not a good option - but neither is having a comment that
> can be read to imply
> that the API requires it if it doesn't :-)
>
Hmm ... why shouldn't you be allowed to be _more_ restrictive in your
documentation then your implementation? It's called erring on the safe
side. It happens all the time, if only to save verification effort for all those
additional corner cases :-)
> Thinking about it, I'm wondering if having something like this will
> save boilerplate code in many drivers:
>
> static inline bool crypto_aead_inplace(struct aead_request req)
> {
> return (req->src == req->dst);
> }
>
That would save only a few characters of typing unless you shorten that function
name ;-) And would it _really_ be more clear to the reader of the code?
> unsigned int crypto_aead_sg_len(struct aead_request req, bool enc, bool src,
> int authsize, bool need_iv)
> {
> struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> unsigned int len = req->assoclen + req->cryptlen;
>
> if (need_iv)
> len += crypto_aead_ivsize(tfm);
>
> if (src && !enc) || (!src && enc) || crypto_aead_inplace(req))
> len += authsize;
>
> return len;
> }
>
Interesting ... my hardware is _very_ sensitive to input length yet I only need
to ever do assoclen+cryptlen for that and that works fine? ...
So I don't understand the +ivsize and +authsize for src. Seems to be already included.
And for the decrypt destination size, you should need to do -authsize as the ICV is included
in cryptlen but not written out(?).
Other than that, the idea of having such a function available isn't bad, as long
as you make it inlineable as you need it in the critical path of the driver.
> It would be better even if we can put the authsize and need_iv into the tfv
> at registration time and not have to pass them as parameters at all.
>
Then again passing them as parameters may be better as they may be constant
in the specific path where the function is called. Allowing the function to be inlined
would then allow the compiler to optimize unnecessary computations and branches
away ..
> <snip>
>
> Anyways, thanks for entertaining my ramblings... :-)
>
> Thanks,
> Gilad
>
> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
>
> values of β will give rise to dom!
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
Stephan,
> -----Original Message-----
> From: Stephan Mueller <[email protected]>
> Sent: Wednesday, January 29, 2020 1:55 PM
> To: Van Leeuwen, Pascal <[email protected]>
> Cc: Eric Biggers <[email protected]>; Gilad Ben-Yossef <[email protected]>; Herbert Xu <[email protected]>;
> Linux Crypto Mailing List <[email protected]>; Geert Uytterhoeven <[email protected]>; David Miller
> <[email protected]>; Ofir Drang <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> Am Mittwoch, 29. Januar 2020, 09:40:28 CET schrieb Van Leeuwen, Pascal:
>
> Hi Pascal,
>
> > Hi Stephan,
> >
> >
> > > -----Original Message-----
> > > From: [email protected]
> > > <[email protected]> On Behalf Of Stephan Mueller
> Sent:
> > > Wednesday, January 29, 2020 2:27 AM
> > > To: Van Leeuwen, Pascal <[email protected]>
> > > Cc: Eric Biggers <[email protected]>; Gilad Ben-Yossef
> > > <[email protected]>; Herbert Xu <[email protected]>;
> Linux
> > > Crypto Mailing List <[email protected]>; Geert Uytterhoeven
> > > <[email protected]>; David Miller <[email protected]>; Ofir Drang
> > > <[email protected]>
> > > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto
> > > tests
> >
> > >
> > >
> > > <<< External Email >>>
> > > CAUTION: This email originated from outside of the organization. Do not
> > > click links or open attachments unless you recognize the
> sender/sender
> > > address and know the content is safe.
> > >
> > >
> > >
> > >
> > > Am Mittwoch, 29. Januar 2020, 01:18:29 CET schrieb Van Leeuwen, Pascal:
> > >
> > >
> > >
> > > Hi Pascal,
> > >
> > >
> > >
> > > > > -----Original Message-----
> > > > > From: [email protected]
> > > > > <[email protected]> On Behalf Of Eric Biggers
> > >
> > > Sent:
> > >
> > > > > Tuesday, January 28, 2020 10:13 PM
> > > > > To: Gilad Ben-Yossef <[email protected]>
> > > > > Cc: Herbert Xu <[email protected]>; Stephan Mueller
> > > > > <[email protected]>; Linux Crypto Mailing List <linux-
> > > > > [email protected]>; Geert Uytterhoeven <[email protected]>;
> > > > > David
> > > > > Miller <[email protected]>; Ofir Drang <[email protected]>
> > > > > Subject: Re: Possible issue with new inauthentic AEAD in extended
> > > > > crypto
> > > > > tests
> > > >
> > > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > > <<< External Email >>>
> > > > > CAUTION: This email originated from outside of the organization. Do
> > > > > not
> > > > > click links or open attachments unless you recognize the
> > >
> > > sender/sender
> > >
> > > > > address and know the content is safe.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> > > > >
> > > > >
> > > > >
> > > > > > - The source is presumed to have enough room for both the
> > > > > > associated
> > > > > > data and the plaintext.
> > > > > > - Unless it's in-place encryption, in which case, you also presume
> > > > > > to
> > > > > > have room for the authentication tag
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > The authentication tag is part of the ciphertext, not the plaintext.
> > > > > So
> > > > > the
> > >
> > > rule is just that the ciphertext buffer needs to have room for it,
> > >
> > > > > not the plaintext.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Of course, when doing in-place encryption/decryption, the two buffers
> > > > > are
> > > > > the
> > >
> > > same, so both will have room for it, even though the tag is only
> > >
> > > > > meaningful on the ciphertext side. That's just the logical
> > > > > consequence
> > > > > of "in-place".>
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > > - The only way to tell if this is in-place encryption or not is to
> > > > > > compare the pointers to the source and destination - there is no
> > > > > > flag.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Requiring users to remember to provide a flag to indicate in-place
> > > > > encryption/decryption, in addition to passing the same scatterlist,
> > > > > would
> > > > > make
> > >
> > > the API more complex.
> > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > Also, what would the benefit? You'd still have to compare the flag. The
> > > > performance
> > >
> > > difference of comparing the flag vs comparing 2 pointers (that
> > >
> > > > you need to read anyway) is likely completely negligible on most modern
> > > > CPU
> architectures ...
> > > >
> > > >
> > > >
> > > > > > - You can count on the scattergather list not having a first NULL
> > > > > > buffer, *unless* the plaintext and associated data length are both
> > > > > > zero AND it's not in place encryption.
> > > > > > - You can count on not getting NULL as a scatterlist point,
> > > > > > *unless*
> > > > > > the plaintext and associated data length are both zero AND it's not
> > > > > > in
> > > > > > place encryption. (I'm actually unsure of this one?)
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > If we consider that the input is not just a scatterlist, but rather a
> > > > > scatterlist and a length, then these observations are really just
> > > > > "you
> > > > > can
> > > > > access the first byte, unless the length is 0" -- which is sort of
> > > > > obvious. And requiring a dereferencable pointer for length = 0 is
> > > > > generally considered to be bad API design; see the memcpy() fiasco
> > > > > (https://www.imperialviolet.org/2016/06/26/nonnull.html).
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > The API could be simplified by only supporting full scatterlists, but
> > > > > it
> > > > > seems that users are currently relying on being able to
> > > > > encrypt/decrypt
> > > > > just a prefix.>
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > IMO, the biggest problems with the AEAD API are actually things you
> > > > > didn't
> > > > > mention, such as the fact that the AAD isn't given in a separate
> > > > > scatterlist,
> > > >
> > > >
> > > >
> > > > >
> > > >
> > > >
> > > >
> > > > While I can understand this may be beneficial in some cases, I believe
> > > > they
> do not
> > > > outweigh the downsides:
> > > > - In many use cases, AAD+cipher text are stored as one contiguous
> > > > string.
> > >
> > >
> > >
> > > Then refer to that one linear buffer with one SGL entry.
> > >
> > >
> >
> > Hmm ... I believe having a seperate scatter list for AAD would imply that
> > you have
> seperate scatter entries for AAD (in that list) and Crypto[+TAG]
> > (in the other list).
>
> Who says that we need a separate SGL entry for the AAD?
>
That's a statement from Eric I responded to:
"the fact that the AAD isn't given in a separate scatterlist".
He is clearly suggesting he would like to to have such a seperate
scatterlist and I'm just pointing out the downsides of _that_.
I understand that the _current_ API doesn't require any seperate SGL
entry, in fact I was arguing that it is therefore more efficient.
> > So you still have the burden of constructing 2
> > scatterlists instead of one, figuring out where the second one starts.
>
>
> I do not see the requirement that the caller must have at least two SGL
> entries.
>
> In fact, for the AF_ALG interface, af_alg_get_rsgl creates the destination SGL
> and creates one SGL entry per user-space IOVEC. If user space provides a
> linear buffer with one IOVEC holding the AAD, CT, Tag, only one SGL entry is
> created.
>
> For the source SGL, af_alg_sendmsg tries to be efficient to put as much as
> possible into one page referenced by one SGL entry. So, if user space provides
> AAD||PT which is less than a page in size, you get one SGL entry for the
> entire input data.
>
Yes, it tries to keep the # of particles down for efficiency, proving exactly my
point against requiring the AAD to be a seperate list (which, I emphasize, is
_not_ the case with the current API).
> > Plus
> > the burden of any hardware accelerator having to handle 2 particles instead
> > of one.
>
> Well, the cipher implementation must be capable of processing any SGL
> structure. It is not given that the SGL with the source data has exactly 2
> entries. It can have one entry with AAD||PT. It can have two entries where the
> split is between AAD and PT. But it can have 2 entries where the split is in
> the middle of, say, AAD. Or it can have more SGL entries.
>
There is a difference between handling something functionally correctly and
handling something efficiently. Each extra particle comes with an overhead.
> Please do not mix up the structure of the data to be contained in the SGL
> (say, AAD||PT) with the physical memory structure (e.g. how many SGL entries
> there are).
>
I do not. Except for that if you insist AAD to be a seperate list, as Eric suggested,
this automatically implies an extra particle. (unless it was already split off ...)
> >
> > Note that even with one scatterlist you can still have the AAD data coming
> > from
> some specific AAD-only buffer(s). Just put it it its own (set of)
> > particle(s), seperate from the crypto data particles. So that is not a
> > reason to have seperate *lists*.
> > The only advantage of having AAD seperate I can think of is for software
> > crypto implementations, not having to skip over the AAD for the scatterlist
> > they
> send to the parallel encryption part. Which IMHO is only a minor
> > inconvenience that you shouldn't push to all the users of the API.
> >
> >
> > > > Requiring this
> > > > string to be spit into seperate particles for AAD and
> > > > ciphertext would be a burden.
> > >
> > >
> > >
> > > There is no need to split a string. All that is said is that the SGL needs
> > > to
> point to memory that is AAD||PT or AAD||CT||TAG. There is no
> > > statement about the number of SGL entries to point to these buffer(s). So
> > > you could have one linear buffer for these components pointing to it with
> > > an SGL holding one entry.
> > >
> > >
> >
> > The remark I responded to was about having a seperate scatterlist for AAD
> > data.
> Which, in my world, implies that the *other* scatterlist does NOT
> > include the AAD data. So that one would then need to be only PT or CT||TAG.
> > Which does require "splitting the string" (virtually, anyway) between AAD
> > and PT/CT.
> > It's not about splitting the data physically (i.e. moving it). It's about
> > splitting the
> particles, creating 2 particles (in 2 lists) where you would
> > now only need 1.
> >
> > > > - For hardware accelerators, there is a cost
> > > > associated with each additional particle, in terms of either bandwidth
> > > > or
> > > > performance or both. So less particles = better, generally.
> > > > The only thing that I find odd is that if you do a non-inplace operation
> > > > you
> have this
> > > > undefined(?) gap in the output data where the AAD would be for
> > > > inplace. That makes little sense to me and requires extra effort to
> > > > skip
> > > > over in the driver.
> > > >
> > > >
> > > >
> > > > > and that the API only supports scatterlists and not virtual addresses
> > > > > (which makes it difficult to use in some cases).
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > While I can understand that this is difficult if the API user just got
> > > > this
> virtual address
> > >
> > > provided from somewhere else and needs to do the
> > >
> > > > translation, the other side of the medal is that any hardware driver
> > > > would
> > > > otherwise have to do address translation and scatterlist building on
> > > > the
> > > > fly (as hardware needs to access contiguous physical memory), which
> > > > would
> > > > be real burden there. While many API users_are_ able to provide a nice
> > > > scatterlist at negligible extra cost. So why burden those?
> > > >
> > > >
> > > >
> > > >
> > > > > In any case we do need much better documentation. I'm planning to
> > > > > improve
> > > > > some
> > >
> > > of the crypto API documentation, but I'll probably do the hash and
> > >
> > > > > skcipher algorithm types first before getting to AEAD. So if you want
> > > > > to
> > > > > improve the AEAD documentation in the mean time, please go ahead.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > - Eric
> > > >
> > > >
> > > >
> > > >
> > > > Regards,
> > > > Pascal van Leeuwen
> > > > Silicon IP Architect Multi-Protocol Engines, Rambus Security
> > > > Rambus ROTW Holding BV
> > > > +31-73 6581953
> > > >
> > > >
> > > >
> > > > Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired
> > > > by
> Rambus.
> > >
> > > Please be so kind to update your e-mail address book with my new
> > >
> > > > e-mail address.
> > > >
> > > >
> > > >
> > > > ** This message and any attachments are for the sole use of the
> > > > intended
> > > > recipient(s). It may contain information that is confidential and
> > > > privileged. If you are not the intended recipient of this message, you
> > > > are
> > > > prohibited from printing, copying, forwarding or saving it. Please
> > > > delete
> > > > the message and attachments and notify the sender immediately. **
> > >
> > >
> > >
> > > > Rambus Inc.<http://www.rambus.com>
> > >
> > >
> > >
> > >
> > >
> > > Ciao
> > > Stephan
> >
> >
> > Regards,
> > Pascal van Leeuwen
> > Silicon IP Architect Multi-Protocol Engines, Rambus Security
> > Rambus ROTW Holding BV
> > +31-73 6581953
> >
> > Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by
> > Rambus.
> Please be so kind to update your e-mail address book with my new
> > e-mail address.
> >
> > ** This message and any attachments are for the sole use of the intended
> > recipient(s). It may contain information that is confidential and
> > privileged. If you are not the intended recipient of this message, you are
> > prohibited from printing, copying, forwarding or saving it. Please delete
> > the message and attachments and notify the sender immediately. **
>
> > Rambus Inc.<http://www.rambus.com>
>
>
>
> Ciao
> Stephan
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
On Tue, Jan 28, 2020 at 11:12 PM Eric Biggers <[email protected]> wrote:
>
> On Tue, Jan 28, 2020 at 09:24:25AM +0200, Gilad Ben-Yossef wrote:
> > - The source is presumed to have enough room for both the associated
> > data and the plaintext.
> > - Unless it's in-place encryption, in which case, you also presume to
> > have room for the authentication tag
>
> The authentication tag is part of the ciphertext, not the plaintext. So the
> rule is just that the ciphertext buffer needs to have room for it, not the
> plaintext.
>
> Of course, when doing in-place encryption/decryption, the two buffers are the
> same, so both will have room for it, even though the tag is only meaningful on
> the ciphertext side. That's just the logical consequence of "in-place".
>
> > - The only way to tell if this is in-place encryption or not is to
> > compare the pointers to the source and destination - there is no flag.
>
> Requiring users to remember to provide a flag to indicate in-place
> encryption/decryption, in addition to passing the same scatterlist, would make
> the API more complex.
>
> > - You can count on the scattergather list not having a first NULL
> > buffer, *unless* the plaintext and associated data length are both
> > zero AND it's not in place encryption.
> > - You can count on not getting NULL as a scatterlist point, *unless*
> > the plaintext and associated data length are both zero AND it's not in
> > place encryption. (I'm actually unsure of this one?)
>
> If we consider that the input is not just a scatterlist, but rather a
> scatterlist and a length, then these observations are really just "you can
> access the first byte, unless the length is 0" -- which is sort of obvious. And
> requiring a dereferencable pointer for length = 0 is generally considered to be
> bad API design; see the memcpy() fiasco
> (https://www.imperialviolet.org/2016/06/26/nonnull.html).
>
> The API could be simplified by only supporting full scatterlists, but it seems
> that users are currently relying on being able to encrypt/decrypt just a prefix.
>
> IMO, the biggest problems with the AEAD API are actually things you didn't
> mention, such as the fact that the AAD isn't given in a separate scatterlist,
> and that the API only supports scatterlists and not virtual addresses (which
> makes it difficult to use in some cases).
>
> In any case we do need much better documentation. I'm planning to improve some
> of the crypto API documentation, but I'll probably do the hash and skcipher
> algorithm types first before getting to AEAD. So if you want to improve the
> AEAD documentation in the mean time, please go ahead.
Probably another issue with my driver, but just in case -
include/crypot/aead.h says:
* The scatter list pointing to the input data must contain:
*
* * for RFC4106 ciphers, the concatenation of
* associated authentication data || IV || plaintext or ciphertext. Note, the
* same IV (buffer) is also set with the aead_request_set_crypt call. Note,
* the API call of aead_request_set_ad must provide the length of the AAD and
* the IV. The API call of aead_request_set_crypt only points to the size of
* the input plaintext or ciphertext.
I seem to be missing the place where this is handled in
generate_random_aead_testvec()
and generate_aead_message()
We seem to be generating a random IV for providing as the parameter to
aead_request_set_crypt()
but than have other random bytes set in aead_request_set_ad() - or am
I'm missing something again?
My apologies if this is just me suffering from lack of coffee...
Thanks!
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> Probably another issue with my driver, but just in case -
> include/crypot/aead.h says:
>
> * The scatter list pointing to the input data must contain:
> *
> * * for RFC4106 ciphers, the concatenation of
> * associated authentication data || IV || plaintext or ciphertext. Note, the
> * same IV (buffer) is also set with the aead_request_set_crypt call. Note,
> * the API call of aead_request_set_ad must provide the length of the AAD and
> * the IV. The API call of aead_request_set_crypt only points to the size of
> * the input plaintext or ciphertext.
>
> I seem to be missing the place where this is handled in
> generate_random_aead_testvec()
> and generate_aead_message()
>
> We seem to be generating a random IV for providing as the parameter to
> aead_request_set_crypt()
> but than have other random bytes set in aead_request_set_ad() - or am
> I'm missing something again?
Yes, for rfc4106 the tests don't pass the same IV in both places. This is
because I wrote the tests from the perspective of a generic AEAD that doesn't
have this weird IV quirk, and then I added the minimum quirks to get the weird
algorithms like rfc4106 passing.
Since the actual behavior of the generic implementation of rfc4106 is that the
last 8 bytes of the AAD are ignored, that means that currently the tests just
avoid mutating these bytes when generating inauthentic input tests. They don't
know that they're (apparently) meant to be another copy of the IV.
So it seems we need to clearly define the behavior when the two IV copies don't
match. Should one or the other be used, should an error be returned, or should
the behavior be unspecified (in which case the tests would need to be updated)?
Unspecified behavior is bad, but it would be easiest for software to use
req->iv, while hardware might want to use the IV in the scatterlist...
Herbert and Stephan, any idea what was intended here?
- Eric
Am Freitag, 7. Februar 2020, 08:27:09 CET schrieb Eric Biggers:
Hi Eric,
> On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> > Probably another issue with my driver, but just in case -
> >
> > include/crypot/aead.h says:
> > * The scatter list pointing to the input data must contain:
> > *
> > * * for RFC4106 ciphers, the concatenation of
> > * associated authentication data || IV || plaintext or ciphertext.
> > Note, the * same IV (buffer) is also set with the
> > aead_request_set_crypt call. Note, * the API call of
> > aead_request_set_ad must provide the length of the AAD and * the IV.
> > The API call of aead_request_set_crypt only points to the size of *
> > the input plaintext or ciphertext.
> >
> > I seem to be missing the place where this is handled in
> > generate_random_aead_testvec()
> > and generate_aead_message()
> >
> > We seem to be generating a random IV for providing as the parameter to
> > aead_request_set_crypt()
> > but than have other random bytes set in aead_request_set_ad() - or am
> > I'm missing something again?
>
> Yes, for rfc4106 the tests don't pass the same IV in both places. This is
> because I wrote the tests from the perspective of a generic AEAD that
> doesn't have this weird IV quirk, and then I added the minimum quirks to
> get the weird algorithms like rfc4106 passing.
>
> Since the actual behavior of the generic implementation of rfc4106 is that
> the last 8 bytes of the AAD are ignored, that means that currently the
> tests just avoid mutating these bytes when generating inauthentic input
> tests. They don't know that they're (apparently) meant to be another copy
> of the IV.
>
> So it seems we need to clearly define the behavior when the two IV copies
> don't match. Should one or the other be used, should an error be returned,
> or should the behavior be unspecified (in which case the tests would need
> to be updated)?
>
> Unspecified behavior is bad, but it would be easiest for software to use
> req->iv, while hardware might want to use the IV in the scatterlist...
>
> Herbert and Stephan, any idea what was intended here?
>
> - Eric
The full structure of RFC4106 is the following:
- the key to be set is always 4 bytes larger than required for the respective
AES operation (i.e. the key is 20, 28 or 36 bytes respectively). The key value
contains the following information: key || first 4 bytes of the IV (note, the
first 4 bytes of the IV are the bytes derived from the KDF invoked by IKE -
i.e. they come from user space and are fixed)
- data block contains AAD || trailing 8 bytes of IV || plaintext or ciphertext
- the trailing 8 bytes of the IV are the SPI which is updated for each new
IPSec package
aead_request_set_ad points to the AAD plus the 8 bytes of IV in the use case
of rfc4106(gcm(aes)) as part of IPSec.
Considering your question about the aead_request_set_ad vs
aead_request_set_crypt I think the RFC4106 gives the answer: the IV is used in
two locations considering that the IV is also the SPI in our case. If you see
RFC 4106 chapter 3 you see the trailing 8 bytes of the IV as, well, the GCM IV
(which is extended by the 4 byte salt as defined in chapter 4 that we provide
with the trailing 4 bytes of the key). The kernel uses the SPI for this. In
chapter 5 RFC4106 you see that the SP is however used as part of the AAD as
well.
Bottom line: if you do not set the same IV value for both, the AAD and the GCM
IV, you deviate from the use case of rfc4106(gcm(aes)) in IPSec. Yet, from a
pure mathematical point of view and also from a cipher implementation point of
view, it does not matter whether the AAD and the IV point to the same value -
the implementation must always process that data. The result however will not
be identical to the IPSec use case.
Some code to illustrate it - this code is from my CAVS test harness used to
perform the crypto testing for FIPS 140-2:
Preparation of the key:
/*
* RFC4106 special handling: append the first 4 bytes of the IV to
* the key. If IV is NULL, append NULL string (i.e. the fixed field is
* zero in case of internal IV generation). The first 4 bytes of
* the IV must be removed from the IV string.
*/
if (strcasestr(ciphername, "rfc4106")) {
struct buffer rfc;
memset(&rfc, 0, sizeof(struct buffer));
if (alloc_buf(data->key.len + 4, &rfc))
goto out;
/* copy the key into buffer */
memcpy(rfc.buf, data->key.buf, data->key.len);
if (data->iv.len >= 4) {
uint32_t i = 0;
/* Copy first four bytes of the IV into key */
memcpy(rfc.buf + data->key.len, data->iv.buf, 4);
/* move remaining bytes to the front to be used as IV
*/
for (i = 0; i < (data->iv.len - 4); i++)
data->iv.buf[i] = data->iv.buf[(i + 4)];
data->iv.len -= 4;
}
Preparation of the SGL - the IV here is the trailing 8 bytes after the
operation above:
if (aead_assoc->len) {
if (rfc4106) {
sg_init_table(sg, 3);
sg_set_buf(&sg[0], aead_assoc->data, aead_assoc->len);
sg_set_buf(&sg[1], iv->data, iv->len);
sg_set_buf(&sg[2], data->data, data->len +
(kccavs_test->type & TYPE_ENC ? authsize :
0));
} else {
sg_init_table(sg, 2);
sg_set_buf(&sg[0], aead_assoc->data, aead_assoc->len);
sg_set_buf(&sg[1], data->data, data->len +
(kccavs_test->type & TYPE_ENC ? authsize :
0));
}
} else {
if (rfc4106) {
sg_init_table(sg, 2);
sg_set_buf(&sg[0], iv->data, iv->len);
sg_set_buf(&sg[1], data->data, data->len +
(kccavs_test->type & TYPE_ENC ? authsize :
0));
} else {
sg_init_table(sg, 1);
sg_set_buf(&sg[0], data->data, data->len +
(kccavs_test->type & TYPE_ENC ? authsize :
0));
}
}
Informing the kernel crypto API about the AAD size:
if (rfc4106)
aead_request_set_ad(req, aead_assoc->len + iv->len);
else
aead_request_set_ad(req, aead_assoc->len);
Set the buffers:
aead_request_set_crypt(req, sg, sg, data->len, iv->data);
Ciao
Stephan
On Fri, Feb 7, 2020 at 9:56 AM Stephan Mueller <[email protected]> wrote:
>
> Am Freitag, 7. Februar 2020, 08:27:09 CET schrieb Eric Biggers:
>
> Hi Eric,
>
> > On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> > > Probably another issue with my driver, but just in case -
> > >
> > > include/crypot/aead.h says:
> > > * The scatter list pointing to the input data must contain:
> > > *
> > > * * for RFC4106 ciphers, the concatenation of
> > > * associated authentication data || IV || plaintext or ciphertext.
> > > Note, the * same IV (buffer) is also set with the
> > > aead_request_set_crypt call. Note, * the API call of
> > > aead_request_set_ad must provide the length of the AAD and * the IV.
> > > The API call of aead_request_set_crypt only points to the size of *
> > > the input plaintext or ciphertext.
> > >
> > > I seem to be missing the place where this is handled in
> > > generate_random_aead_testvec()
> > > and generate_aead_message()
> > >
> > > We seem to be generating a random IV for providing as the parameter to
> > > aead_request_set_crypt()
> > > but than have other random bytes set in aead_request_set_ad() - or am
> > > I'm missing something again?
> >
> > Yes, for rfc4106 the tests don't pass the same IV in both places. This is
> > because I wrote the tests from the perspective of a generic AEAD that
> > doesn't have this weird IV quirk, and then I added the minimum quirks to
> > get the weird algorithms like rfc4106 passing.
> >
> > Since the actual behavior of the generic implementation of rfc4106 is that
> > the last 8 bytes of the AAD are ignored, that means that currently the
> > tests just avoid mutating these bytes when generating inauthentic input
> > tests. They don't know that they're (apparently) meant to be another copy
> > of the IV.
> >
> > So it seems we need to clearly define the behavior when the two IV copies
> > don't match. Should one or the other be used, should an error be returned,
> > or should the behavior be unspecified (in which case the tests would need
> > to be updated)?
> >
> > Unspecified behavior is bad, but it would be easiest for software to use
> > req->iv, while hardware might want to use the IV in the scatterlist...
> >
> > Herbert and Stephan, any idea what was intended here?
> >
> > - Eric
>
> The full structure of RFC4106 is the following:
>
> - the key to be set is always 4 bytes larger than required for the respective
> AES operation (i.e. the key is 20, 28 or 36 bytes respectively). The key value
> contains the following information: key || first 4 bytes of the IV (note, the
> first 4 bytes of the IV are the bytes derived from the KDF invoked by IKE -
> i.e. they come from user space and are fixed)
>
> - data block contains AAD || trailing 8 bytes of IV || plaintext or ciphertext
> - the trailing 8 bytes of the IV are the SPI which is updated for each new
> IPSec package
>
> aead_request_set_ad points to the AAD plus the 8 bytes of IV in the use case
> of rfc4106(gcm(aes)) as part of IPSec.
>
> Considering your question about the aead_request_set_ad vs
> aead_request_set_crypt I think the RFC4106 gives the answer: the IV is used in
> two locations considering that the IV is also the SPI in our case. If you see
> RFC 4106 chapter 3 you see the trailing 8 bytes of the IV as, well, the GCM IV
> (which is extended by the 4 byte salt as defined in chapter 4 that we provide
> with the trailing 4 bytes of the key). The kernel uses the SPI for this. In
> chapter 5 RFC4106 you see that the SP is however used as part of the AAD as
> well.
>
> Bottom line: if you do not set the same IV value for both, the AAD and the GCM
> IV, you deviate from the use case of rfc4106(gcm(aes)) in IPSec. Yet, from a
> pure mathematical point of view and also from a cipher implementation point of
> view, it does not matter whether the AAD and the IV point to the same value -
> the implementation must always process that data. The result however will not
> be identical to the IPSec use case.
>
It is correct, but is it smart?
Either we require the same IV to be passed twice as we do today, in which case
passing different IV should fail in a predictable manner OR we should define
the operation is taking two IV like structures - one as the IV and one as
bytes in the associated data and have the IPsec code use it in a specific way of
happen to pass the same IV in both places.
I don't care either way - but right now the tests basically relies on
undefined behaviour
which is always a bad thing, I think.
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
Am Freitag, 7. Februar 2020, 12:50:51 CET schrieb Gilad Ben-Yossef:
Hi Gilad,
>
> It is correct, but is it smart?
>
> Either we require the same IV to be passed twice as we do today, in which
> case passing different IV should fail in a predictable manner OR we should
> define the operation is taking two IV like structures - one as the IV and
> one as bytes in the associated data and have the IPsec code use it in a
> specific way of happen to pass the same IV in both places.
>
> I don't care either way - but right now the tests basically relies on
> undefined behaviour
> which is always a bad thing, I think.
I am not sure about the motivation of this discussion: we have exactly one
user of the RFC4106 implementation: IPSec. Providing the IV/AAD is efficient
as the rfc4106 template intents to require the data in a format that requires
minimal processing on the IPSec side to bring it in the right format.
On the other hand, the cipher implementation should just do the operation
regardless of where the data comes from or whether the AAD buffer overlaps
with the IV buffer. I.e. the cipher should try to interpret the data but just
do the work.
So, where is it inefficient? Maybe the API for RFC4106 could be a bit nicer,
but it needs to fit into the overall AEAD API as a specific RFC4106-API seems
to be overkill.
Ciao
Stephan
Hi Stephan,
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Stephan Mueller
> Sent: Friday, February 7, 2020 8:56 AM
> To: Eric Biggers <[email protected]>
> Cc: Gilad Ben-Yossef <[email protected]>; Herbert Xu <[email protected]>; Linux Crypto Mailing List <linux-
> [email protected]>; Geert Uytterhoeven <[email protected]>; David Miller <[email protected]>; Ofir Drang
> <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> Am Freitag, 7. Februar 2020, 08:27:09 CET schrieb Eric Biggers:
>
> Hi Eric,
>
> > On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> > > Probably another issue with my driver, but just in case -
> > >
> > > include/crypot/aead.h says:
> > > * The scatter list pointing to the input data must contain:
> > > *
> > > * * for RFC4106 ciphers, the concatenation of
> > > * associated authentication data || IV || plaintext or ciphertext.
> > > Note, the * same IV (buffer) is also set with the
> > > aead_request_set_crypt call. Note, * the API call of
> > > aead_request_set_ad must provide the length of the AAD and * the IV.
> > > The API call of aead_request_set_crypt only points to the size of *
> > > the input plaintext or ciphertext.
> > >
> > > I seem to be missing the place where this is handled in
> > > generate_random_aead_testvec()
> > > and generate_aead_message()
> > >
> > > We seem to be generating a random IV for providing as the parameter to
> > > aead_request_set_crypt()
> > > but than have other random bytes set in aead_request_set_ad() - or am
> > > I'm missing something again?
> >
> > Yes, for rfc4106 the tests don't pass the same IV in both places. This is
> > because I wrote the tests from the perspective of a generic AEAD that
> > doesn't have this weird IV quirk, and then I added the minimum quirks to
> > get the weird algorithms like rfc4106 passing.
> >
> > Since the actual behavior of the generic implementation of rfc4106 is that
> > the last 8 bytes of the AAD are ignored, that means that currently the
> > tests just avoid mutating these bytes when generating inauthentic input
> > tests. They don't know that they're (apparently) meant to be another copy
> > of the IV.
> >
> > So it seems we need to clearly define the behavior when the two IV copies
> > don't match. Should one or the other be used, should an error be returned,
> > or should the behavior be unspecified (in which case the tests would need
> > to be updated)?
> >
> > Unspecified behavior is bad, but it would be easiest for software to use
> > req->iv, while hardware might want to use the IV in the scatterlist...
> >
> > Herbert and Stephan, any idea what was intended here?
> >
> > - Eric
>
> The full structure of RFC4106 is the following:
>
> - the key to be set is always 4 bytes larger than required for the respective
> AES operation (i.e. the key is 20, 28 or 36 bytes respectively). The key value
> contains the following information: key || first 4 bytes of the IV (note, the
> first 4 bytes of the IV are the bytes derived from the KDF invoked by IKE -
> i.e. they come from user space and are fixed)
>
> - data block contains AAD || trailing 8 bytes of IV || plaintext or ciphertext
> - the trailing 8 bytes of the IV are the SPI which is updated for each new
> IPSec package
>
By SPI you must mean sequence number?
(The SPI is actually the SA index which certainly doesn't change per packet!)
That would be one possible way of generating the explicit IV, but you certainly
cannot count on that. Anything unique under the key would be fine for GCM.
> aead_request_set_ad points to the AAD plus the 8 bytes of IV in the use case
> of rfc4106(gcm(aes)) as part of IPSec.
>
> Considering your question about the aead_request_set_ad vs
> aead_request_set_crypt I think the RFC4106 gives the answer: the IV is used in
> two locations considering that the IV is also the SPI in our case. If you see
> RFC 4106 chapter 3 you see the trailing 8 bytes of the IV as, well, the GCM IV
> (which is extended by the 4 byte salt as defined in chapter 4 that we provide
> with the trailing 4 bytes of the key). The kernel uses the SPI for this.
>
Again, by SPI you must mean sequence number. The SPI itself is entirely seperate.
So the IV is not "used in two places", it is only used as IV for the AEAD operation,
with the explicit part (8 bytes) inserted into the packet.
[For GCM the IV, despite being in the AAD buffer, is _not_ authenticated]
The sequence number _may_ be used in two places (AAD and explicit part of the IV),
but that is not a given and out of the scope of the crypto API. I would not make
any assumptions there.
The "problem" Gilad was referring to is that the _explicit_ part of the IV appears to be
available from both req->iv and from the AAD scatterbuffer. Which one should you use?
API wise I would assume req->iv but from a (our) hardware perspective, it would
be more efficient to extract it from the datastream. But is it allowed to assume
there is a valid IV stored there? (which implies that it has to match req->iv,
otherwise behaviour would deviate from implementations using that)
> In chapter 5 RFC4106 you see that the SP is however used as part of the AAD as
> well.
>
> Bottom line: if you do not set the same IV value for both, the AAD and the GCM
> IV, you deviate from the use case of rfc4106(gcm(aes)) in IPSec. Yet, from a
> pure mathematical point of view and also from a cipher implementation point of
> view, it does not matter whether the AAD and the IV point to the same value -
> the implementation must always process that data. The result however will not
> be identical to the IPSec use case.
>
For the IPsec use case, it's perfectly legal to have IV != sequence number as long
as it is unique under the key.
So you should not assume the sequence number part of the AAD buffer to match
the IV part (or req->iv), but it _would_ make sense if the IV part of the AAD matches
req->iv. (then again, if this is not _required_ by the API the application might not
bother providing it, which is my reason not to use in in the inside_secure driver)
> Some code to illustrate it - this code is from my CAVS test harness used to
> perform the crypto testing for FIPS 140-2:
>
>
> Preparation of the key:
>
> /*
> * RFC4106 special handling: append the first 4 bytes of the IV to
> * the key. If IV is NULL, append NULL string (i.e. the fixed field is
> * zero in case of internal IV generation). The first 4 bytes of
> * the IV must be removed from the IV string.
> */
> if (strcasestr(ciphername, "rfc4106")) {
> struct buffer rfc;
>
> memset(&rfc, 0, sizeof(struct buffer));
> if (alloc_buf(data->key.len + 4, &rfc))
> goto out;
>
> /* copy the key into buffer */
> memcpy(rfc.buf, data->key.buf, data->key.len);
> if (data->iv.len >= 4) {
> uint32_t i = 0;
>
> /* Copy first four bytes of the IV into key */
> memcpy(rfc.buf + data->key.len, data->iv.buf, 4);
>
> /* move remaining bytes to the front to be used as IV
> */
> for (i = 0; i < (data->iv.len - 4); i++)
> data->iv.buf[i] = data->iv.buf[(i + 4)];
> data->iv.len -= 4;
> }
>
>
> Preparation of the SGL - the IV here is the trailing 8 bytes after the
> operation above:
>
> if (aead_assoc->len) {
> if (rfc4106) {
> sg_init_table(sg, 3);
> sg_set_buf(&sg[0], aead_assoc->data, aead_assoc->len);
> sg_set_buf(&sg[1], iv->data, iv->len);
> sg_set_buf(&sg[2], data->data, data->len +
> (kccavs_test->type & TYPE_ENC ? authsize :
> 0));
> } else {
> sg_init_table(sg, 2);
> sg_set_buf(&sg[0], aead_assoc->data, aead_assoc->len);
> sg_set_buf(&sg[1], data->data, data->len +
> (kccavs_test->type & TYPE_ENC ? authsize :
> 0));
> }
> } else {
> if (rfc4106) {
> sg_init_table(sg, 2);
> sg_set_buf(&sg[0], iv->data, iv->len);
> sg_set_buf(&sg[1], data->data, data->len +
> (kccavs_test->type & TYPE_ENC ? authsize :
> 0));
> } else {
> sg_init_table(sg, 1);
> sg_set_buf(&sg[0], data->data, data->len +
> (kccavs_test->type & TYPE_ENC ? authsize :
> 0));
> }
> }
>
>
> Informing the kernel crypto API about the AAD size:
>
> if (rfc4106)
> aead_request_set_ad(req, aead_assoc->len + iv->len);
> else
> aead_request_set_ad(req, aead_assoc->len);
>
>
> Set the buffers:
>
> aead_request_set_crypt(req, sg, sg, data->len, iv->data);
>
> Ciao
> Stephan
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
Am Freitag, 7. Februar 2020, 15:07:49 CET schrieb Van Leeuwen, Pascal:
Hi Pascal,
> Hi Stephan,
>
>
> > -----Original Message-----
> > From: [email protected]
> > <[email protected]> On Behalf Of Stephan Mueller
Sent:
> > Friday, February 7, 2020 8:56 AM
> > To: Eric Biggers <[email protected]>
> > Cc: Gilad Ben-Yossef <[email protected]>; Herbert Xu
> > <[email protected]>; Linux Crypto Mailing List <linux-
> > [email protected]>; Geert Uytterhoeven <[email protected]>; David
> > Miller <[email protected]>; Ofir Drang <[email protected]>
> > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto
> > tests
>
> >
> >
> > <<< External Email >>>
> > CAUTION: This email originated from outside of the organization. Do not
> > click links or open attachments unless you recognize the
sender/sender
> > address and know the content is safe.
> >
> >
> >
> >
> > Am Freitag, 7. Februar 2020, 08:27:09 CET schrieb Eric Biggers:
> >
> >
> >
> > Hi Eric,
> >
> >
> >
> > > On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> > >
> > > > Probably another issue with my driver, but just in case -
> > > >
> > > >
> > > >
> > > > include/crypot/aead.h says:
> > > >
> > > > * The scatter list pointing to the input data must contain:
> > > > *
> > > > * * for RFC4106 ciphers, the concatenation of
> > > > * associated authentication data || IV || plaintext or ciphertext.
> > > > Note, the * same IV (buffer) is also set with the
> > > > aead_request_set_crypt call. Note, * the API call of
> > > > aead_request_set_ad must provide the length of the AAD and * the
> > > > IV.
> > > > The API call of aead_request_set_crypt only points to the size of *
> > > > the input plaintext or ciphertext.
> > > >
> > > >
> > > >
> > > > I seem to be missing the place where this is handled in
> > > > generate_random_aead_testvec()
> > > > and generate_aead_message()
> > > >
> > > >
> > > >
> > > > We seem to be generating a random IV for providing as the parameter
> > > > to
> > > > aead_request_set_crypt()
> > > > but than have other random bytes set in aead_request_set_ad() - or am
> > > > I'm missing something again?
> > >
> > >
> > >
> > > Yes, for rfc4106 the tests don't pass the same IV in both places. This
> > > is
> > > because I wrote the tests from the perspective of a generic AEAD that
> > > doesn't have this weird IV quirk, and then I added the minimum quirks
> > > to
> > > get the weird algorithms like rfc4106 passing.
> > >
> > >
> > >
> > > Since the actual behavior of the generic implementation of rfc4106 is
> > > that
> > > the last 8 bytes of the AAD are ignored, that means that currently the
> > > tests just avoid mutating these bytes when generating inauthentic input
> > > tests. They don't know that they're (apparently) meant to be another
> > > copy
> > > of the IV.
> > >
> > >
> > >
> > > So it seems we need to clearly define the behavior when the two IV
> > > copies
> > > don't match. Should one or the other be used, should an error be
> > > returned,
or should the behavior be unspecified (in which case the
> > > tests would need to be updated)?
> > >
> > >
> > >
> > > Unspecified behavior is bad, but it would be easiest for software to
> > > use
> > > req->iv, while hardware might want to use the IV in the scatterlist...
> > >
> > >
> > >
> > > Herbert and Stephan, any idea what was intended here?
> > >
> > >
> > >
> > > - Eric
> >
> >
> >
> > The full structure of RFC4106 is the following:
> >
> >
> >
> > - the key to be set is always 4 bytes larger than required for the
> > respective
AES operation (i.e. the key is 20, 28 or 36 bytes
> > respectively). The key value contains the following information: key ||
> > first 4 bytes of the IV (note, the first 4 bytes of the IV are the bytes
> > derived from the KDF invoked by IKE - i.e. they come from user space and
> > are fixed)
> >
> >
> >
> > - data block contains AAD || trailing 8 bytes of IV || plaintext or
> > ciphertext
- the trailing 8 bytes of the IV are the SPI which is updated
> > for each new IPSec package
> >
> >
>
> By SPI you must mean sequence number?
> (The SPI is actually the SA index which certainly doesn't change per
> packet!)
> That would be one possible way of generating the explicit IV, but
> you certainly cannot count on that. Anything unique under the key would be
> fine for GCM.
The IV actually is generated with an IV generator (I think it is the SEQIV
generator from crypto/seqiv.c - it is set in the XFRM framework). It is a
deterministic construction XORed with a random number from the SP800-90A DRBG.
>
> > aead_request_set_ad points to the AAD plus the 8 bytes of IV in the use
> > case
of rfc4106(gcm(aes)) as part of IPSec.
> >
> >
> >
> > Considering your question about the aead_request_set_ad vs
> > aead_request_set_crypt I think the RFC4106 gives the answer: the IV is
> > used in
two locations considering that the IV is also the SPI in our
> > case. If you see RFC 4106 chapter 3 you see the trailing 8 bytes of the
> > IV as, well, the GCM IV (which is extended by the 4 byte salt as defined
> > in chapter 4 that we provide with the trailing 4 bytes of the key). The
> > kernel uses the SPI for this.>
> >
>
> Again, by SPI you must mean sequence number. The SPI itself is entirely
> seperate.
See above, it is actually not the SPI, or sequence number, it is what the IV
generator provides.
> So the IV is not "used in two places", it is only used as IV for
> the AEAD operation, with the explicit part (8 bytes) inserted into the
> packet.
> [For GCM the IV, despite being in the AAD buffer, is _not_ authenticated]
> The sequence number _may_ be used in two places (AAD and explicit part of
> the IV),
> but that is not a given and out of the scope of the crypto API. I
> would not make any assumptions there.
>
> The "problem" Gilad was referring to is that the _explicit_ part of the IV
> appears to be
> available from both req->iv and from the AAD scatterbuffer.
> Which one should you use? API wise I would assume req->iv but from a (our)
> hardware perspective, it would be more efficient to extract it from the
> datastream. But is it allowed to assume there is a valid IV stored there?
> (which implies that it has to match req->iv, otherwise behaviour would
> deviate from implementations using that)
req->iv is your IV.
The use of the IV as part of the AAD is just a use case for rfc4106. Although
I doubt that the rfc4106 structure will change any time soon, I would not use
the IV from the AAD but only look at the req->iv.
>
> > In chapter 5 RFC4106 you see that the SP is however used as part of the
> > AAD as
well.
> >
> >
> >
> > Bottom line: if you do not set the same IV value for both, the AAD and the
> > GCM
IV, you deviate from the use case of rfc4106(gcm(aes)) in IPSec.
> > Yet, from a pure mathematical point of view and also from a cipher
> > implementation point of view, it does not matter whether the AAD and the
> > IV point to the same value - the implementation must always process that
> > data. The result however will not be identical to the IPSec use case.
> >
> >
>
> For the IPsec use case, it's perfectly legal to have IV != sequence number
> as long
> as it is unique under the key.
Right, it is a perfectly legal way of doing it, but it is currently not done
that way in the kernel. Thus, I would reiterate my suggestion from above to
always use req->iv as your IV.
> So you should not assume the sequence number part of the AAD buffer to
> match
> the IV part (or req->iv), but it _would_ make sense if the IV part
> of the AAD matches req->iv. (then again, if this is not _required_ by the
> API the application might not bother providing it, which is my reason not
> to use in in the inside_secure driver)
Precisely.
Ciao
Stephan
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Gilad Ben-Yossef
> Sent: Friday, February 7, 2020 12:51 PM
> To: Stephan Mueller <[email protected]>
> Cc: Eric Biggers <[email protected]>; Herbert Xu <[email protected]>; Linux Crypto Mailing List <linux-
> [email protected]>; Geert Uytterhoeven <[email protected]>; David Miller <[email protected]>; Ofir Drang
> <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> On Fri, Feb 7, 2020 at 9:56 AM Stephan Mueller <[email protected]> wrote:
> >
> > Am Freitag, 7. Februar 2020, 08:27:09 CET schrieb Eric Biggers:
> >
> > Hi Eric,
> >
> > > On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> > > > Probably another issue with my driver, but just in case -
> > > >
> > > > include/crypot/aead.h says:
> > > > * The scatter list pointing to the input data must contain:
> > > > *
> > > > * * for RFC4106 ciphers, the concatenation of
> > > > * associated authentication data || IV || plaintext or ciphertext.
> > > > Note, the * same IV (buffer) is also set with the
> > > > aead_request_set_crypt call. Note, * the API call of
> > > > aead_request_set_ad must provide the length of the AAD and * the IV.
> > > > The API call of aead_request_set_crypt only points to the size of *
> > > > the input plaintext or ciphertext.
> > > >
> > > > I seem to be missing the place where this is handled in
> > > > generate_random_aead_testvec()
> > > > and generate_aead_message()
> > > >
> > > > We seem to be generating a random IV for providing as the parameter to
> > > > aead_request_set_crypt()
> > > > but than have other random bytes set in aead_request_set_ad() - or am
> > > > I'm missing something again?
> > >
> > > Yes, for rfc4106 the tests don't pass the same IV in both places. This is
> > > because I wrote the tests from the perspective of a generic AEAD that
> > > doesn't have this weird IV quirk, and then I added the minimum quirks to
> > > get the weird algorithms like rfc4106 passing.
> > >
> > > Since the actual behavior of the generic implementation of rfc4106 is that
> > > the last 8 bytes of the AAD are ignored, that means that currently the
> > > tests just avoid mutating these bytes when generating inauthentic input
> > > tests. They don't know that they're (apparently) meant to be another copy
> > > of the IV.
> > >
> > > So it seems we need to clearly define the behavior when the two IV copies
> > > don't match. Should one or the other be used, should an error be returned,
> > > or should the behavior be unspecified (in which case the tests would need
> > > to be updated)?
> > >
> > > Unspecified behavior is bad, but it would be easiest for software to use
> > > req->iv, while hardware might want to use the IV in the scatterlist...
> > >
> > > Herbert and Stephan, any idea what was intended here?
> > >
> > > - Eric
> >
> > The full structure of RFC4106 is the following:
> >
> > - the key to be set is always 4 bytes larger than required for the respective
> > AES operation (i.e. the key is 20, 28 or 36 bytes respectively). The key value
> > contains the following information: key || first 4 bytes of the IV (note, the
> > first 4 bytes of the IV are the bytes derived from the KDF invoked by IKE -
> > i.e. they come from user space and are fixed)
> >
> > - data block contains AAD || trailing 8 bytes of IV || plaintext or ciphertext
> > - the trailing 8 bytes of the IV are the SPI which is updated for each new
> > IPSec package
> >
> > aead_request_set_ad points to the AAD plus the 8 bytes of IV in the use case
> > of rfc4106(gcm(aes)) as part of IPSec.
> >
> > Considering your question about the aead_request_set_ad vs
> > aead_request_set_crypt I think the RFC4106 gives the answer: the IV is used in
> > two locations considering that the IV is also the SPI in our case. If you see
> > RFC 4106 chapter 3 you see the trailing 8 bytes of the IV as, well, the GCM IV
> > (which is extended by the 4 byte salt as defined in chapter 4 that we provide
> > with the trailing 4 bytes of the key). The kernel uses the SPI for this. In
> > chapter 5 RFC4106 you see that the SP is however used as part of the AAD as
> > well.
> >
> > Bottom line: if you do not set the same IV value for both, the AAD and the GCM
> > IV, you deviate from the use case of rfc4106(gcm(aes)) in IPSec. Yet, from a
> > pure mathematical point of view and also from a cipher implementation point of
> > view, it does not matter whether the AAD and the IV point to the same value -
> > the implementation must always process that data. The result however will not
> > be identical to the IPSec use case.
> >
>
> It is correct, but is it smart?
>
> Either we require the same IV to be passed twice as we do today, in which case
> passing different IV should fail in a predictable manner
>
I hope you are not suggesting comparing two on the fly ...
For GCM, it is just a matter of either clearly defining where to take the IV (either
req->iv _or_ the AAD buffer) _or_ _requiring_ them to be always identical
(Pushing that responsibility to the application. And I would expect the kernel
IPsec spec to just make req->iv point to the IV in that AAD scatter buffer which
would mean they are indeed always identical. But that just a guess.)
If that requirement is not met, I would expect an authentication fail, either
on the local side for decryption or on the remote side for encryption.
I just realised that for the similar rfc4543, the IV _is_ authenticated so there
the IV in the AAD _must_ either match req->iv or it should be used instead of
req->iv. In any case, if that requirement is not met you should get similar fails to
what I mentioned for GCM above.
> OR we should define
> the operation is taking two IV like structures - one as the IV and one as
> bytes in the associated data and have the IPsec code use it in a specific way of
> happen to pass the same IV in both places.
>
> I don't care either way - but right now the tests basically relies on
> undefined behaviour
> which is always a bad thing, I think.
>
I think the current implementation is primarily based on what would be
convenient for the only user - the kernel IPsec stack ...
> Gilad
>
> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
>
> values of β will give rise to dom!
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
> -----Original Message-----
> From: Stephan Mueller <[email protected]>
> Sent: Friday, February 7, 2020 3:29 PM
> To: Van Leeuwen, Pascal <[email protected]>
> Cc: Eric Biggers <[email protected]>; Gilad Ben-Yossef <[email protected]>; Herbert Xu <[email protected]>;
> Linux Crypto Mailing List <[email protected]>; Geert Uytterhoeven <[email protected]>; David Miller
> <[email protected]>; Ofir Drang <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> Am Freitag, 7. Februar 2020, 15:07:49 CET schrieb Van Leeuwen, Pascal:
>
> Hi Pascal,
>
> > Hi Stephan,
> >
> >
> > > -----Original Message-----
> > > From: [email protected]
> > > <[email protected]> On Behalf Of Stephan Mueller
> Sent:
> > > Friday, February 7, 2020 8:56 AM
> > > To: Eric Biggers <[email protected]>
> > > Cc: Gilad Ben-Yossef <[email protected]>; Herbert Xu
> > > <[email protected]>; Linux Crypto Mailing List <linux-
> > > [email protected]>; Geert Uytterhoeven <[email protected]>; David
> > > Miller <[email protected]>; Ofir Drang <[email protected]>
> > > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto
> > > tests
> >
> > >
> > >
> > > <<< External Email >>>
> > > CAUTION: This email originated from outside of the organization. Do not
> > > click links or open attachments unless you recognize the
> sender/sender
> > > address and know the content is safe.
> > >
> > >
> > >
> > >
> > > Am Freitag, 7. Februar 2020, 08:27:09 CET schrieb Eric Biggers:
> > >
> > >
> > >
> > > Hi Eric,
> > >
> > >
> > >
> > > > On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> > > >
> > > > > Probably another issue with my driver, but just in case -
> > > > >
> > > > >
> > > > >
> > > > > include/crypot/aead.h says:
> > > > >
> > > > > * The scatter list pointing to the input data must contain:
> > > > > *
> > > > > * * for RFC4106 ciphers, the concatenation of
> > > > > * associated authentication data || IV || plaintext or ciphertext.
> > > > > Note, the * same IV (buffer) is also set with the
> > > > > aead_request_set_crypt call. Note, * the API call of
> > > > > aead_request_set_ad must provide the length of the AAD and * the
> > > > > IV.
> > > > > The API call of aead_request_set_crypt only points to the size of *
> > > > > the input plaintext or ciphertext.
> > > > >
> > > > >
> > > > >
> > > > > I seem to be missing the place where this is handled in
> > > > > generate_random_aead_testvec()
> > > > > and generate_aead_message()
> > > > >
> > > > >
> > > > >
> > > > > We seem to be generating a random IV for providing as the parameter
> > > > > to
> > > > > aead_request_set_crypt()
> > > > > but than have other random bytes set in aead_request_set_ad() - or am
> > > > > I'm missing something again?
> > > >
> > > >
> > > >
> > > > Yes, for rfc4106 the tests don't pass the same IV in both places. This
> > > > is
> > > > because I wrote the tests from the perspective of a generic AEAD that
> > > > doesn't have this weird IV quirk, and then I added the minimum quirks
> > > > to
> > > > get the weird algorithms like rfc4106 passing.
> > > >
> > > >
> > > >
> > > > Since the actual behavior of the generic implementation of rfc4106 is
> > > > that
> > > > the last 8 bytes of the AAD are ignored, that means that currently the
> > > > tests just avoid mutating these bytes when generating inauthentic input
> > > > tests. They don't know that they're (apparently) meant to be another
> > > > copy
> > > > of the IV.
> > > >
> > > >
> > > >
> > > > So it seems we need to clearly define the behavior when the two IV
> > > > copies
> > > > don't match. Should one or the other be used, should an error be
> > > > returned,
> or should the behavior be unspecified (in which case the
> > > > tests would need to be updated)?
> > > >
> > > >
> > > >
> > > > Unspecified behavior is bad, but it would be easiest for software to
> > > > use
> > > > req->iv, while hardware might want to use the IV in the scatterlist...
> > > >
> > > >
> > > >
> > > > Herbert and Stephan, any idea what was intended here?
> > > >
> > > >
> > > >
> > > > - Eric
> > >
> > >
> > >
> > > The full structure of RFC4106 is the following:
> > >
> > >
> > >
> > > - the key to be set is always 4 bytes larger than required for the
> > > respective
> AES operation (i.e. the key is 20, 28 or 36 bytes
> > > respectively). The key value contains the following information: key ||
> > > first 4 bytes of the IV (note, the first 4 bytes of the IV are the bytes
> > > derived from the KDF invoked by IKE - i.e. they come from user space and
> > > are fixed)
> > >
> > >
> > >
> > > - data block contains AAD || trailing 8 bytes of IV || plaintext or
> > > ciphertext
> - the trailing 8 bytes of the IV are the SPI which is updated
> > > for each new IPSec package
> > >
> > >
> >
> > By SPI you must mean sequence number?
> > (The SPI is actually the SA index which certainly doesn't change per
> > packet!)
> > That would be one possible way of generating the explicit IV, but
> > you certainly cannot count on that. Anything unique under the key would be
> > fine for GCM.
>
> The IV actually is generated with an IV generator (I think it is the SEQIV
> generator from crypto/seqiv.c - it is set in the XFRM framework). It is a
> deterministic construction XORed with a random number from the SP800-90A DRBG.
>
That would be a good way of generating IV's for CBC mode (which requires
unpredictability and sufficient Hamming distance precluding a counter), but I would not
recommend that for CTR based modes like GCM, where all you need is a nonce because:
a) randomness does not guarantee uniqueness perse
b) it is far too heavy on the CPU for this purpose
So I would certainly hope it doesn't do it like that? The name seqiv alone would imply
something based on a sequence numberand not a DRBG ... IIRC it was doing sequence
number XOR some key material?
> >
> > > aead_request_set_ad points to the AAD plus the 8 bytes of IV in the use
> > > case
> of rfc4106(gcm(aes)) as part of IPSec.
> > >
> > >
> > >
> > > Considering your question about the aead_request_set_ad vs
> > > aead_request_set_crypt I think the RFC4106 gives the answer: the IV is
> > > used in
> two locations considering that the IV is also the SPI in our
> > > case. If you see RFC 4106 chapter 3 you see the trailing 8 bytes of the
> > > IV as, well, the GCM IV (which is extended by the 4 byte salt as defined
> > > in chapter 4 that we provide with the trailing 4 bytes of the key). The
> > > kernel uses the SPI for this.>
> > >
> >
> > Again, by SPI you must mean sequence number. The SPI itself is entirely
> > seperate.
>
> See above, it is actually not the SPI, or sequence number, it is what the IV
> generator provides.
>
Yes. But what you were describing sounded like the sequence number.
Which would be perfectly legal to use _directly_ for this (unlike the SPI).
Thats what our hardware does in case of full protocol offload.
> > So the IV is not "used in two places", it is only used as IV for
> > the AEAD operation, with the explicit part (8 bytes) inserted into the
> > packet.
> > [For GCM the IV, despite being in the AAD buffer, is _not_ authenticated]
> > The sequence number _may_ be used in two places (AAD and explicit part of
> > the IV),
> > but that is not a given and out of the scope of the crypto API. I
> > would not make any assumptions there.
> >
> > The "problem" Gilad was referring to is that the _explicit_ part of the IV
> > appears to be
> > available from both req->iv and from the AAD scatterbuffer.
> > Which one should you use? API wise I would assume req->iv but from a (our)
> > hardware perspective, it would be more efficient to extract it from the
> > datastream. But is it allowed to assume there is a valid IV stored there?
> > (which implies that it has to match req->iv, otherwise behaviour would
> > deviate from implementations using that)
>
> req->iv is your IV.
>
But the IV is also in the last bytes of the AAD buffer. Which would be _way_ more
convenient to use _directly_ compared to req->iv.
Saves a lot of effort in both the driver and the HW to get the IV to where it
is actually needed. For _our_ driver and hardware, anyway.
So that's the point: if it's already where you want it to be, then why insisting
on getting it from a different location (i.e. req->iv) just for the sake of entertaining
some generic API? These rfcxxxx ciphersuites appear to be for a very specific use
case (IPsec) and are already deviating from the normal AEAD implementations.
> The use of the IV as part of the AAD is just a use case for rfc4106.
>
No, it is most definitely not. The IV is _not_ part of the AAD for rfc4106.
Just take another long look at chapter 5 and tell me where it says "IV".
It's just SPI and (full extended) sequence number, no more.
So actually, the implementation needs to be aware of this and stop
authenticating IV size bytes before the end of the AAD buffer. Which is
rather strange if you think about it ... but I guess it is what is is now.
> Although
> I doubt that the rfc4106 structure will change any time soon, I would not use
> the IV from the AAD but only look at the req->iv.
>
That is what I did mostly because I didn't know if I could rely on the IV in the
AAD buffer being correct.
BUT if you're not allowed to use it from the AAD buffer, then why is it even there?
Let's put it differently: if I _would_ take it from the AAD buffer instead of req->iv,
the current test vectors from testmgr.h would pass just fine(!)
And how about GMAC (rfc4543), where IV _does_ need to be authenticated. If
it doesn't match req->iv there, it would certainly not result in output complying
with rfc4543. (though you could argue this to be useful for _other_ purposes)
I think, especially considering the only user being the kernel IPsec stack here,
it would make some sense to _require_ req->iv to match the AAD buffer IV and
allow taking the IV from there instead of from req->iv, if that is more convenient.
> >
> > > In chapter 5 RFC4106 you see that the SP is however used as part of the
> > > AAD as
> well.
> > >
> > >
> > >
> > > Bottom line: if you do not set the same IV value for both, the AAD and the
> > > GCM
> IV, you deviate from the use case of rfc4106(gcm(aes)) in IPSec.
> > > Yet, from a pure mathematical point of view and also from a cipher
> > > implementation point of view, it does not matter whether the AAD and the
> > > IV point to the same value - the implementation must always process that
> > > data. The result however will not be identical to the IPSec use case.
> > >
> > >
> >
> > For the IPsec use case, it's perfectly legal to have IV != sequence number
> > as long
> > as it is unique under the key.
>
> Right, it is a perfectly legal way of doing it, but it is currently not done
> that way in the kernel.
>
I guess my main point was there is no "IV for AAD" (if not the sequence number)
so it can't possibly mismatch the "IV for GCM", ergo it's not possible to deviate from
any IPsec use case. (for GCM anyway, for GMAC you could)
> Thus, I would reiterate my suggestion from above to always use req->iv as your IV.
>
Which is what I do, BUT is rather silly _if_ req->iv in practice will always point to
the IV stored in the AAD scatter buffer.
> > So you should not assume the sequence number part of the AAD buffer to
> > match
> > the IV part (or req->iv), but it _would_ make sense if the IV part
> > of the AAD matches req->iv. (then again, if this is not _required_ by the
> > API the application might not bother providing it, which is my reason not
> > to use in in the inside_secure driver)
>
> Precisely.
>
> Ciao
> Stephan
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of RBRpvanleeuwen
> Sent: Friday, February 7, 2020 4:37 PM
> To: Stephan Mueller <[email protected]>
> Cc: Eric Biggers <[email protected]>; Gilad Ben-Yossef <[email protected]>; Herbert Xu <[email protected]>;
> Linux Crypto Mailing List <[email protected]>; Geert Uytterhoeven <[email protected]>; David Miller
> <[email protected]>; Ofir Drang <[email protected]>
> Subject: RE: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> > -----Original Message-----
> > From: Stephan Mueller <[email protected]>
> > Sent: Friday, February 7, 2020 3:29 PM
> > To: Van Leeuwen, Pascal <[email protected]>
> > Cc: Eric Biggers <[email protected]>; Gilad Ben-Yossef <[email protected]>; Herbert Xu <[email protected]>;
> > Linux Crypto Mailing List <[email protected]>; Geert Uytterhoeven <[email protected]>; David Miller
> > <[email protected]>; Ofir Drang <[email protected]>
> > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
> >
> > <<< External Email >>>
> > Am Freitag, 7. Februar 2020, 15:07:49 CET schrieb Van Leeuwen, Pascal:
> >
> > Hi Pascal,
> >
> > > Hi Stephan,
> > >
> > >
> > > > -----Original Message-----
> > > > From: [email protected]
> > > > <[email protected]> On Behalf Of Stephan Mueller
> > Sent:
> > > > Friday, February 7, 2020 8:56 AM
> > > > To: Eric Biggers <[email protected]>
> > > > Cc: Gilad Ben-Yossef <[email protected]>; Herbert Xu
> > > > <[email protected]>; Linux Crypto Mailing List <linux-
> > > > [email protected]>; Geert Uytterhoeven <[email protected]>; David
> > > > Miller <[email protected]>; Ofir Drang <[email protected]>
> > > > Subject: Re: Possible issue with new inauthentic AEAD in extended crypto
> > > > tests
> > >
> > > >
> > > >
> > > > <<< External Email >>>
> > > > CAUTION: This email originated from outside of the organization. Do not
> > > > click links or open attachments unless you recognize the
> > sender/sender
> > > > address and know the content is safe.
> > > >
> > > >
> > > >
> > > >
> > > > Am Freitag, 7. Februar 2020, 08:27:09 CET schrieb Eric Biggers:
> > > >
> > > >
> > > >
> > > > Hi Eric,
> > > >
> > > >
> > > >
> > > > > On Wed, Feb 05, 2020 at 04:48:16PM +0200, Gilad Ben-Yossef wrote:
> > > > >
> > > > > > Probably another issue with my driver, but just in case -
> > > > > >
> > > > > >
> > > > > >
> > > > > > include/crypot/aead.h says:
> > > > > >
> > > > > > * The scatter list pointing to the input data must contain:
> > > > > > *
> > > > > > * * for RFC4106 ciphers, the concatenation of
> > > > > > * associated authentication data || IV || plaintext or ciphertext.
> > > > > > Note, the * same IV (buffer) is also set with the
> > > > > > aead_request_set_crypt call. Note, * the API call of
> > > > > > aead_request_set_ad must provide the length of the AAD and * the
> > > > > > IV.
> > > > > > The API call of aead_request_set_crypt only points to the size of *
> > > > > > the input plaintext or ciphertext.
> > > > > >
> > > > > >
> > > > > >
> > > > > > I seem to be missing the place where this is handled in
> > > > > > generate_random_aead_testvec()
> > > > > > and generate_aead_message()
> > > > > >
> > > > > >
> > > > > >
> > > > > > We seem to be generating a random IV for providing as the parameter
> > > > > > to
> > > > > > aead_request_set_crypt()
> > > > > > but than have other random bytes set in aead_request_set_ad() - or am
> > > > > > I'm missing something again?
> > > > >
> > > > >
> > > > >
> > > > > Yes, for rfc4106 the tests don't pass the same IV in both places. This
> > > > > is
> > > > > because I wrote the tests from the perspective of a generic AEAD that
> > > > > doesn't have this weird IV quirk, and then I added the minimum quirks
> > > > > to
> > > > > get the weird algorithms like rfc4106 passing.
> > > > >
> > > > >
> > > > >
> > > > > Since the actual behavior of the generic implementation of rfc4106 is
> > > > > that
> > > > > the last 8 bytes of the AAD are ignored, that means that currently the
> > > > > tests just avoid mutating these bytes when generating inauthentic input
> > > > > tests. They don't know that they're (apparently) meant to be another
> > > > > copy
> > > > > of the IV.
> > > > >
> > > > >
> > > > >
> > > > > So it seems we need to clearly define the behavior when the two IV
> > > > > copies
> > > > > don't match. Should one or the other be used, should an error be
> > > > > returned,
> > or should the behavior be unspecified (in which case the
> > > > > tests would need to be updated)?
> > > > >
> > > > >
> > > > >
> > > > > Unspecified behavior is bad, but it would be easiest for software to
> > > > > use
> > > > > req->iv, while hardware might want to use the IV in the scatterlist...
> > > > >
> > > > >
> > > > >
> > > > > Herbert and Stephan, any idea what was intended here?
> > > > >
> > > > >
> > > > >
> > > > > - Eric
> > > >
> > > >
> > > >
> > > > The full structure of RFC4106 is the following:
> > > >
> > > >
> > > >
> > > > - the key to be set is always 4 bytes larger than required for the
> > > > respective
> > AES operation (i.e. the key is 20, 28 or 36 bytes
> > > > respectively). The key value contains the following information: key ||
> > > > first 4 bytes of the IV (note, the first 4 bytes of the IV are the bytes
> > > > derived from the KDF invoked by IKE - i.e. they come from user space and
> > > > are fixed)
> > > >
> > > >
> > > >
> > > > - data block contains AAD || trailing 8 bytes of IV || plaintext or
> > > > ciphertext
> > - the trailing 8 bytes of the IV are the SPI which is updated
> > > > for each new IPSec package
> > > >
> > > >
> > >
> > > By SPI you must mean sequence number?
> > > (The SPI is actually the SA index which certainly doesn't change per
> > > packet!)
> > > That would be one possible way of generating the explicit IV, but
> > > you certainly cannot count on that. Anything unique under the key would be
> > > fine for GCM.
> >
> > The IV actually is generated with an IV generator (I think it is the SEQIV
> > generator from crypto/seqiv.c - it is set in the XFRM framework). It is a
> > deterministic construction XORed with a random number from the SP800-90A DRBG.
> >
> That would be a good way of generating IV's for CBC mode (which requires
> unpredictability and sufficient Hamming distance precluding a counter), but I would not
> recommend that for CTR based modes like GCM, where all you need is a nonce because:
>
> a) randomness does not guarantee uniqueness perse
> b) it is far too heavy on the CPU for this purpose
>
> So I would certainly hope it doesn't do it like that? The name seqiv alone would imply
> something based on a sequence numberand not a DRBG ... IIRC it was doing sequence
> number XOR some key material?
>
> > >
> > > > aead_request_set_ad points to the AAD plus the 8 bytes of IV in the use
> > > > case
> > of rfc4106(gcm(aes)) as part of IPSec.
> > > >
> > > >
> > > >
> > > > Considering your question about the aead_request_set_ad vs
> > > > aead_request_set_crypt I think the RFC4106 gives the answer: the IV is
> > > > used in
> > two locations considering that the IV is also the SPI in our
> > > > case. If you see RFC 4106 chapter 3 you see the trailing 8 bytes of the
> > > > IV as, well, the GCM IV (which is extended by the 4 byte salt as defined
> > > > in chapter 4 that we provide with the trailing 4 bytes of the key). The
> > > > kernel uses the SPI for this.>
> > > >
> > >
> > > Again, by SPI you must mean sequence number. The SPI itself is entirely
> > > seperate.
> >
> > See above, it is actually not the SPI, or sequence number, it is what the IV
> > generator provides.
> >
> Yes. But what you were describing sounded like the sequence number.
> Which would be perfectly legal to use _directly_ for this (unlike the SPI).
> Thats what our hardware does in case of full protocol offload.
>
> > > So the IV is not "used in two places", it is only used as IV for
> > > the AEAD operation, with the explicit part (8 bytes) inserted into the
> > > packet.
> > > [For GCM the IV, despite being in the AAD buffer, is _not_ authenticated]
> > > The sequence number _may_ be used in two places (AAD and explicit part of
> > > the IV),
> > > but that is not a given and out of the scope of the crypto API. I
> > > would not make any assumptions there.
> > >
> > > The "problem" Gilad was referring to is that the _explicit_ part of the IV
> > > appears to be
> > > available from both req->iv and from the AAD scatterbuffer.
> > > Which one should you use? API wise I would assume req->iv but from a (our)
> > > hardware perspective, it would be more efficient to extract it from the
> > > datastream. But is it allowed to assume there is a valid IV stored there?
> > > (which implies that it has to match req->iv, otherwise behaviour would
> > > deviate from implementations using that)
> >
> > req->iv is your IV.
> >
> But the IV is also in the last bytes of the AAD buffer. Which would be _way_ more
> convenient to use _directly_ compared to req->iv.
> Saves a lot of effort in both the driver and the HW to get the IV to where it
> is actually needed. For _our_ driver and hardware, anyway.
>
> So that's the point: if it's already where you want it to be, then why insisting
> on getting it from a different location (i.e. req->iv) just for the sake of entertaining
> some generic API? These rfcxxxx ciphersuites appear to be for a very specific use
> case (IPsec) and are already deviating from the normal AEAD implementations.
>
> > The use of the IV as part of the AAD is just a use case for rfc4106.
> >
> No, it is most definitely not. The IV is _not_ part of the AAD for rfc4106.
> Just take another long look at chapter 5 and tell me where it says "IV".
> It's just SPI and (full extended) sequence number, no more.
>
> So actually, the implementation needs to be aware of this and stop
> authenticating IV size bytes before the end of the AAD buffer. Which is
> rather strange if you think about it ... but I guess it is what is is now.
>
Then again, this was probably done to provide a common AEAD API from the
kernel IPsec stack, such that that doesn't need to worry about these ciphersuite
specific details ... and that had to be shoe-horned into the existing kernel crypto
API also making sure it doesn't get inefficient on that side ...
> > Although
> > I doubt that the rfc4106 structure will change any time soon, I would not use
> > the IV from the AAD but only look at the req->iv.
> >
> That is what I did mostly because I didn't know if I could rely on the IV in the
> AAD buffer being correct.
> BUT if you're not allowed to use it from the AAD buffer, then why is it even there?
>
> Let's put it differently: if I _would_ take it from the AAD buffer instead of req->iv,
> the current test vectors from testmgr.h would pass just fine(!)
>
> And how about GMAC (rfc4543), where IV _does_ need to be authenticated. If
> it doesn't match req->iv there, it would certainly not result in output complying
> with rfc4543. (though you could argue this to be useful for _other_ purposes)
>
> I think, especially considering the only user being the kernel IPsec stack here,
> it would make some sense to _require_ req->iv to match the AAD buffer IV and
> allow taking the IV from there instead of from req->iv, if that is more convenient.
>
> > >
> > > > In chapter 5 RFC4106 you see that the SP is however used as part of the
> > > > AAD as
> > well.
> > > >
> > > >
> > > >
> > > > Bottom line: if you do not set the same IV value for both, the AAD and the
> > > > GCM
> > IV, you deviate from the use case of rfc4106(gcm(aes)) in IPSec.
> > > > Yet, from a pure mathematical point of view and also from a cipher
> > > > implementation point of view, it does not matter whether the AAD and the
> > > > IV point to the same value - the implementation must always process that
> > > > data. The result however will not be identical to the IPSec use case.
> > > >
> > > >
> > >
> > > For the IPsec use case, it's perfectly legal to have IV != sequence number
> > > as long
> > > as it is unique under the key.
> >
> > Right, it is a perfectly legal way of doing it, but it is currently not done
> > that way in the kernel.
> >
> I guess my main point was there is no "IV for AAD" (if not the sequence number)
> so it can't possibly mismatch the "IV for GCM", ergo it's not possible to deviate from
> any IPsec use case. (for GCM anyway, for GMAC you could)
>
> > Thus, I would reiterate my suggestion from above to always use req->iv as your IV.
> >
> Which is what I do, BUT is rather silly _if_ req->iv in practice will always point to
> the IV stored in the AAD scatter buffer.
>
> > > So you should not assume the sequence number part of the AAD buffer to
> > > match
> > > the IV part (or req->iv), but it _would_ make sense if the IV part
> > > of the AAD matches req->iv. (then again, if this is not _required_ by the
> > > API the application might not bother providing it, which is my reason not
> > > to use in in the inside_secure driver)
> >
> > Precisely.
> >
> > Ciao
> > Stephan
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect Multi-Protocol Engines, Rambus Security
> Rambus ROTW Holding BV
> +31-73 6581953
>
> Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
> Please be so kind to update your e-mail address book with my new e-mail address.
>
> ** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential
> and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it.
> Please delete the message and attachments and notify the sender immediately. **
>
> Rambus
> Inc.<https://nam12.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.rambus.com&data=01%7C01%7Cpvanleeuwe
> n%40verimatrix.com%7C5c74040eea5748c69aee08d7abe388cb%7Cdcb260f9022d44958602eae51035a0d0%7C0&sdata=Wlq96le14
> BiueepIAtGY6MykFRcKKcR7JGnYNAYVqPM%3D&reserved=0>
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
On Fri, Feb 7, 2020 at 2:30 PM Stephan Mueller <[email protected]> wrote:
>
> Am Freitag, 7. Februar 2020, 12:50:51 CET schrieb Gilad Ben-Yossef:
>
> Hi Gilad,
>
> >
> > It is correct, but is it smart?
> >
> > Either we require the same IV to be passed twice as we do today, in which
> > case passing different IV should fail in a predictable manner OR we should
> > define the operation is taking two IV like structures - one as the IV and
> > one as bytes in the associated data and have the IPsec code use it in a
> > specific way of happen to pass the same IV in both places.
> >
> > I don't care either way - but right now the tests basically relies on
> > undefined behaviour
> > which is always a bad thing, I think.
>
> I am not sure about the motivation of this discussion: we have exactly one
> user of the RFC4106 implementation: IPSec. Providing the IV/AAD is efficient
> as the rfc4106 template intents to require the data in a format that requires
> minimal processing on the IPSec side to bring it in the right format.
>
The motivation for this discussion is that our current test suite for
RFC4106 generates test messages where req->iv is different than the
copy in the associated data.
This is not per my interpretation of RFC 4106, this is not the API as
is described in the header files and finally it is not per the use
case of the single user of RFC 4106 in the kernel and right now these
tests
causes the ccree driver to fail these tests.
Again, I am *not* suggesting or discussing changing the API.
I am asking the very practical question if it makes sense to me to
delve into understanding why this use case is failing versus fixing
the test suite to test what we actually use.
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
On Fri, Feb 7, 2020 at 4:07 PM Van Leeuwen, Pascal
<[email protected]> wrote:
> The "problem" Gilad was referring to is that the _explicit_ part of the IV appears to be
> available from both req->iv and from the AAD scatterbuffer. Which one should you use?
> API wise I would assume req->iv but from a (our) hardware perspective, it would
> be more efficient to extract it from the datastream. But is it allowed to assume
> there is a valid IV stored there? (which implies that it has to match req->iv,
> otherwise behaviour would deviate from implementations using that)
>
No, it isn't.
The problem that I was referring to was that part of our test suites
passes different values in req->iv and as part of the AAD,
in contrast to what we document as the API requirements in the include
file, my understanding of the relevant standard and
the single users of this API in the kernel and that the driver I'm
maintaining fails these tests,
I'm all fine with getting my hands dirty and fixing the driver, I'm
just suspect fixing a driver to pass a test that misuses the API
may not actually improve the quality of the driver.
Gilad
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf Of Gilad Ben-Yossef
> Sent: Sunday, February 9, 2020 9:05 AM
> To: Stephan Mueller <[email protected]>
> Cc: Eric Biggers <[email protected]>; Herbert Xu <[email protected]>; Linux Crypto Mailing List <linux-
> [email protected]>; Geert Uytterhoeven <[email protected]>; David Miller <[email protected]>; Ofir Drang
> <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> On Fri, Feb 7, 2020 at 2:30 PM Stephan Mueller <[email protected]> wrote:
> >
> > Am Freitag, 7. Februar 2020, 12:50:51 CET schrieb Gilad Ben-Yossef:
> >
> > Hi Gilad,
> >
> > >
> > > It is correct, but is it smart?
> > >
> > > Either we require the same IV to be passed twice as we do today, in which
> > > case passing different IV should fail in a predictable manner OR we should
> > > define the operation is taking two IV like structures - one as the IV and
> > > one as bytes in the associated data and have the IPsec code use it in a
> > > specific way of happen to pass the same IV in both places.
> > >
> > > I don't care either way - but right now the tests basically relies on
> > > undefined behaviour
> > > which is always a bad thing, I think.
> >
> > I am not sure about the motivation of this discussion: we have exactly one
> > user of the RFC4106 implementation: IPSec. Providing the IV/AAD is efficient
> > as the rfc4106 template intents to require the data in a format that requires
> > minimal processing on the IPSec side to bring it in the right format.
> >
>
> The motivation for this discussion is that our current test suite for
> RFC4106 generates test messages where req->iv is different than the
> copy in the associated data.
>
Interesting ... this must be a recent change then, because that's not what
I remember and it's also not in the current 5.6-rc1 tree from Linus.
So what would you expect then? That it takes the IV from req->iv and
totally ignores the AAD data part? That would be the only behavor making
sense for rfc4106 specifically. Leaves the question why you would allow
the application to supply totally random data to the ciphersuite.
But then what about rfc4543 where you have the same API (presumably
rfc4106 was aligned with that?) but you MUST have req->iv matching that
AAD data otherwise you're not compliant with that RFC. (regardless of
whether that 'might be useful' - the name would be wrong then)
> This is not per my interpretation of RFC 4106, this is not the API as
> is described in the header files and finally it is not per the use
> case of the single user of RFC 4106 in the kernel and right now these
> tests
> causes the ccree driver to fail these tests.
>
Agree
> Again, I am *not* suggesting or discussing changing the API.
>
The API just needs some clarification in this area. It makes sense to
_require_ req->iv and the IV part at the end of the AAD buffer to be
_identical_ such that the driver can _assume_ this to be the case.
Considering these ciphersuites are really specific to IPsec ESP.
> I am asking the very practical question if it makes sense to me to
> delve into understanding why this use case is failing versus fixing
> the test suite to test what we actually use.
>
> Gilad
>
> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
>
> values of β will give rise to dom!
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
> -----Original Message-----
> From: Gilad Ben-Yossef <[email protected]>
> Sent: Sunday, February 9, 2020 9:10 AM
> To: Van Leeuwen, Pascal <[email protected]>
> Cc: Stephan Mueller <[email protected]>; Eric Biggers <[email protected]>; Herbert Xu <[email protected]>;
> Linux Crypto Mailing List <[email protected]>; Geert Uytterhoeven <[email protected]>; David Miller
> <[email protected]>; Ofir Drang <[email protected]>
> Subject: Re: Possible issue with new inauthentic AEAD in extended crypto tests
>
> <<< External Email >>>
> On Fri, Feb 7, 2020 at 4:07 PM Van Leeuwen, Pascal
> <[email protected]> wrote:
>
> > The "problem" Gilad was referring to is that the _explicit_ part of the IV appears to be
> > available from both req->iv and from the AAD scatterbuffer. Which one should you use?
> > API wise I would assume req->iv but from a (our) hardware perspective, it would
> > be more efficient to extract it from the datastream. But is it allowed to assume
> > there is a valid IV stored there? (which implies that it has to match req->iv,
> > otherwise behaviour would deviate from implementations using that)
> >
>
>
> No, it isn't.
>
> The problem that I was referring to was that part of our test suites
> passes different values in req->iv and as part of the AAD,
> in contrast to what we document as the API requirements in the include
> file, my understanding of the relevant standard and
> the single users of this API in the kernel and that the driver I'm
> maintaining fails these tests,
>
But that's the same problem. If they were identical it doesn't matter
which one your driver uses, but because the testsuite now makes
them unequal you have a problem if you happen to use the other one.
> I'm all fine with getting my hands dirty and fixing the driver, I'm
> just suspect fixing a driver to pass a test that misuses the API
> may not actually improve the quality of the driver.
>
> Gilad
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
On Thu, Feb 06, 2020 at 11:27:09PM -0800, Eric Biggers wrote:
>
> Yes, for rfc4106 the tests don't pass the same IV in both places. This is
> because I wrote the tests from the perspective of a generic AEAD that doesn't
> have this weird IV quirk, and then I added the minimum quirks to get the weird
> algorithms like rfc4106 passing.
>
> Since the actual behavior of the generic implementation of rfc4106 is that the
> last 8 bytes of the AAD are ignored, that means that currently the tests just
> avoid mutating these bytes when generating inauthentic input tests. They don't
> know that they're (apparently) meant to be another copy of the IV.
>
> So it seems we need to clearly define the behavior when the two IV copies don't
> match. Should one or the other be used, should an error be returned, or should
> the behavior be unspecified (in which case the tests would need to be updated)?
>
> Unspecified behavior is bad, but it would be easiest for software to use
> req->iv, while hardware might want to use the IV in the scatterlist...
>
> Herbert and Stephan, any idea what was intended here?
I think unspecified would be OK here to give the hardware the
maximum latitude. However, we also don't want it to crash or
do something funny so perhaps generate the test vectors as you
do now but compare it against the generic using two IV values?
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt