Skip to contentSkip to conversation

crypto: qce - fix self-test issues in skcipher

PATCH Superseded Bartosz Golaszewski 1 participant 5 messages Oct 16

Subsystems (4)

v11 of 12 Superseded Latest is v12 →

lore
Share & tools
lore
#0 lore raw

v11 cover crypto: qce - fix self-test issues in skcipher

As requested by Herbert: this is the first of smaller series wits fixes for QCE. This one contains the patches addressing issues in skcipher.c.

---
Changes in v11:
- Split out the skcipher patches into a smaller series for easier review
- Drop patch "crypto: qce - Fix CTR-AES for partial block requests" as a
subsequent commit makes this adjustment effectively dead code
- Fix a mid-series build failure due to an earlier bad rebase
- Link to v10: https://patch.msgid.link/[email protected]

Changes in v10:
- don't pass a NULL buffer to sg_init_one()
- fix byte order of the imported hash state on empty finalization
- don't recompute the CTR counter in software for every request
- reject skcipher requests with a too short scatterlist
- check fragmentation for empty CCM decryption requests too
- don't use a static loop iterator (duh...)
- initialize the return value in qce_handle_queue()
- Link to v9: https://patch.msgid.link/[email protected]

Changes in v9:
- Rebase on top of cryptodev/master
- Rephrase Kconfig help text changes
- Link to v8: https://patch.msgid.link/[email protected]

Changes in v8:
- Add a patch making the crypto driver *not* register algos by default
but only if the user explicitly requests it with the provided module
parameter
- Use the "offloader" name for the crypto IP instead of "accelerator"
- Fix device module table issues
- Make the core driver use a list (despite it only having a single
member now) of child auxiliary devices to register to make it clear we
have the intention of extending this once we start the work on secure
media playback
- Link to v7: https://patch.msgid.link/[email protected]

Changes in v7:
- Add follow-up changes converting the QCE crypto driver to auxiliary
bus, adding the core QCE driver under drivers/soc/ registering the
auxiliary device and removing the BROKEN Kconfig label
- Link to v6: https://patch.msgid.link/[email protected]

Changes in v6:
- Handle all zero-length HMAC finalizations (like imported state with
data already hashed), not only the genuinely empty message case
- Add an additional patch addressing the fragmented skcipher payload
issue
- Link to v5: https://patch.msgid.link/[email protected]

Changes in v5:
- Dropped patch 1/8 that's already queued
- Use the pre-allocated fallback ahash for HMAC transforms (Herbert)
- Link to v4: https://patch.msgid.link/[email protected]

Changes in v4:
- Remove remaining ECB and DES3 bits
- Pick up tags
- Link to v3: https://patch.msgid.link/[email protected]

Changes in v3:
- Remove even more algorithms and dead code in patch 1/8
- Link to v2: https://patch.msgid.link/[email protected]

Changes in v2:
- Add fixes for the full suite of crypto self-tests
- Add Fixes and Cc tags
- Link to v1: https://patch.msgid.link/[email protected]/

---
Bartosz Golaszewski (3):
crypto: qce - Reject empty messages for AES-XTS
crypto: qce - Use a fallback for AES-CTR with a partial final block
crypto: qce - Use fallback for fragmented skcipher payloads

Kuldeep Singh (1):
crypto: qce - Fix xts-aes-qce for weak keys

 drivers/crypto/qce/cipher.h   |  1 +
 drivers/crypto/qce/skcipher.c | 39 ++++++++++++++++++++++++++++++---------
 2 files changed, 31 insertions(+), 9 deletions(-)

---
base-commit: f0f20b900bbb8bdcfaf6cb282787854067d8eec8
change-id: 20261016-qce-fixes-skcipher-e0384a650e25

Best regards,

Signature · 1 line
Bartosz Golaszewski <[email protected]>
#1 lore raw

PATCH v11 1/4 crypto: qce - Reject empty messages for AES-XTS

XTS is not defined for an empty plaintext: it requires at least one full block of data. The driver treated a zero-length request as a successful no-op, so the crypto self-tests "unexpectedly succeeded" when -EINVAL was expected.

Return -EINVAL for empty XTS requests while keeping the no-op behavior for the other ciphers, which the crypto engine simply cannot process due to its DMA not supporting zero-length transfers.

Fixes f08789462255 ("crypto: qce - Return error for zero length messages")
---
 drivers/crypto/qce/skcipher.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/crypto/qce/skcipher.c +5 -1
index 03a3cec2bb90e8d8c17e491ca40ec740a003d1b4..f177062eb735148561f798d0d3e65265f3235da8 100644
@@ -222,8 +222,12 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
 	keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen;
 
 	/* CE does not handle 0 length messages */
