> -----Original Message-----
> From: linux-crypto-owner@xxxxxxxxxxxxxxx <linux-crypto-owner@xxxxxxxxxxxxxxx> On Behalf Of Gilad Ben-Yossef
> Sent: Wednesday, January 29, 2020 12:28 PM
> To: Eric Biggers <ebiggers@xxxxxxxxxx>
> Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>; Stephan Mueller <smueller@xxxxxxxxxx>; Linux Crypto Mailing List <linux-
> crypto@xxxxxxxxxxxxxxx>; Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>; David Miller <davem@xxxxxxxxxxxxx>; Ofir Drang
> <Ofir.Drang@xxxxxxx>
> 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 <ebiggers@xxxxxxxxxx> 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>
