| Message ID | aCWlmOE6VQJoYeaJ@gondor.apana.org.au |
|---|---|
| State | Superseded |
| Headers | show |
| Series | crypto: lrw - Only add ecb if it is not already there | expand |
On Thu, May 15, 2025 at 04:28:08PM +0800, Herbert Xu wrote: > On Thu, May 15, 2025 at 03:41:05PM +0800, kernel test robot wrote: > > > > [ 16.077514][ T327] ------------[ cut here ]------------ > > [ 16.079451][ T327] alg: self-tests for lrw(twofish) using lrw(ecb(twofish-asm)) failed (rc=-22) > > The crucial line actually got cut off: > > alg: skcipher: error allocating lrw(ecb(twofish-generic)) (generic impl of lrw(twofish)): -22 > > The bug is in lrw, which unconditionally adds ecb() around its > parameter, so we end up with ecb(ecb(twofish-generic)), which is > then correctly rejected by the ecb template when it tries to > create the inner ecb(twofish-generic) as a simple cipher. Please include this explanation in the commit message itself. Also adding ecb wasn't unconditional, but only if it wasn't found with one ecb. > Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher") It didn't actually make a difference until 795f85fca229 ("crypto: algboss - Pass instance creation error up") though, right? Before then, if "ecb(...)" gave ENOENT then "ecb(ecb(...))" gave ENOENT too. As I said in https://lore.kernel.org/linux-crypto/20240924222839.GC1585@sol.localdomain/, that commit (which had no explanation) just seems wrong. We should have simply stuck with ENOENT. But as usual my concern just got ignored and it got pushed out anyway. - Eric
On Thu, May 15, 2025 at 01:04:55PM -0700, Eric Biggers wrote: > > It didn't actually make a difference until 795f85fca229 ("crypto: algboss - Pass > instance creation error up") though, right? Before then, if "ecb(...)" gave > ENOENT then "ecb(ecb(...))" gave ENOENT too. I would consider this a success, it actually caught lrw doing something silly by trying to allocate "ecb(ecb(XXX))". Sure we can sweep all of this under the rug with ENOENT but it might end up blowing up in a much worse place. Also I don't think it's true that ecb(ecb(XXX)) will always give an ENOENT if ecb(XXX) gives an ENOENT. The error is actually originating from the lskcipher code, which tries to construct ecb from either an lskcipher, or a simple cipher. It is the latter that triggers the EINVAL error since the ecb template cannot create a simple cipher. > As I said in > https://lore.kernel.org/linux-crypto/20240924222839.GC1585@sol.localdomain/, > that commit (which had no explanation) just seems wrong. We should have simply > stuck with ENOENT. > > But as usual my concern just got ignored and it got pushed out anyway. I don't ignore all your concerns, just the ones that I think are unwarranted :) Cheers,
diff --git a/crypto/lrw.c b/crypto/lrw.c index e7f0368f8c97..dd403b800513 100644 --- a/crypto/lrw.c +++ b/crypto/lrw.c @@ -322,7 +322,7 @@ static int lrw_create(struct crypto_template *tmpl, struct rtattr **tb) err = crypto_grab_skcipher(spawn, skcipher_crypto_instance(inst), cipher_name, 0, mask); - if (err == -ENOENT) { + if (err == -ENOENT && memcmp(cipher_name, "ecb(", 4)) { err = -ENAMETOOLONG; if (snprintf(ecb_name, CRYPTO_MAX_ALG_NAME, "ecb(%s)", cipher_name) >= CRYPTO_MAX_ALG_NAME) @@ -356,7 +356,7 @@ static int lrw_create(struct crypto_template *tmpl, struct rtattr **tb) /* Alas we screwed up the naming so we have to mangle the * cipher name. */ - if (!strncmp(cipher_name, "ecb(", 4)) { + if (!memcmp(cipher_name, "ecb(", 4)) { int len; len = strscpy(ecb_name, cipher_name + 4, sizeof(ecb_name));