-	if (!req->cryptlen)
+	if (!req->cryptlen) {
+		/* XTS requires at least one full block of data */
+		if (IS_XTS(rctx->flags))
+			return -EINVAL;
 		return 0;
+	}
 
 	/*
 	 * ECB and CBC algorithms require message lengths to be
Signature · 1 line
2.47.3
#2 lore raw

PATCH v11 2/4 crypto: qce - Use a fallback for AES-CTR with a partial final block

In reply to Bartosz Golaszewski

ctr(aes) is registered with a block size of 1, so the crypto API hands the driver requests whose length is not a multiple of the AES block size. The crypto engine, however, stalls waiting for a full block of input in that case, leaving the operation incomplete and failing the request (and the crypto self-tests) with a hardware operation error.

Route AES-CTR requests with a partial final block to the software fallback, which already handles the other cases the engine cannot.

Fixes bb5c863b3d3c ("crypto: qce - fix ctr-aes-qce block, chunk sizes")
---
 drivers/crypto/qce/skcipher.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/crypto/qce/skcipher.c +3 -0
index f177062eb735148561f798d0d3e65265f3235da8..c23ebc61873f1c542a4b475904c3c3a3d89bcc35 100644
@@ -244,9 +244,12 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
 	 * AES-XTS request with len > QCE_SECTOR_SIZE and
 	 * is not a multiple of it.(Revisit this condition to check if it is
 	 * needed in all versions of CE)
+	 * AES-CTR with a partial final block (the CE stalls waiting for a full
+	 * block of input).
 	 */
 	if (IS_AES(rctx->flags) &&
 	    ((keylen != AES_KEYSIZE_128 && keylen != AES_KEYSIZE_256) ||
+	    (IS_CTR(rctx->flags) && !IS_ALIGNED(req->cryptlen, AES_BLOCK_SIZE)) ||
 	    (IS_XTS(rctx->flags) && ((req->cryptlen <= aes_sw_max_len) ||
 	    (req->cryptlen > QCE_SECTOR_SIZE &&
 	    req->cryptlen % QCE_SECTOR_SIZE))))) {
Signature · 1 line
2.47.3
#3 lore raw

PATCH v11 3/4 crypto: qce - Use fallback for fragmented skcipher payloads

In reply to Bartosz Golaszewski

The crypto engine reliably processes AES requests only when the payload is a single contiguous buffer. A payload split across multiple scatterlist entries makes the engine stall waiting for input, failing the request with a hardware operation error. This was uncovered by the crypto self-tests, which feed the algorithms randomly fragmented buffers.

Detect a payload that spans more than one scatterlist entry, in either the source or the destination, and route the request to the software fallback.

Fixes 25b71d61d631 ("crypto: qce - Improve the conditions for requesting AES fallback cipher")
---
 drivers/crypto/qce/cipher.h   |  1 +
 drivers/crypto/qce/skcipher.c | 13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)
drivers/crypto/qce/cipher.h +1 -0
index 850f257d00f3aca0397adc1f703aea690c754d60..daea07551118d444d2f749588bdfe2ae2c6c553f 100644
@@ -14,6 +14,7 @@
 struct qce_cipher_ctx {
 	u8 enc_key[QCE_MAX_KEY_SIZE];
 	unsigned int enc_keylen;
+	bool use_fallback;
 	struct crypto_skcipher *fallback;
 };
 
drivers/crypto/qce/skcipher.c +11 -2
index c23ebc61873f1c542a4b475904c3c3a3d89bcc35..0473033db2af90e119b731b9a6ec96de5973d3c8 100644
@@ -215,7 +215,7 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
 	struct qce_cipher_reqctx *rctx = skcipher_request_ctx(req);
 	struct qce_alg_template *tmpl = to_cipher_tmpl(tfm);
 	unsigned int blocksize = crypto_skcipher_blocksize(tfm);
-	int keylen;
+	int keylen, src_nents, dst_nents;
 
 	rctx->flags = tmpl->alg_flags;
 	rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT;
@@ -237,6 +237,11 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
 		if (!IS_ALIGNED(req->cryptlen, blocksize))
 			return -EINVAL;
 
+	src_nents = sg_nents_for_len(req->src, req->cryptlen);
+	dst_nents = sg_nents_for_len(req->dst, req->cryptlen);
+	if (src_nents < 0 || dst_nents < 0)
+		return -EINVAL;
+
 	/*
 	 * Conditions for requesting a fallback cipher
 	 * AES-192 (not supported by crypto engine (CE))
@@ -246,13 +251,17 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
 	 * needed in all versions of CE)
 	 * AES-CTR with a partial final block (the CE stalls waiting for a full
 	 * block of input).
+	 * A payload fragmented across more than one scatterlist entry (the CE
+	 * stalls waiting for input in that case too).
 	 */
 	if (IS_AES(rctx->flags) &&
 	    ((keylen != AES_KEYSIZE_128 && keylen != AES_KEYSIZE_256) ||
 	    (IS_CTR(rctx->flags) && !IS_ALIGNED(req->cryptlen, AES_BLOCK_SIZE)) ||
 	    (IS_XTS(rctx->flags) && ((req->cryptlen <= aes_sw_max_len) ||
 	    (req->cryptlen > QCE_SECTOR_SIZE &&
-	    req->cryptlen % QCE_SECTOR_SIZE))))) {
+	    req->cryptlen % QCE_SECTOR_SIZE))) ||
+	    (IS_XTS(rctx->flags) && ctx->use_fallback) ||
+	    src_nents > 1 || dst_nents > 1)) {
 		skcipher_request_set_tfm(&rctx->fallback_req, ctx->fallback);
 		skcipher_request_set_callback(&rctx->fallback_req,
 					      req->base.flags,
Signature · 1 line
2.47.3
#4 lore raw

PATCH v11 4/4 crypto: qce - Fix xts-aes-qce for weak keys

In reply to Bartosz Golaszewski

From: Kuldeep Singh <[email protected]>

The QCE hardware does not support AES XTS mode when key1 and key2 are equal. The driver was handling this by unconditionally rejecting the keys with -ENOKEY(-126), regardless of whether FIPS mode is active or the FORBID_WEAK_KEYS flag is set.

[    5.599170] alg: skcipher: xts-aes-qce setkey failed on test vector 0; expected_error=0, actual_error=-126, flags=0x1
[    5.599184] alg: self-tests for xts(aes) using xts-aes-qce failed (rc=-126)

In general for weak keys,
- If FIPS mode is active or FORBID_WEAK_KEYS is set: return -EINVAL.
- In non-FIPS mode, Accept the key and encrypt successfully.

Since QCE was returning -ENOKEY for non-FIPS mode whereas the expectation is to encrypt content and return success, the selftest saw a mismatch and failed.

There are two problems in QCE behavior:
* -ENOKEY is returned instead of -EINVAL for the FIPS/weak-key rejection case.
* key1 == key2 is rejected even in non-FIPS mode

Fix xts-aes-qce behavior by using generic helper xts_verify_key() to reject keys early with -EINVAL for FIPS mode active(or FORBID_WEAK_KEYS set). For non-FIPS mode, since QCE hardware cannot accept the keys, use software fallback mechanism to encrypt the data.

Fixes f0d078dd6c49 ("crypto: qce - Return unsupported if key1 and key 2 are same for AES XTS algorithm")
---
 drivers/crypto/qce/skcipher.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/crypto/qce/skcipher.c +11 -6
index 0473033db2af90e119b731b9a6ec96de5973d3c8..03492b2fbbd4e22fcfe391cd110dec6cd65a9c48 100644
@@ -12,6 +12,7 @@
 #include <linux/errno.h>
 #include <crypto/aes.h>
 #include <crypto/internal/skcipher.h>
+#include <crypto/xts.h>
 
 #include "cipher.h"
 
@@ -179,14 +180,17 @@ static int qce_skcipher_setkey(struct crypto_skcipher *ablk, const u8 *key,
 	if (!key || !keylen)
 		return -EINVAL;
 
-	/*
-	 * AES XTS key1 = key2 not supported by crypto engine.
-	 * Revisit to request a fallback cipher in this case.
-	 */
 	if (IS_XTS(flags)) {
+		ret = xts_verify_key(ablk, key, keylen);
+		if (ret)
+			return ret;
 		__keylen = keylen >> 1;
-		if (!memcmp(key, key + __keylen, __keylen))
-			return -ENOKEY;
+		/*
+		 * QCE does not support key1 == key2 for XTS.
+		 * Use fallback cipher in this case.
+		 */
+		ctx->use_fallback = !crypto_memneq(key, key + __keylen,
+						       __keylen);
 	} else {
 		__keylen = keylen;
 	}
@@ -251,6 +255,7 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
 	 * needed in all versions of CE)
 	 * AES-CTR with a partial final block (the CE stalls waiting for a full
 	 * block of input).
+	 * AES-XTS with key1 == key2 (not supported by the CE).
 	 * A payload fragmented across more than one scatterlist entry (the CE
 	 * stalls waiting for input in that case too).
 	 */
Signature · 1 line
2.47.3