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