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. > > - 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!
