| Message ID | 2444206.rbm5Z3UBF9@tachyon.chronox.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On Fri, May 29, 2015 at 12:09:35PM +0200, Stephan Mueller wrote: > > > - return (ctx->used >= (ctx->aead_assoclen + (ctx->enc ? 0 : as))); > > + return ctx->used >= ctx->aead_assoclen + as; > > This change requires that the buffer handed in by user space always has room > for the tag, regardless whether it is needed or not. Is that intended? Yes for two reasons. One is that sometimes we need to enforce in-place processing, in which case dst must be at least as big as src. The other reason is to eventually allow in-place processing through algif_aead. Unless the two SG lists were of the same length, it isn't possible to do that. > > - /* add the size needed for the auth tag to be created */ > > - outlen += as; > > - } else { > > - /* output data size is input without the authentication tag */ > > - outlen = used - as; > > The removal of these make me wonder: with those missing, the output of the > cipher operation does not have CT || tag (in case of encryption) or PT (in > case of encryption. > > Note, I have updated my user space code to require space for the AD in the > output buffer. When reverting those changes with the following patch, the code > works nicely. If I do not apply the patch, the beginning of the CT or PT is as > expected, but the end is bogus. Also, the tag would be missing. Well used is now supposed to always contain the tag in both cases. Can you send me the code that you're using to test this and I'll do some tests on it. > However, when use those changes and I perform the test of libkcapi/test/kcapi > -y -s, I get the following strange crash which i have no idea where to look > for the cause (normal sendmsg and vmsplice tests with libkcapi/test/kcapi -y > and libkcapi/test/kcapi -y -v work flawless) This is clearly not good. It looks like memory corruption. Thanks,
Am Freitag, 29. Mai 2015, 21:28:40 schrieb Herbert Xu: Hi Herbert, >On Fri, May 29, 2015 at 12:09:35PM +0200, Stephan Mueller wrote: >> > - return (ctx->used >= (ctx->aead_assoclen + (ctx->enc ? 0 : as))); >> > + return ctx->used >= ctx->aead_assoclen + as; >> >> This change requires that the buffer handed in by user space always has >> room >> for the tag, regardless whether it is needed or not. Is that intended? > >Yes for two reasons. One is that sometimes we need to enforce >in-place processing, in which case dst must be at least as big >as src. The other reason is to eventually allow in-place processing >through algif_aead. Unless the two SG lists were of the same length, >it isn't possible to do that. Do we really need to copy in and copy out unneeded data? That sounds very inefficient. Besides, can't we leave it to user space to build the right memory structure? I.e. if user space wants in-place operation, it needs to ensure that the one buffer is sufficient for the requested operation (i.e. that the requirements for src lengths and dst lengths are covered). >> However, when use those changes and I perform the test of >> libkcapi/test/kcapi -y -s, I get the following strange crash which i have >> no idea where to look for the cause (normal sendmsg and vmsplice tests >> with libkcapi/test/kcapi -y and libkcapi/test/kcapi -y -v work flawless) > >This is clearly not good. It looks like memory corruption. It seems it is triggered by my change suggestions. It is triggered in the while() loop in the recvmsg when allocating an output buffer larger than 16 pages. So, this is nothing in the current upstream code. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Freitag, 29. Mai 2015, 16:50:50 schrieb Stephan Mueller: Hi Herbert, > > Do we really need to copy in and copy out unneeded data? That sounds very > inefficient. Besides, can't we leave it to user space to build the right > memory structure? I.e. if user space wants in-place operation, it needs to > ensure that the one buffer is sufficient for the requested operation (i.e. > that the requirements for src lengths and dst lengths are covered). Please disregard my comment for the memory structure -- using IOVECs, userspace is free to format the memory layout as necessary. PS: I tested all code paths that I see for the new algif_aead and it works with the code currently in your tree. Please disregard the discussion around code changes.
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c index 38a6cab..b6af158 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -72,7 +72,7 @@ static inline bool aead_sufficient_data(struct aead_ctx *ctx) { unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(&ctx- >aead_req)); - return ctx->used >= ctx->aead_assoclen + as; + return ctx->used >= ctx->aead_assoclen + (ctx->enc ? 0 : as); }