crypto: qce - fix self-test issues in skcipher

This iteration will hopefully fix the final remaining comment from sashiko.

---
Changes in v12:
- Also reject messages shorter than a full block for AES-XTS
- Link to v11: https://patch.msgid.link/[email protected]

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 messages shorter than a block 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 | 41 +++++++++++++++++++++++++++++++++--------
 2 files changed, 34 insertions(+), 8 deletions(-)

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

Best regards,

Bartosz Golaszewski <[email protected]>

XTS is not defined for a payload shorter than one cipher block. The driver treated a zero-length request as a successful no-op, so the crypto self-tests "unexpectedly succeeded" when -EINVAL was expected. Requests of 1 to 15 bytes are not rejected either and, when they are not diverted to the fallback (e.g. with aes_sw_max_len set to 0), reach the engine which stalls waiting for a full block of input.

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

---
 drivers/crypto/qce/skcipher.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 03a3cec2bb90e8d8c17e491ca40ec740a003d1b4..d9a5b255015341e741515bdfd6ff54bd3b24f1e3 100644
@@ -221,6 +221,14 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
 	rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT;
 	keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen;
 
+	/*
+	 * XTS requires at least one full block of data. Reject anything
+	 * shorter here as the engine would stall waiting for more input if
+	 * such a request reached it.
+	 */
+	if (IS_XTS(rctx->flags) && req->cryptlen < AES_BLOCK_SIZE)
+		return -EINVAL;
+
 	/* CE does not handle 0 length messages */
 	if (!req->cryptlen)
 		return 0;

2.47.3

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.

---
 drivers/crypto/qce/skcipher.c | 3 +++
 1 file changed, 3 insertions(+)

index d9a5b255015341e741515bdfd6ff54bd3b24f1e3..8d4cd1d08c0e9d78f044057ecb45e5aa8b9a40a3 100644
@@ -248,9 +248,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))))) {

2.47.3

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.

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

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;
 };
 

index 8d4cd1d08c0e9d78f044057ecb45e5aa8b9a40a3..6a8d642c40a36974231bc4aa2717a5378f17134c 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;

@@ -241,6 +241,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))

@@ -250,13 +255,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,

2.47.3

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.

---
 drivers/crypto/qce/skcipher.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

index 6a8d642c40a36974231bc4aa2717a5378f17134c..79f764429edc469a4f06319b0113afefc96a5ae1 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;
 	}

@@ -255,6 +259,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).
 	 */

2.47.3

Phew, no comments from sashiko finally.

Bart

All applied. Thanks.

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt