Re: Possible issue with new inauthentic AEAD in extended crypto tests

[Gilad Ben-Yossef <gilad@benyossef.com>]

On Tue, Jan 28, 2020 at 11:12 PM Eric Biggers <ebiggers@kernel.org> 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!