All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes
@ 2022-12-29 21:17 Vladis Dronov
  2022-12-29 21:17 ` [PATCH v3 1/6] crypto: xts - restrict key lengths to approved values in FIPS mode Vladis Dronov
                   ` (6 more replies)
  0 siblings, 7 replies; 10+ messages in thread
From: Vladis Dronov @ 2022-12-29 21:17 UTC (permalink / raw)
  To: Herbert Xu, David S . Miller
  Cc: Nicolai Stange, Elliott Robert, Stephan Mueller, Eric Biggers,
	linux-crypto, linux-kernel, Vladis Dronov

Hi,

This patchset and cover letter was initially composed by Nicolai Stange
and sent earlier as:

https://lore.kernel.org/r/20221108142025.13461-1-nstange@suse.de/
with a subject: [PATCH 0/4] Trivial set of FIPS 140-3 related changes

I'm adding patches 2 and 3 which (I hope) resolve issues spotted by
reviewers of previous version of the patchset. This new patchset should
ease our future kernel work on the FIPS mode.

I'm quoting Nicolai's cover letter here:

> Hi all,
> 
> these four rather unrelated patches are basically a dump of some of the
> more trivial changes required for working towards FIPS 140-3 conformance.
> 
> Please pick as you deem appropriate.
> 
> Thanks!
> 
> Nicolai

v2: fixed a block comment formatting

v3: "Reviewed-by: Eric Biggers" was copied from the v1 thread:
    https://lore.kernel.org/r/Y6OXuT95MlkNanSR@sol.localdomain/

Nicolai Stange (4):
  crypto: xts - restrict key lengths to approved values in FIPS mode
  crypto: testmgr - disallow plain cbcmac(aes) in FIPS mode
  crypto: testmgr - disallow plain ghash in FIPS mode
  crypto: testmgr - allow ecdsa-nist-p256 and -p384 in FIPS mode

Vladis Dronov (2):
  crypto: xts - drop xts_check_key()
  crypto: xts - drop redundant xts key check

 arch/s390/crypto/aes_s390.c                   |  4 ---
 arch/s390/crypto/paes_s390.c                  |  2 +-
 crypto/testmgr.c                              |  4 +--
 drivers/crypto/atmel-aes.c                    |  2 +-
 drivers/crypto/axis/artpec6_crypto.c          |  2 +-
 drivers/crypto/cavium/cpt/cptvf_algs.c        |  8 +++---
 .../crypto/cavium/nitrox/nitrox_skcipher.c    |  8 +++---
 drivers/crypto/ccree/cc_cipher.c              |  2 +-
 .../crypto/marvell/octeontx/otx_cptvf_algs.c  |  2 +-
 .../marvell/octeontx2/otx2_cptvf_algs.c       |  2 +-
 include/crypto/xts.h                          | 25 +++++++------------
 11 files changed, 23 insertions(+), 38 deletions(-)

base-commit: b6bb9676f2165d518b35ba3bea5f1fcfc0d969bf
-- 
2.38.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/6] crypto: xts - restrict key lengths to approved values in FIPS mode
  2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
@ 2022-12-29 21:17 ` Vladis Dronov
  2022-12-29 21:17 ` [PATCH v3 2/6] crypto: xts - drop xts_check_key() Vladis Dronov
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Vladis Dronov @ 2022-12-29 21:17 UTC (permalink / raw)
  To: Herbert Xu, David S . Miller
  Cc: Nicolai Stange, Elliott Robert, Stephan Mueller, Eric Biggers,
	linux-crypto, linux-kernel, Vladis Dronov

From: Nicolai Stange <nstange@suse.de>

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>
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
 include/crypto/xts.h | 7 +++++++
 1 file changed, 7 insertions(+)

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)) &&
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/6] crypto: xts - drop xts_check_key()
  2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
  2022-12-29 21:17 ` [PATCH v3 1/6] crypto: xts - restrict key lengths to approved values in FIPS mode Vladis Dronov
@ 2022-12-29 21:17 ` Vladis Dronov
  2023-01-05 11:27   ` Nicolai Stange
  2022-12-29 21:17 ` [PATCH v3 3/6] crypto: xts - drop redundant xts key check Vladis Dronov
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 10+ messages in thread
From: Vladis Dronov @ 2022-12-29 21:17 UTC (permalink / raw)
  To: Herbert Xu, David S . Miller
  Cc: Nicolai Stange, Elliott Robert, Stephan Mueller, Eric Biggers,
	linux-crypto, linux-kernel, Vladis Dronov

