| Message ID | 20221108142025.13461-2-nstange@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| 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? 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. Thanks! Nicolai
"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,
diff --git a/include/crypto/xts.h b/include/crypto/xts.h index 0f8dba69feb4..a233c1054df2 100644 --- 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)) &&
According to FIPS 140-3 IG C.I., only (total) key lengths of either 256 bits or 512 bits are allowed with xts(aes). Make xts_verify_key() to reject anything else in FIPS mode. As xts(aes) is the only approved xts() template instantiation in FIPS mode, the new restriction implemented in xts_verify_key() effectively only applies to this particular construction. Signed-off-by: Nicolai Stange <nstange@suse.de> --- include/crypto/xts.h | 7 +++++++ 1 file changed, 7 insertions(+)