| Message ID | 20221108142025.13461-1-nstange@suse.de |
|---|---|
| Headers | show |
| Series | Trivial set of FIPS 140-3 related changes | expand |
> diff --git a/include/crypto/xts.h b/include/crypto/xts.h ... > @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher > *tfm, > if (keylen % 2) > return -EINVAL; > > + /* > + * In FIPS mode only a combined key length of either 256 or > + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I. > + */ > + if (fips_enabled && keylen != 32 && keylen != 64) > + return -EINVAL; > + > /* ensure that the AES and tweak key are not identical */ > if ((fips_enabled || (crypto_skcipher_get_flags(tfm) & > CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) && > -- > 2.38.0 arch/s390/crypto/aes_s390.c has similar lines: static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key, unsigned int key_len) { struct s390_xts_ctx *xts_ctx = crypto_skcipher_ctx(tfm); unsigned long fc; int err; err = xts_fallback_setkey(tfm, in_key, key_len); if (err) return err; /* In fips mode only 128 bit or 256 bit keys are valid */ if (fips_enabled && key_len != 32 && key_len != 64) return -EINVAL; xts_fallback_setkey will now enforce that rule when setting up the fallback algorithm keys, which makes the xts_aes_set_key check unreachable. If that fallback setup were not present, then a call to xts_verify_key might be preferable to enforce any other rules like the WEAK_KEYS rule.
> diff --git a/include/crypto/xts.h b/include/crypto/xts.h ... > @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher > *tfm, > if (keylen % 2) > return -EINVAL; > > + /* > + * In FIPS mode only a combined key length of either 256 or > + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I. > + */ > + if (fips_enabled && keylen != 32 && keylen != 64) > + return -EINVAL; > + > /* ensure that the AES and tweak key are not identical */ > if ((fips_enabled || (crypto_skcipher_get_flags(tfm) & > CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) && > -- > 2.38.0 There's another function in the same file called xts_check_key() that is used by some of the hardware drivers: arch/s390/crypto/paes_s390.c: * xts_check_key verifies the key length is not odd and makes [that references it in the comment but actually calls xts_verify_key in the code] drivers/crypto/axis/artpec6_crypto.c: ret = xts_check_key(&cipher->base, key, keylen); drivers/crypto/cavium/cpt/cptvf_algs.c: err = xts_check_key(tfm, key, keylen); drivers/crypto/cavium/nitrox/nitrox_skcipher.c: ret = xts_check_key(tfm, key, keylen); drivers/crypto/ccree/cc_cipher.c: xts_check_key(tfm, key, keylen)) { drivers/crypto/marvell/octeontx/otx_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen); drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen); drivers/crypto/atmel-aes.c: err = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen); It already has one check qualified by fips_enabled: /* ensure that the AES and tweak key are not identical */ if (fips_enabled && !crypto_memneq(key, key + (keylen / 2), keylen / 2)) return -EINVAL; Should that implement the same key length restrictions?
"Elliott, Robert (Servers)" <elliott@hpe.com> writes: >> diff --git a/include/crypto/xts.h b/include/crypto/xts.h > ... >> @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher >> *tfm, >> if (keylen % 2) >> return -EINVAL; >> >> + /* >> + * In FIPS mode only a combined key length of either 256 or >> + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I. >> + */ >> + if (fips_enabled && keylen != 32 && keylen != 64) >> + return -EINVAL; >> + >> /* ensure that the AES and tweak key are not identical */ >> if ((fips_enabled || (crypto_skcipher_get_flags(tfm) & >> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) && >> -- >> 2.38.0 > > There's another function in the same file called xts_check_key() > that is used by some of the hardware drivers: Right, thanks for spotting. AFAICT, xts_check_key() is the older of the two variants, xts_verify_key() had been introduced with commit f1c131b45410 ("crypto: xts - Convert to skcipher"). There had initially only been a single call from generic crypto/xts.c and the main difference to xts_check_key() had been that it took a crypto_skcipher for its tfm argument rather than a plain crypto_tfm as xts_check_key() did. It seems that over time, xts crypto drivers adopted the newer xts_verify_key() variant then. > > arch/s390/crypto/paes_s390.c: * xts_check_key verifies the key length is not odd and makes > [that references it in the comment but actually calls xts_verify_key in the code] > drivers/crypto/axis/artpec6_crypto.c: ret = xts_check_key(&cipher->base, key, keylen); > drivers/crypto/cavium/cpt/cptvf_algs.c: err = xts_check_key(tfm, key, keylen); > drivers/crypto/cavium/nitrox/nitrox_skcipher.c: ret = xts_check_key(tfm, key, keylen); > drivers/crypto/ccree/cc_cipher.c: xts_check_key(tfm, key, keylen)) { > drivers/crypto/marvell/octeontx/otx_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen); > drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c: ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen); > drivers/crypto/atmel-aes.c: err = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen); > > It already has one check qualified by fips_enabled: > > /* ensure that the AES and tweak key are not identical */ > if (fips_enabled && !crypto_memneq(key, key + (keylen / 2), keylen / 2)) > return -EINVAL; > > Should that implement the same key length restrictions?
"Elliott, Robert (Servers)" <elliott@hpe.com> writes: >> diff --git a/include/crypto/xts.h b/include/crypto/xts.h > ... >> @@ -35,6 +35,13 @@ static inline int xts_verify_key(struct crypto_skcipher >> *tfm, >> if (keylen % 2) >> return -EINVAL; >> >> + /* >> + * In FIPS mode only a combined key length of either 256 or >> + * 512 bits is allowed, c.f. FIPS 140-3 IG C.I. >> + */ >> + if (fips_enabled && keylen != 32 && keylen != 64) >> + return -EINVAL; >> + >> /* ensure that the AES and tweak key are not identical */ >> if ((fips_enabled || (crypto_skcipher_get_flags(tfm) & >> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) && >> -- >> 2.38.0 > > arch/s390/crypto/aes_s390.c has similar lines: > > static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key, > unsigned int key_len) > { > struct s390_xts_ctx *xts_ctx = crypto_skcipher_ctx(tfm); > unsigned long fc; > int err; > > err = xts_fallback_setkey(tfm, in_key, key_len); > if (err) > return err; > > /* In fips mode only 128 bit or 256 bit keys are valid */ > if (fips_enabled && key_len != 32 && key_len != 64) > return -EINVAL; > > > xts_fallback_setkey will now enforce that rule when setting up the > fallback algorithm keys, which makes the xts_aes_set_key check > unreachable. Good finding! > > If that fallback setup were not present, then a call to xts_verify_key > might be preferable to enforce any other rules like the WEAK_KEYS > rule. > So if this patch here would get accepted, I'd propose to remove the then dead code from aes_s390 afterwards and make an explicit call to xts_verify_key() instead. Or shall I split out the XTS patch from this series here and post these two changes separately then? Herbert, any preferences? Thanks! Nicolai
On Wed, Nov 09, 2022 at 11:39:19AM +0100, Nicolai Stange wrote: > > Or shall I split out the XTS patch from this series here and post these > two changes separately then? Herbert, any preferences? You can do this as a follow-up. Thanks,
On Wed, Nov 09, 2022 at 11:06:17AM +0100, Nicolai Stange wrote: > > >From a quick glance, all of the above drivers merely convert some > crypto_skcipher to a crypto_tfm before passing it to xts_check_key(). > > So I think these should all be made to call xts_verify_key() directly > instead, the former xts_check_key() could then get dropped. But that's > more of a cleanup IMO and would probably deserve a separate patch series > on its own. We should make sure both do the same thing though. So either change all the drivers or just change xts_check_key in your patch in addition to xts_verify_key. Cheers,
On Wed, Dec 21, 2022 at 04:24:00PM +0100, Vladis Dronov wrote: > Hi Nicolai, Robert, Herbert, all, > > I would like to revive this older upstream email thread. I would like > to address notes from reviewers (namely, Robert) by additional patches > so the whole patchset can be accepted. This should ease our future > kernel work re: FIPS. > > The below 2 patches address (I hope) both notes Robert and Herbert have > provided (thanks!). I hope the whole patchset can be accepted then. > > Logically my 2 patches should follow [PATCH 1/4] and be patches 2 and 3. > Herbert is it possible to reorder them when accepting? > > Thank you! and > > Best regards, > Vladis Please just resend the whole series, with the --base option to git format-patch used, so that reviewers don't have to try to piece it together. - Eric
Hi, On Wed, Dec 21, 2022 at 9:56 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, Dec 21, 2022 at 04:24:00PM +0100, Vladis Dronov wrote: > > Hi Nicolai, Robert, Herbert, all, > > > > I would like to revive this older upstream email thread. I would like > > to address notes from reviewers (namely, Robert) by additional patches > > so the whole patchset can be accepted. This should ease our future > > kernel work re: FIPS. > > > > The below 2 patches address (I hope) both notes Robert and Herbert have > > provided (thanks!). I hope the whole patchset can be accepted then. > > > > Logically my 2 patches should follow [PATCH 1/4] and be patches 2 and 3. > > Herbert is it possible to reorder them when accepting? > > > > Thank you! and > > > > Best regards, > > Vladis > > Please just resend the whole series, with the --base option to git format-patch > used, so that reviewers don't have to try to piece it together. Thank you, Eric, the patchset was resend with a proper ordering: https://lore.kernel.org/linux-crypto/20221221224111.19254-1-vdronov@redhat.com/T/#t with a subject: [PATCH 0/6] Trivial set of FIPS 140-3 related changes Best regards, Vladis