xts_check_key() is obsoleted by xts_verify_key(). Over time XTS crypto
drivers adopted the newer xts_verify_key() variant, but xts_check_key()
is still used by a number of drivers. Switch drivers to use the newer
xts_verify_key() and make a couple of cleanups. This allows us to drop
xts_check_key() completely and avoid redundancy.

Signed-off-by: Vladis Dronov <vdronov@redhat.com>
Reviewed-by: Eric Biggers <ebiggers@google.com>
---
 arch/s390/crypto/paes_s390.c                  |  2 +-
 drivers/crypto/atmel-aes.c                    |  2 +-
 drivers/crypto/axis/artpec6_crypto.c          |  2 +-
 drivers/crypto/cavium/cpt/cptvf_algs.c        |  8 +++----
 .../crypto/cavium/nitrox/nitrox_skcipher.c    |  8 +++----
 drivers/crypto/ccree/cc_cipher.c              |  2 +-
 .../crypto/marvell/octeontx/otx_cptvf_algs.c  |  2 +-
 .../marvell/octeontx2/otx2_cptvf_algs.c       |  2 +-
 include/crypto/xts.h                          | 22 ++++---------------
 9 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/arch/s390/crypto/paes_s390.c b/arch/s390/crypto/paes_s390.c
index a279b7d23a5e..29dc827e0fe8 100644
--- a/arch/s390/crypto/paes_s390.c
+++ b/arch/s390/crypto/paes_s390.c
@@ -474,7 +474,7 @@ static int xts_paes_set_key(struct crypto_skcipher *tfm, const u8 *in_key,
 		return rc;
 
 	/*
-	 * xts_check_key verifies the key length is not odd and makes
+	 * xts_verify_key verifies the key length is not odd and makes
 	 * sure that the two keys are not the same. This can be done
 	 * on the two protected keys as well
 	 */
diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 886bf258544c..130f8bf09a9a 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -1879,7 +1879,7 @@ static int atmel_aes_xts_setkey(struct crypto_skcipher *tfm, const u8 *key,
 	struct atmel_aes_xts_ctx *ctx = crypto_skcipher_ctx(tfm);
 	int err;
 
-	err = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
+	err = xts_verify_key(tfm, key, keylen);
 	if (err)
 		return err;
 
diff --git a/drivers/crypto/axis/artpec6_crypto.c b/drivers/crypto/axis/artpec6_crypto.c
index 51c66afbe677..f6f41e316dfe 100644
--- a/drivers/crypto/axis/artpec6_crypto.c
+++ b/drivers/crypto/axis/artpec6_crypto.c
@@ -1621,7 +1621,7 @@ artpec6_crypto_xts_set_key(struct crypto_skcipher *cipher, const u8 *key,
 		crypto_skcipher_ctx(cipher);
 	int ret;
 
-	ret = xts_check_key(&cipher->base, key, keylen);
+	ret = xts_verify_key(cipher, key, keylen);
 	if (ret)
 		return ret;
 
diff --git a/drivers/crypto/cavium/cpt/cptvf_algs.c b/drivers/crypto/cavium/cpt/cptvf_algs.c
index 9eca0c302186..0b38c2600b86 100644
--- a/drivers/crypto/cavium/cpt/cptvf_algs.c
+++ b/drivers/crypto/cavium/cpt/cptvf_algs.c
@@ -232,13 +232,12 @@ static int cvm_decrypt(struct skcipher_request *req)
 static int cvm_xts_setkey(struct crypto_skcipher *cipher, const u8 *key,
 		   u32 keylen)
 {
-	struct crypto_tfm *tfm = crypto_skcipher_tfm(cipher);
-	struct cvm_enc_ctx *ctx = crypto_tfm_ctx(tfm);
+	struct cvm_enc_ctx *ctx = crypto_skcipher_ctx(cipher);
 	int err;
 	const u8 *key1 = key;
 	const u8 *key2 = key + (keylen / 2);
 
-	err = xts_check_key(tfm, key, keylen);
+	err = xts_verify_key(cipher, key, keylen);
 	if (err)
 		return err;
 	ctx->key_len = keylen;
@@ -289,8 +288,7 @@ static int cvm_validate_keylen(struct cvm_enc_ctx *ctx, u32 keylen)
 static int cvm_setkey(struct crypto_skcipher *cipher, const u8 *key,
 		      u32 keylen, u8 cipher_type)
 {
-	struct crypto_tfm *tfm = crypto_skcipher_tfm(cipher);
-	struct cvm_enc_ctx *ctx = crypto_tfm_ctx(tfm);
+	struct cvm_enc_ctx *ctx = crypto_skcipher_ctx(cipher);
 
 	ctx->cipher_type = cipher_type;
 	if (!cvm_validate_keylen(ctx, keylen)) {
diff --git a/drivers/crypto/cavium/nitrox/nitrox_skcipher.c b/drivers/crypto/cavium/nitrox/nitrox_skcipher.c
index 248b4fff1c72..138261dcd032 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_skcipher.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_skcipher.c
@@ -337,12 +337,11 @@ static int nitrox_3des_decrypt(struct skcipher_request *skreq)
 static int nitrox_aes_xts_setkey(struct crypto_skcipher *cipher,
 				 const u8 *key, unsigned int keylen)
 {
-	struct crypto_tfm *tfm = crypto_skcipher_tfm(cipher);
-	struct nitrox_crypto_ctx *nctx = crypto_tfm_ctx(tfm);
+	struct nitrox_crypto_ctx *nctx = crypto_skcipher_ctx(cipher);
 	struct flexi_crypto_context *fctx;
 	int aes_keylen, ret;
 
-	ret = xts_check_key(tfm, key, keylen);
+	ret = xts_verify_key(cipher, key, keylen);
 	if (ret)
 		return ret;
 
@@ -362,8 +361,7 @@ static int nitrox_aes_xts_setkey(struct crypto_skcipher *cipher,
 static int nitrox_aes_ctr_rfc3686_setkey(struct crypto_skcipher *cipher,
 					 const u8 *key, unsigned int keylen)
 {
-	struct crypto_tfm *tfm = crypto_skcipher_tfm(cipher);
-	struct nitrox_crypto_ctx *nctx = crypto_tfm_ctx(tfm);
+	struct nitrox_crypto_ctx *nctx = crypto_skcipher_ctx(cipher);
 	struct flexi_crypto_context *fctx;
 	int aes_keylen;
 
diff --git a/drivers/crypto/ccree/cc_cipher.c b/drivers/crypto/ccree/cc_cipher.c
index 309da6334a0a..2cd44d7457a4 100644
--- a/drivers/crypto/ccree/cc_cipher.c
+++ b/drivers/crypto/ccree/cc_cipher.c
@@ -460,7 +460,7 @@ static int cc_cipher_setkey(struct crypto_skcipher *sktfm, const u8 *key,
 	}
 
 	if (ctx_p->cipher_mode == DRV_CIPHER_XTS &&
-	    xts_check_key(tfm, key, keylen)) {
+	    xts_verify_key(sktfm, key, keylen)) {
 		dev_dbg(dev, "weak XTS key");
 		return -EINVAL;
 	}
diff --git a/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c b/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c
index 80ba77c793a7..83493dd0416f 100644
--- a/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c
+++ b/drivers/crypto/marvell/octeontx/otx_cptvf_algs.c
@@ -398,7 +398,7 @@ static int otx_cpt_skcipher_xts_setkey(struct crypto_skcipher *tfm,
 	const u8 *key1 = key;
 	int ret;
 
-	ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
+	ret = xts_verify_key(tfm, key, keylen);
 	if (ret)
 		return ret;
 	ctx->key_len = keylen;
diff --git a/drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c b/drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c
index 30b423605c9c..443202caa140 100644
--- a/drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c
+++ b/drivers/crypto/marvell/octeontx2/otx2_cptvf_algs.c
@@ -412,7 +412,7 @@ static int otx2_cpt_skcipher_xts_setkey(struct crypto_skcipher *tfm,
 	const u8 *key1 = key;
 	int ret;
 
-	ret = xts_check_key(crypto_skcipher_tfm(tfm), key, keylen);
+	ret = xts_verify_key(tfm, key, keylen);
 	if (ret)
 		return ret;
 	ctx->key_len = keylen;
diff --git a/include/crypto/xts.h b/include/crypto/xts.h
index a233c1054df2..15b16c4853d8 100644
--- a/include/crypto/xts.h
+++ b/include/crypto/xts.h
@@ -8,23 +8,6 @@
 
 #define XTS_BLOCK_SIZE 16
 
-static inline int xts_check_key(struct crypto_tfm *tfm,
-				const u8 *key, unsigned int keylen)
-{
-	/*
-	 * key consists of keys of equal size concatenated, therefore
-	 * the length must be even.
-	 */
-	if (keylen % 2)
-		return -EINVAL;
-
-	/* ensure that the AES and tweak key are not identical */
-	if (fips_enabled && !crypto_memneq(key, key + (keylen / 2), keylen / 2))
-		return -EINVAL;
-
-	return 0;
-}
-
 static inline int xts_verify_key(struct crypto_skcipher *tfm,
 				 const u8 *key, unsigned int keylen)
 {
@@ -42,7 +25,10 @@ static inline int xts_verify_key(struct crypto_skcipher *tfm,
 	if (fips_enabled && keylen != 32 && keylen != 64)
 		return -EINVAL;
 
-	/* ensure that the AES and tweak key are not identical */
+	/*
+	 * Ensure that the AES and tweak key are not identical when
+	 * in FIPS mode or the FORBID_WEAK_KEYS flag is set.
+	 */
 	if ((fips_enabled || (crypto_skcipher_get_flags(tfm) &
 			      CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) &&
 	    !crypto_memneq(key, key + (keylen / 2), keylen / 2))
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 3/6] crypto: xts - drop redundant xts key check
  2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
  2022-12-29 21:17 ` [PATCH v3 1/6] crypto: xts - restrict key lengths to approved values in FIPS mode Vladis Dronov
  2022-12-29 21:17 ` [PATCH v3 2/6] crypto: xts - drop xts_check_key() Vladis Dronov
@ 2022-12-29 21:17 ` Vladis Dronov
  2023-01-05 11:16   ` Nicolai Stange
  2022-12-29 21:17 ` [PATCH v3 4/6] crypto: testmgr - disallow plain cbcmac(aes) in FIPS mode Vladis Dronov
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 10+ messages in thread
From: Vladis Dronov @ 2022-12-29 21:17 UTC (permalink / raw)
  To: Herbert Xu, David S . Miller
  Cc: Nicolai Stange, Elliott Robert, Stephan Mueller, Eric Biggers,
	linux-crypto, linux-kernel, Vladis Dronov

xts_fallback_setkey() in xts_aes_set_key() will now enforce key size
rule in FIPS mode when setting up the fallback algorithm keys, which
makes the check in xts_aes_set_key() redundant or unreachable. So just
drop this check.

xts_fallback_setkey() now makes a key size check in xts_verify_key():

xts_fallback_setkey()
  crypto_skcipher_setkey() [ skcipher_setkey_unaligned() ]
    cipher->setkey() { .setkey = xts_setkey }
      xts_setkey()
        xts_verify_key()

Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
 arch/s390/crypto/aes_s390.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 526c3f40f6a2..c773820e4af9 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -398,10 +398,6 @@ static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key,
 	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;
-
 	/* Pick the correct function code based on the key length */
 	fc = (key_len == 32) ? CPACF_KM_XTS_128 :
 	     (key_len == 64) ? CPACF_KM_XTS_256 : 0;
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 4/6] crypto: testmgr - disallow plain cbcmac(aes) in FIPS mode
  2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
                   ` (2 preceding siblings ...)
  2022-12-29 21:17 ` [PATCH v3 3/6] crypto: xts - drop redundant xts key check Vladis Dronov
@ 2022-12-29 21:17 ` Vladis Dronov
  2022-12-29 21:17 ` [PATCH v3 5/6] crypto: testmgr - disallow plain ghash " Vladis Dronov
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Vladis Dronov @ 2022-12-29 21:17 UTC (permalink / raw)
  To: Herbert Xu, David S . Miller
  Cc: Nicolai Stange, Elliott Robert, Stephan Mueller, Eric Biggers,
	linux-crypto, linux-kernel, Vladis Dronov

From: Nicolai Stange <nstange@suse.de>

cbcmac(aes) may be used only as part of the ccm(aes) construction in FIPS
mode. Since commit d6097b8d5d55 ("crypto: api - allow algs only in specific
constructions in FIPS mode") there's support for using spawns which by
itself are marked as non-approved from approved template instantiations.
So simply mark plain cbcmac(aes) as non-approved in testmgr to block any
attempts of direct instantiations in FIPS mode.

Signed-off-by: Nicolai Stange <nstange@suse.de>
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
 crypto/testmgr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 4476ac97baa5..562463a77a76 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -4501,7 +4501,6 @@ static const struct alg_test_desc alg_test_descs[] = {
 	}, {
 #endif
 		.alg = "cbcmac(aes)",
-		.fips_allowed = 1,
 		.test = alg_test_hash,
 		.suite = {
 			.hash = __VECS(aes_cbcmac_tv_template)
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 5/6] crypto: testmgr - disallow plain ghash in FIPS mode
  2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
                   ` (3 preceding siblings ...)
  2022-12-29 21:17 ` [PATCH v3 4/6] crypto: testmgr - disallow plain cbcmac(aes) in FIPS mode Vladis Dronov
@ 2022-12-29 21:17 ` Vladis Dronov
  2022-12-29 21:17 ` [PATCH v3 6/6] crypto: testmgr - allow ecdsa-nist-p256 and -p384 " Vladis Dronov
  2023-01-06 15:18 ` [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Herbert Xu
  6 siblings, 0 replies; 10+ messages in thread
From: Vladis Dronov @ 2022-12-29 21:17 UTC (permalink / raw)
  To: Herbert Xu, David S . Miller
  Cc: Nicolai Stange, Elliott Robert, Stephan Mueller, Eric Biggers,
	linux-crypto, linux-kernel, Vladis Dronov

From: Nicolai Stange <nstange@suse.de>

ghash may be used only as part of the gcm(aes) construction in FIPS
mode. Since commit d6097b8d5d55 ("crypto: api - allow algs only in specific
constructions in FIPS mode") there's support for using spawns which by
itself are marked as non-approved from approved template instantiations.
So simply mark plain ghash as non-approved in testmgr to block any attempts
of direct instantiations in FIPS mode.

Signed-off-by: Nicolai Stange <nstange@suse.de>
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
 crypto/testmgr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 562463a77a76..a223cf5f3626 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -5125,7 +5125,6 @@ static const struct alg_test_desc alg_test_descs[] = {
 	}, {
 		.alg = "ghash",
 		.test = alg_test_hash,
-		.fips_allowed = 1,
 		.suite = {
 			.hash = __VECS(ghash_tv_template)
 		}
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 6/6] crypto: testmgr - allow ecdsa-nist-p256 and -p384 in FIPS mode
  2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
                   ` (4 preceding siblings ...)
  2022-12-29 21:17 ` [PATCH v3 5/6] crypto: testmgr - disallow plain ghash " Vladis Dronov
@ 2022-12-29 21:17 ` Vladis Dronov
  2023-01-06 15:18 ` [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Herbert Xu
  6 siblings, 0 replies; 10+ messages in thread
From: Vladis Dronov @ 2022-12-29 21:17 UTC (permalink / raw)
  To: Herbert Xu, David S . Miller
  Cc: Nicolai Stange, Elliott Robert, Stephan Mueller, Eric Biggers,
	linux-crypto, linux-kernel, Vladis Dronov

From: Nicolai Stange <nstange@suse.de>

The kernel provides implementations of the NIST ECDSA signature
verification primitives. For key sizes of 256 and 384 bits respectively
they are approved and can be enabled in FIPS mode. Do so.

Signed-off-by: Nicolai Stange <nstange@suse.de>
Signed-off-by: Vladis Dronov <vdronov@redhat.com>
---
 crypto/testmgr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index a223cf5f3626..795c4858c741 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -5034,12 +5034,14 @@ static const struct alg_test_desc alg_test_descs[] = {
 	}, {
 		.alg = "ecdsa-nist-p256",
 		.test = alg_test_akcipher,
+		.fips_allowed = 1,
 		.suite = {
 			.akcipher = __VECS(ecdsa_nist_p256_tv_template)
 		}
 	}, {
 		.alg = "ecdsa-nist-p384",
 		.test = alg_test_akcipher,
+		.fips_allowed = 1,
 		.suite = {
 			.akcipher = __VECS(ecdsa_nist_p384_tv_template)
 		}
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/6] crypto: xts - drop redundant xts key check
  2022-12-29 21:17 ` [PATCH v3 3/6] crypto: xts - drop redundant xts key check Vladis Dronov
@ 2023-01-05 11:16   ` Nicolai Stange
  0 siblings, 0 replies; 10+ messages in thread
From: Nicolai Stange @ 2023-01-05 11:16 UTC (permalink / raw)
  To: Vladis Dronov
  Cc: Herbert Xu, David S . Miller, Nicolai Stange, Elliott Robert,
	Stephan Mueller, Eric Biggers, linux-crypto, linux-kernel

Hi Vladis,

the patch subject prefix is a bit misleading IMO, it kind of suggests
that this patch would apply to the generic crypto/xts.c. How about using
a format similar to e.g. the one from commit 7988fb2c03c8 ("crypto:
s390/aes - convert to skcipher API"), i.e.

  "crypto: s390/aes - drop redundant xts key check"

?

Vladis Dronov <vdronov@redhat.com> writes:

> xts_fallback_setkey() in xts_aes_set_key() will now enforce key size
> rule in FIPS mode when setting up the fallback algorithm keys,

I think it would be nice to make it more explicit why/how
xts_fallback_setkey() happens to enforce the key size rules now.

Perhaps amend the above sentence by something like

  "xts_fallback_setkey() in xts_aes_set_key() will now implictly enforce
   the key size rule in FIPS mode by means of invoking the generic xts
   implementation with its key checks for setting up the fallback
   algorithm,"

?

> which makes the check in xts_aes_set_key() redundant or
> unreachable. So just drop this check.
>
> xts_fallback_setkey() now makes a key size check in xts_verify_key():
>
> xts_fallback_setkey()
>   crypto_skcipher_setkey() [ skcipher_setkey_unaligned() ]
>     cipher->setkey() { .setkey = xts_setkey }
>       xts_setkey()
>         xts_verify_key()
>
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> ---
>  arch/s390/crypto/aes_s390.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 526c3f40f6a2..c773820e4af9 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
> @@ -398,10 +398,6 @@ static int xts_aes_set_key(struct crypto_skcipher *tfm, const u8 *in_key,
>  	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;
> -

The change itself looks good, but it might be worth adding a comment
right at the invocation of xts_fallback_setkey() that this includes an
implicit xts_verify_key() check? So that if anybody ever was about to
remove the xts_fallback_setkey() for some reason in the future, it would
give a clear indication that xts_verify_key() needs to get called
directly instead?

Thanks!

Nicolai

>  	/* Pick the correct function code based on the key length */
>  	fc = (key_len == 32) ? CPACF_KM_XTS_128 :
>  	     (key_len == 64) ? CPACF_KM_XTS_256 : 0;

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
(HRB 36809, AG Nürnberg)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/6] crypto: xts - drop xts_check_key()
  2022-12-29 21:17 ` [PATCH v3 2/6] crypto: xts - drop xts_check_key() Vladis Dronov
@ 2023-01-05 11:27   ` Nicolai Stange
  0 siblings, 0 replies; 10+ messages in thread
From: Nicolai Stange @ 2023-01-05 11:27 UTC (permalink / raw)
  To: Vladis Dronov
  Cc: Herbert Xu, David S . Miller, Nicolai Stange, Elliott Robert,
	Stephan Mueller, Eric Biggers, linux-crypto, linux-kernel

Hi Vladis,

Vladis Dronov <vdronov@redhat.com> writes:

> xts_check_key() is obsoleted by xts_verify_key(). Over time XTS crypto
> drivers adopted the newer xts_verify_key() variant, but xts_check_key()
> is still used by a number of drivers. Switch drivers to use the newer
> xts_verify_key() and make a couple of cleanups. This allows us to drop
> xts_check_key() completely and avoid redundancy.

nice work, thanks a lot for doing it!

There are two (obviously correct) cleanups which seem unrelated to
xts_verify_key(), namely in cvm_setkey() and
nitrox_aes_ctr_rfc3686_setkey(), see below.

But the commit message does say "and make a couple of cleanups" and
these changes certainly qualify as such. I just wanted to have it
mentioned.

Reviewed-by: Nicolai Stange <nstange@suse.de>

Thanks!

Nicolai


>
> Signed-off-by: Vladis Dronov <vdronov@redhat.com>
> Reviewed-by: Eric Biggers <ebiggers@google.com>
> ---

<snip>

> diff --git a/drivers/crypto/cavium/cpt/cptvf_algs.c b/drivers/crypto/cavium/cpt/cptvf_algs.c
> index 9eca0c302186..0b38c2600b86 100644
> --- a/drivers/crypto/cavium/cpt/cptvf_algs.c
> +++ b/drivers/crypto/cavium/cpt/cptvf_algs.c

<snip>

> @@ -289,8 +288,7 @@ static int cvm_validate_keylen(struct cvm_enc_ctx *ctx, u32 keylen)
>  static int cvm_setkey(struct crypto_skcipher *cipher, const u8 *key,
>  		      u32 keylen, u8 cipher_type)
>  {
> -	struct crypto_tfm *tfm = crypto_skcipher_tfm(cipher);
> -	struct cvm_enc_ctx *ctx = crypto_tfm_ctx(tfm);
> +	struct cvm_enc_ctx *ctx = crypto_skcipher_ctx(cipher);
>  
>  	ctx->cipher_type = cipher_type;
>  	if (!cvm_validate_keylen(ctx, keylen)) {

This change here and ...


> diff --git a/drivers/crypto/cavium/nitrox/nitrox_skcipher.c b/drivers/crypto/cavium/nitrox/nitrox_skcipher.c
> index 248b4fff1c72..138261dcd032 100644
> --- a/drivers/crypto/cavium/nitrox/nitrox_skcipher.c
> +++ b/drivers/crypto/cavium/nitrox/nitrox_skcipher.c

<snip>

> @@ -362,8 +361,7 @@ static int nitrox_aes_xts_setkey(struct crypto_skcipher *cipher,
>  static int nitrox_aes_ctr_rfc3686_setkey(struct crypto_skcipher *cipher,
>  					 const u8 *key, unsigned int keylen)
>  {
> -	struct crypto_tfm *tfm = crypto_skcipher_tfm(cipher);
> -	struct nitrox_crypto_ctx *nctx = crypto_tfm_ctx(tfm);
> +	struct nitrox_crypto_ctx *nctx = crypto_skcipher_ctx(cipher);
>  	struct flexi_crypto_context *fctx;
>  	int aes_keylen;
>

this one are not strictly related to xts_verify_key() AFAICS, but it's
fine to have them included nonetheless, IMO.


-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
(HRB 36809, AG Nürnberg)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes
  2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
                   ` (5 preceding siblings ...)
  2022-12-29 21:17 ` [PATCH v3 6/6] crypto: testmgr - allow ecdsa-nist-p256 and -p384 " Vladis Dronov
@ 2023-01-06 15:18 ` Herbert Xu
  6 siblings, 0 replies; 10+ messages in thread
From: Herbert Xu @ 2023-01-06 15:18 UTC (permalink / raw)
  To: Vladis Dronov
  Cc: David S . Miller, Nicolai Stange, Elliott Robert,
	Stephan Mueller, Eric Biggers, linux-crypto, linux-kernel

On Thu, Dec 29, 2022 at 10:17:04PM +0100, Vladis Dronov wrote:
> Hi,
> 
> This patchset and cover letter was initially composed by Nicolai Stange
> and sent earlier as:
> 
> https://lore.kernel.org/r/20221108142025.13461-1-nstange@suse.de/
> with a subject: [PATCH 0/4] Trivial set of FIPS 140-3 related changes
> 
> I'm adding patches 2 and 3 which (I hope) resolve issues spotted by
> reviewers of previous version of the patchset. This new patchset should
> ease our future kernel work on the FIPS mode.
> 
> I'm quoting Nicolai's cover letter here:
> 
> > Hi all,
> > 
> > these four rather unrelated patches are basically a dump of some of the
> > more trivial changes required for working towards FIPS 140-3 conformance.
> > 
> > Please pick as you deem appropriate.
> > 
> > Thanks!
> > 
> > Nicolai
> 
> v2: fixed a block comment formatting
> 
> v3: "Reviewed-by: Eric Biggers" was copied from the v1 thread:
>     https://lore.kernel.org/r/Y6OXuT95MlkNanSR@sol.localdomain/
> 
> Nicolai Stange (4):
>   crypto: xts - restrict key lengths to approved values in FIPS mode
>   crypto: testmgr - disallow plain cbcmac(aes) in FIPS mode
>   crypto: testmgr - disallow plain ghash in FIPS mode
>   crypto: testmgr - allow ecdsa-nist-p256 and -p384 in FIPS mode
> 
> Vladis Dronov (2):
>   crypto: xts - drop xts_check_key()
>   crypto: xts - drop redundant xts key check
> 
>  arch/s390/crypto/aes_s390.c                   |  4 ---
>  arch/s390/crypto/paes_s390.c                  |  2 +-
>  crypto/testmgr.c                              |  4 +--
>  drivers/crypto/atmel-aes.c                    |  2 +-
>  drivers/crypto/axis/artpec6_crypto.c          |  2 +-
>  drivers/crypto/cavium/cpt/cptvf_algs.c        |  8 +++---
>  .../crypto/cavium/nitrox/nitrox_skcipher.c    |  8 +++---
>  drivers/crypto/ccree/cc_cipher.c              |  2 +-
>  .../crypto/marvell/octeontx/otx_cptvf_algs.c  |  2 +-
>  .../marvell/octeontx2/otx2_cptvf_algs.c       |  2 +-
>  include/crypto/xts.h                          | 25 +++++++------------
>  11 files changed, 23 insertions(+), 38 deletions(-)
> 
> base-commit: b6bb9676f2165d518b35ba3bea5f1fcfc0d969bf
> -- 
> 2.38.1

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-01-06 15:20 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-29 21:17 [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Vladis Dronov
2022-12-29 21:17 ` [PATCH v3 1/6] crypto: xts - restrict key lengths to approved values in FIPS mode Vladis Dronov
2022-12-29 21:17 ` [PATCH v3 2/6] crypto: xts - drop xts_check_key() Vladis Dronov
2023-01-05 11:27   ` Nicolai Stange
2022-12-29 21:17 ` [PATCH v3 3/6] crypto: xts - drop redundant xts key check Vladis Dronov
2023-01-05 11:16   ` Nicolai Stange
2022-12-29 21:17 ` [PATCH v3 4/6] crypto: testmgr - disallow plain cbcmac(aes) in FIPS mode Vladis Dronov
2022-12-29 21:17 ` [PATCH v3 5/6] crypto: testmgr - disallow plain ghash " Vladis Dronov
2022-12-29 21:17 ` [PATCH v3 6/6] crypto: testmgr - allow ecdsa-nist-p256 and -p384 " Vladis Dronov
2023-01-06 15:18 ` [PATCH v3 0/6] Trivial set of FIPS 140-3 related changes Herbert Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.