This patch series is a result of running kernel crypto fuzz tests (by
enabling CONFIG_CRYPTO_MANAGER_EXTRA_TESTS) on the transformations
currently supported via the Qualcomm crypto engine on sdm845. The first
nine patches are fixes for various regressions found during testing. The
last two patches are minor clean ups of unused variable and parameters.
v4->v5:
- Fixed build warning/error in patch for wrong assignment of const
pointer as reported by kernel test robot <[email protected]>.
- Rebased to 5.11-rc6.
v3->v4:
- Fixed the bug where only two bytes of byte_count were getting
saved and restored instead of all eight bytes. Thanks Bjorn for
catching this.
- Split patch 3 "Fix regressions found during fuzz testing" into
6 patches as requested by Bjorn.
- Dropped crypto from all subject headers.
- Rebased to 5.11-rc5
v2->v3:
- Made the comparison between keys to check if any two keys are
same for triple des algorithms constant-time as per
Nym Seddon's suggestion.
- Rebased to 5.11-rc4.
v1->v2:
- Introduced custom struct qce_sha_saved_state to store and restore
partial sha transformation.
- Rebased to 5.11-rc3.
Thara Gopinath (11):
crypto: qce: sha: Restore/save ahash state with custom struct in
export/import
crypto: qce: sha: Hold back a block of data to be transferred as part
of final
crypto: qce: skcipher: Return unsupported if key1 and key 2 are same
for AES XTS algorithm
crypto: qce: skcipher: Return unsupported if any three keys are same
for DES3 algorithms
crypto: qce: skcipher: Return error for zero length messages
crypto: qce: skcipher: Return error for non-blocksize data(ECB/CBC
algorithms)
crypto: qce: skcipher: Set ivsize to 0 for ecb(aes)
crypto: qce: skcipher: Improve the conditions for requesting AES
fallback cipher
crypto: qce: common: Set data unit size to message length for AES XTS
transformation
crypto: qce: Remover src_tbl from qce_cipher_reqctx
crypto: qce: Remove totallen and offset in qce_start
drivers/crypto/qce/cipher.h | 1 -
drivers/crypto/qce/common.c | 25 +++---
drivers/crypto/qce/common.h | 3 +-
drivers/crypto/qce/sha.c | 143 +++++++++++++---------------------
drivers/crypto/qce/skcipher.c | 72 ++++++++++++++---
5 files changed, 129 insertions(+), 115 deletions(-)
--
2.25.1
Crypto engine BAM dma does not support 0 length data. Return unsupported
if zero length messages are passed for transformation.
Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/crypto/qce/skcipher.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index de1f37ed4ee6..331b3c3a5b59 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -8,6 +8,7 @@
#include <linux/interrupt.h>
#include <linux/moduleparam.h>
#include <linux/types.h>
+#include <linux/errno.h>
#include <crypto/aes.h>
#include <crypto/internal/des.h>
#include <crypto/internal/skcipher.h>
@@ -260,6 +261,10 @@ 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;
+ /* CE does not handle 0 length messages */
+ if (!req->cryptlen)
+ return -EOPNOTSUPP;
+
/* qce is hanging when AES-XTS request len > QCE_SECTOR_SIZE and
* is not a multiple of it; pass such requests to the fallback
*/
--
2.25.1
Export and import interfaces save and restore partial transformation
states. The partial states were being stored and restored in struct
sha1_state for sha1/hmac(sha1) transformations and sha256_state for
sha256/hmac(sha256) transformations.This led to a bunch of corner cases
where improper state was being stored and restored. A few of the corner
cases that turned up during testing are:
- wrong byte_count restored if export/import is called twice without h/w
transaction in between
- wrong buflen restored back if the pending buffer
length is exactly the block size.
- wrong state restored if buffer length is 0.
To fix these issues, save and restore the partial transformation state
using the newly introduced qce_sha_saved_state struct. This ensures that
all the pieces required to properly restart the transformation is captured
and restored back
Signed-off-by: Thara Gopinath <[email protected]>
---
v4->v5:
- Fixed build warning/error in patch for wrong assignment of const
pointer as reported by kernel test robot <[email protected]>.
v1->v2:
- Introduced custom struct qce_sha_saved_state to store and
restore partial sha transformation. v1 was re-using
qce_sha_reqctx to save and restore partial states and this
could lead to potential memcpy issues around pointer copying.
drivers/crypto/qce/sha.c | 122 +++++++++++----------------------------
1 file changed, 34 insertions(+), 88 deletions(-)
diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
index 61c418c12345..7da562dca740 100644
--- a/drivers/crypto/qce/sha.c
+++ b/drivers/crypto/qce/sha.c
@@ -12,9 +12,15 @@
#include "core.h"
#include "sha.h"
-/* crypto hw padding constant for first operation */
-#define SHA_PADDING 64
-#define SHA_PADDING_MASK (SHA_PADDING - 1)
+struct qce_sha_saved_state {
+ u8 pending_buf[QCE_SHA_MAX_BLOCKSIZE];
+ u8 partial_digest[QCE_SHA_MAX_DIGESTSIZE];
+ __be32 byte_count[2];
+ unsigned int pending_buflen;
+ unsigned int flags;
+ u64 count;
+ bool first_blk;
+};
static LIST_HEAD(ahash_algs);
@@ -139,97 +145,37 @@ static int qce_ahash_init(struct ahash_request *req)
static int qce_ahash_export(struct ahash_request *req, void *out)
{
- struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
struct qce_sha_reqctx *rctx = ahash_request_ctx(req);
- unsigned long flags = rctx->flags;
- unsigned int digestsize = crypto_ahash_digestsize(ahash);
- unsigned int blocksize =
- crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash));
-
- if (IS_SHA1(flags) || IS_SHA1_HMAC(flags)) {
- struct sha1_state *out_state = out;
-
- out_state->count = rctx->count;
- qce_cpu_to_be32p_array((__be32 *)out_state->state,
- rctx->digest, digestsize);
- memcpy(out_state->buffer, rctx->buf, blocksize);
- } else if (IS_SHA256(flags) || IS_SHA256_HMAC(flags)) {
- struct sha256_state *out_state = out;
-
- out_state->count = rctx->count;
- qce_cpu_to_be32p_array((__be32 *)out_state->state,
- rctx->digest, digestsize);
- memcpy(out_state->buf, rctx->buf, blocksize);
- } else {
- return -EINVAL;
- }
-
- return 0;
-}
-
-static int qce_import_common(struct ahash_request *req, u64 in_count,
- const u32 *state, const u8 *buffer, bool hmac)
-{
- struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
- struct qce_sha_reqctx *rctx = ahash_request_ctx(req);
- unsigned int digestsize = crypto_ahash_digestsize(ahash);
- unsigned int blocksize;
- u64 count = in_count;
-
- blocksize = crypto_tfm_alg_blocksize(crypto_ahash_tfm(ahash));
- rctx->count = in_count;
- memcpy(rctx->buf, buffer, blocksize);
-
- if (in_count <= blocksize) {
- rctx->first_blk = 1;
- } else {
- rctx->first_blk = 0;
- /*
- * For HMAC, there is a hardware padding done when first block
- * is set. Therefore the byte_count must be incremened by 64
- * after the first block operation.
- */
- if (hmac)
- count += SHA_PADDING;
- }
+ struct qce_sha_saved_state *export_state = out;
- rctx->byte_count[0] = (__force __be32)(count & ~SHA_PADDING_MASK);
- rctx->byte_count[1] = (__force __be32)(count >> 32);
- qce_cpu_to_be32p_array((__be32 *)rctx->digest, (const u8 *)state,
- digestsize);
- rctx->buflen = (unsigned int)(in_count & (blocksize - 1));
+ memcpy(export_state->pending_buf, rctx->buf, rctx->buflen);
+ memcpy(export_state->partial_digest, rctx->digest, sizeof(rctx->digest));
+ export_state->byte_count[0] = rctx->byte_count[0];
+ export_state->byte_count[1] = rctx->byte_count[1];
+ export_state->pending_buflen = rctx->buflen;
+ export_state->count = rctx->count;
+ export_state->first_blk = rctx->first_blk;
+ export_state->flags = rctx->flags;
return 0;
}
static int qce_ahash_import(struct ahash_request *req, const void *in)
{
- struct qce_sha_reqctx *rctx;
- unsigned long flags;
- bool hmac;
- int ret;
-
- ret = qce_ahash_init(req);
- if (ret)
- return ret;
-
- rctx = ahash_request_ctx(req);
- flags = rctx->flags;
- hmac = IS_SHA_HMAC(flags);
-
- if (IS_SHA1(flags) || IS_SHA1_HMAC(flags)) {
- const struct sha1_state *state = in;
-
- ret = qce_import_common(req, state->count, state->state,
- state->buffer, hmac);
- } else if (IS_SHA256(flags) || IS_SHA256_HMAC(flags)) {
- const struct sha256_state *state = in;
+ struct qce_sha_reqctx *rctx = ahash_request_ctx(req);
+ const struct qce_sha_saved_state *import_state = in;
- ret = qce_import_common(req, state->count, state->state,
- state->buf, hmac);
- }
+ memset(rctx, 0, sizeof(*rctx));
+ rctx->count = import_state->count;
+ rctx->buflen = import_state->pending_buflen;
+ rctx->first_blk = import_state->first_blk;
+ rctx->flags = import_state->flags;
+ rctx->byte_count[0] = import_state->byte_count[0];
+ rctx->byte_count[1] = import_state->byte_count[1];
+ memcpy(rctx->buf, import_state->pending_buf, rctx->buflen);
+ memcpy(rctx->digest, import_state->partial_digest, sizeof(rctx->digest));
- return ret;
+ return 0;
}
static int qce_ahash_update(struct ahash_request *req)
@@ -450,7 +396,7 @@ static const struct qce_ahash_def ahash_def[] = {
.drv_name = "sha1-qce",
.digestsize = SHA1_DIGEST_SIZE,
.blocksize = SHA1_BLOCK_SIZE,
- .statesize = sizeof(struct sha1_state),
+ .statesize = sizeof(struct qce_sha_saved_state),
.std_iv = std_iv_sha1,
},
{
@@ -459,7 +405,7 @@ static const struct qce_ahash_def ahash_def[] = {
.drv_name = "sha256-qce",
.digestsize = SHA256_DIGEST_SIZE,
.blocksize = SHA256_BLOCK_SIZE,
- .statesize = sizeof(struct sha256_state),
+ .statesize = sizeof(struct qce_sha_saved_state),
.std_iv = std_iv_sha256,
},
{
@@ -468,7 +414,7 @@ static const struct qce_ahash_def ahash_def[] = {
.drv_name = "hmac-sha1-qce",
.digestsize = SHA1_DIGEST_SIZE,
.blocksize = SHA1_BLOCK_SIZE,
- .statesize = sizeof(struct sha1_state),
+ .statesize = sizeof(struct qce_sha_saved_state),
.std_iv = std_iv_sha1,
},
{
@@ -477,7 +423,7 @@ static const struct qce_ahash_def ahash_def[] = {
.drv_name = "hmac-sha256-qce",
.digestsize = SHA256_DIGEST_SIZE,
.blocksize = SHA256_BLOCK_SIZE,
- .statesize = sizeof(struct sha256_state),
+ .statesize = sizeof(struct qce_sha_saved_state),
.std_iv = std_iv_sha256,
},
};
--
2.25.1
The following are the conditions for requesting AES fallback cipher.
- AES-192
- AES-XTS request with len <= 512 byte (Allow messages of length
less than 512 bytes for all other AES encryption algorithms other
than AES XTS)
- AES-XTS request with len > QCE_SECTOR_SIZE and is not a multiple
of it
Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/crypto/qce/skcipher.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index 10e85b1fc0fd..8599250946b7 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -277,14 +277,19 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
if (!IS_ALIGNED(req->cryptlen, blocksize))
return -EINVAL;
- /* qce is hanging when AES-XTS request len > QCE_SECTOR_SIZE and
- * is not a multiple of it; pass such requests to the fallback
+ /*
+ * Conditions for requesting a fallback cipher
+ * AES-192 (not supported by crypto engine (CE))
+ * AES-XTS request with len <= 512 byte (not recommended to use CE)
+ * 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)
*/
if (IS_AES(rctx->flags) &&
- (((keylen != AES_KEYSIZE_128 && keylen != AES_KEYSIZE_256) ||
- req->cryptlen <= aes_sw_max_len) ||
- (IS_XTS(rctx->flags) && req->cryptlen > QCE_SECTOR_SIZE &&
- req->cryptlen % QCE_SECTOR_SIZE))) {
+ ((keylen != AES_KEYSIZE_128 && keylen != AES_KEYSIZE_256) ||
+ (IS_XTS(rctx->flags) && ((req->cryptlen <= aes_sw_max_len) ||
+ (req->cryptlen > QCE_SECTOR_SIZE &&
+ req->cryptlen % QCE_SECTOR_SIZE))))) {
skcipher_request_set_tfm(&rctx->fallback_req, ctx->fallback);
skcipher_request_set_callback(&rctx->fallback_req,
req->base.flags,
--
2.25.1
totallen is used to get the size of the data to be transformed.
This is also available via nbytes or cryptlen in the qce_sha_reqctx
and qce_cipher_ctx. Similarly offset convey nothing for the supported
encryption and authentication transformations and is always 0.
Remove these two redundant parameters in qce_start.
Reviewed-by: Bjorn Andersson <[email protected]>
Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/crypto/qce/common.c | 17 +++++++----------
drivers/crypto/qce/common.h | 3 +--
drivers/crypto/qce/sha.c | 2 +-
drivers/crypto/qce/skcipher.c | 2 +-
4 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
index f7bc701a4aa2..dceb9579d87a 100644
--- a/drivers/crypto/qce/common.c
+++ b/drivers/crypto/qce/common.c
@@ -140,8 +140,7 @@ static u32 qce_auth_cfg(unsigned long flags, u32 key_size)
return cfg;
}
-static int qce_setup_regs_ahash(struct crypto_async_request *async_req,
- u32 totallen, u32 offset)
+static int qce_setup_regs_ahash(struct crypto_async_request *async_req)
{
struct ahash_request *req = ahash_request_cast(async_req);
struct crypto_ahash *ahash = __crypto_ahash_cast(async_req->tfm);
@@ -306,8 +305,7 @@ static void qce_xtskey(struct qce_device *qce, const u8 *enckey,
qce_write(qce, REG_ENCR_XTS_DU_SIZE, cryptlen);
}
-static int qce_setup_regs_skcipher(struct crypto_async_request *async_req,
- u32 totallen, u32 offset)
+static int qce_setup_regs_skcipher(struct crypto_async_request *async_req)
{
struct skcipher_request *req = skcipher_request_cast(async_req);
struct qce_cipher_reqctx *rctx = skcipher_request_ctx(req);
@@ -367,7 +365,7 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req,
qce_write(qce, REG_ENCR_SEG_CFG, encr_cfg);
qce_write(qce, REG_ENCR_SEG_SIZE, rctx->cryptlen);
- qce_write(qce, REG_ENCR_SEG_START, offset & 0xffff);
+ qce_write(qce, REG_ENCR_SEG_START, 0);
if (IS_CTR(flags)) {
qce_write(qce, REG_CNTR_MASK, ~0);
@@ -376,7 +374,7 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req,
qce_write(qce, REG_CNTR_MASK2, ~0);
}
- qce_write(qce, REG_SEG_SIZE, totallen);
+ qce_write(qce, REG_SEG_SIZE, rctx->cryptlen);
/* get little endianness */
config = qce_config_reg(qce, 1);
@@ -388,17 +386,16 @@ static int qce_setup_regs_skcipher(struct crypto_async_request *async_req,
}
#endif
-int qce_start(struct crypto_async_request *async_req, u32 type, u32 totallen,
- u32 offset)
+int qce_start(struct crypto_async_request *async_req, u32 type)
{
switch (type) {
#ifdef CONFIG_CRYPTO_DEV_QCE_SKCIPHER
case CRYPTO_ALG_TYPE_SKCIPHER:
- return qce_setup_regs_skcipher(async_req, totallen, offset);
+ return qce_setup_regs_skcipher(async_req);
#endif
#ifdef CONFIG_CRYPTO_DEV_QCE_SHA
case CRYPTO_ALG_TYPE_AHASH:
- return qce_setup_regs_ahash(async_req, totallen, offset);
+ return qce_setup_regs_ahash(async_req);
#endif
default:
return -EINVAL;
diff --git a/drivers/crypto/qce/common.h b/drivers/crypto/qce/common.h
index 85ba16418a04..3bc244bcca2d 100644
--- a/drivers/crypto/qce/common.h
+++ b/drivers/crypto/qce/common.h
@@ -94,7 +94,6 @@ struct qce_alg_template {
void qce_cpu_to_be32p_array(__be32 *dst, const u8 *src, unsigned int len);
int qce_check_status(struct qce_device *qce, u32 *status);
void qce_get_version(struct qce_device *qce, u32 *major, u32 *minor, u32 *step);
-int qce_start(struct crypto_async_request *async_req, u32 type, u32 totallen,
- u32 offset);
+int qce_start(struct crypto_async_request *async_req, u32 type);
#endif /* _COMMON_H_ */
diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
index 2813c9a27a6e..8e6fcf2c21cc 100644
--- a/drivers/crypto/qce/sha.c
+++ b/drivers/crypto/qce/sha.c
@@ -113,7 +113,7 @@ static int qce_ahash_async_req_handle(struct crypto_async_request *async_req)
qce_dma_issue_pending(&qce->dma);
- ret = qce_start(async_req, tmpl->crypto_alg_type, 0, 0);
+ ret = qce_start(async_req, tmpl->crypto_alg_type);
if (ret)
goto error_terminate;
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index 8599250946b7..3fc0b263d498 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -144,7 +144,7 @@ qce_skcipher_async_req_handle(struct crypto_async_request *async_req)
qce_dma_issue_pending(&qce->dma);
- ret = qce_start(async_req, tmpl->crypto_alg_type, req->cryptlen, 0);
+ ret = qce_start(async_req, tmpl->crypto_alg_type);
if (ret)
goto error_terminate;
--
2.25.1
ECB/CBC encryption/decryption requires the data to be blocksize aligned.
Crypto engine hangs on non-block sized operations for these algorithms.
Return invalid data if data size is not blocksize aligned for these
algorithms.
Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/crypto/qce/skcipher.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index 331b3c3a5b59..28bea9584c33 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -254,6 +254,7 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
struct qce_cipher_ctx *ctx = crypto_skcipher_ctx(tfm);
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 ret;
@@ -265,6 +266,17 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt)
if (!req->cryptlen)
return -EOPNOTSUPP;
+ /*
+ * ECB and CBC algorithms require message lengths to be
+ * multiples of block size.
+ * TODO: The spec says AES CBC mode for certain versions
+ * of crypto engine can handle partial blocks as well.
+ * Test and enable such messages.
+ */
+ if (IS_ECB(rctx->flags) || IS_CBC(rctx->flags))
+ if (!IS_ALIGNED(req->cryptlen, blocksize))
+ return -EINVAL;
+
/* qce is hanging when AES-XTS request len > QCE_SECTOR_SIZE and
* is not a multiple of it; pass such requests to the fallback
*/
--
2.25.1
ECB transformations do not have an IV and hence set the ivsize to 0 for
ecb(aes).
Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/crypto/qce/skcipher.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index 28bea9584c33..10e85b1fc0fd 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -356,7 +356,7 @@ static const struct qce_skcipher_def skcipher_def[] = {
.name = "ecb(aes)",
.drv_name = "ecb-aes-qce",
.blocksize = AES_BLOCK_SIZE,
- .ivsize = AES_BLOCK_SIZE,
+ .ivsize = 0,
.min_keysize = AES_MIN_KEY_SIZE,
.max_keysize = AES_MAX_KEY_SIZE,
},
--
2.25.1
Return unsupported if any three keys are same for DES3 algorithms
since CE does not support this and the operation causes the engine to
hang.
Signed-off-by: Thara Gopinath <[email protected]>
---
drivers/crypto/qce/skcipher.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
index 12955dcd53dd..de1f37ed4ee6 100644
--- a/drivers/crypto/qce/skcipher.c
+++ b/drivers/crypto/qce/skcipher.c
@@ -221,12 +221,27 @@ static int qce_des3_setkey(struct crypto_skcipher *ablk, const u8 *key,
unsigned int keylen)
{
struct qce_cipher_ctx *ctx = crypto_skcipher_ctx(ablk);
+ u32 _key[6];
int err;
err = verify_skcipher_des3_key(ablk, key);
if (err)
return err;
+ /*
+ * The crypto engine does not support any two keys
+ * being the same for triple des algorithms. The
+ * verify_skcipher_des3_key does not check for all the
+ * below conditions. Return -ENOKEY in case any two keys
+ * are the same. Revisit to see if a fallback cipher
+ * is needed to handle this condition.
+ */
+ memcpy(_key, key, DES3_EDE_KEY_SIZE);
+ if (!((_key[0] ^ _key[2]) | (_key[1] ^ _key[3])) |
+ !((_key[2] ^ _key[4]) | (_key[3] ^ _key[5])) |
+ !((_key[0] ^ _key[4]) | (_key[1] ^ _key[5])))
+ return -ENOKEY;
+
ctx->enc_keylen = keylen;
memcpy(ctx->enc_key, key, keylen);
return 0;
--
2.25.1
On Thu, Feb 04, 2021 at 04:43:53PM -0500, Thara Gopinath wrote:
> Crypto engine BAM dma does not support 0 length data. Return unsupported
> if zero length messages are passed for transformation.
>
> Signed-off-by: Thara Gopinath <[email protected]>
> ---
> drivers/crypto/qce/skcipher.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
> index de1f37ed4ee6..331b3c3a5b59 100644
> --- a/drivers/crypto/qce/skcipher.c
> +++ b/drivers/crypto/qce/skcipher.c
> @@ -8,6 +8,7 @@
> #include <linux/interrupt.h>
> #include <linux/moduleparam.h>
> #include <linux/types.h>
> +#include <linux/errno.h>
> #include <crypto/aes.h>
> #include <crypto/internal/des.h>
> #include <crypto/internal/skcipher.h>
> @@ -260,6 +261,10 @@ 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;
>
> + /* CE does not handle 0 length messages */
> + if (!req->cryptlen)
> + return -EOPNOTSUPP;
> +
For the algorithms in question, the correct behavior is to return 0.
Aren't the tests catching that difference?
- Eric
On Thu, Feb 04, 2021 at 04:43:54PM -0500, Thara Gopinath wrote:
> + /*
> + * ECB and CBC algorithms require message lengths to be
> + * multiples of block size.
> + * TODO: The spec says AES CBC mode for certain versions
> + * of crypto engine can handle partial blocks as well.
> + * Test and enable such messages.
> + */
> + if (IS_ECB(rctx->flags) || IS_CBC(rctx->flags))
> + if (!IS_ALIGNED(req->cryptlen, blocksize))
> + return -EINVAL;
CBC by definition only operates on full blocks, so the TODO doesn't make sense.
Is the partial block support really CTS-CBC?
- Eric
Hi Eric,
On 2/4/21 5:48 PM, Eric Biggers wrote:
> On Thu, Feb 04, 2021 at 04:43:53PM -0500, Thara Gopinath wrote:
>> Crypto engine BAM dma does not support 0 length data. Return unsupported
>> if zero length messages are passed for transformation.
>>
>> Signed-off-by: Thara Gopinath <[email protected]>
>> ---
>> drivers/crypto/qce/skcipher.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c
>> index de1f37ed4ee6..331b3c3a5b59 100644
>> --- a/drivers/crypto/qce/skcipher.c
>> +++ b/drivers/crypto/qce/skcipher.c
>> @@ -8,6 +8,7 @@
>> #include <linux/interrupt.h>
>> #include <linux/moduleparam.h>
>> #include <linux/types.h>
>> +#include <linux/errno.h>
>> #include <crypto/aes.h>
>> #include <crypto/internal/des.h>
>> #include <crypto/internal/skcipher.h>
>> @@ -260,6 +261,10 @@ 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;
>>
>> + /* CE does not handle 0 length messages */
>> + if (!req->cryptlen)
>> + return -EOPNOTSUPP;
>> +
>
> For the algorithms in question, the correct behavior is to return 0.
What do you mean? The driver should return a 0 ?
>
> Aren't the tests catching that difference?
I was anyways planning on sending an email to the list with these
queries. But since you asked, these are my observations with fuzz
testing which I have been doing quite a bit now (I am also working on
adding a few qualcomm AEAD algorithms support in mainline).
- if the generic algorithm supports 0 length messages and the
transformation I am testing does not, the test framework throws an error
and stops.
- key support mismatch between the generic algorithm vs my algorithm
/engine also does the same thing.For eg, Qualcomm CE engine does not
support any three keys being same for triple des algorithms. Where as a
two key 3des is a valid scenario for generic algorithm(k1=k3). Another
example is hardware engine not supporting AES192.
How are these scenarios usually handled ? Why not allow the test
framework to proceed with the testing if the algorithm does not support
a particular scenario ?
>
> - Eric
>
--
Warm Regards
Thara
On 2/4/21 5:50 PM, Eric Biggers wrote:
> On Thu, Feb 04, 2021 at 04:43:54PM -0500, Thara Gopinath wrote:
>> + /*
>> + * ECB and CBC algorithms require message lengths to be
>> + * multiples of block size.
>> + * TODO: The spec says AES CBC mode for certain versions
>> + * of crypto engine can handle partial blocks as well.
>> + * Test and enable such messages.
>> + */
>> + if (IS_ECB(rctx->flags) || IS_CBC(rctx->flags))
>> + if (!IS_ALIGNED(req->cryptlen, blocksize))
>> + return -EINVAL;
>
> CBC by definition only operates on full blocks, so the TODO doesn't make sense.
> Is the partial block support really CTS-CBC?
Ya you are right. It should be CTS-CBC and not AES CBC. Though the spec
is quite fuzzy about this part.
I can remove the comment and spin the next version or just leave it
there for now and remove it later.
>
> - Eric
>
--
Warm Regards
Thara
On Thu, Feb 04, 2021 at 07:09:53PM -0500, Thara Gopinath wrote:
> > > @@ -260,6 +261,10 @@ 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;
> > > + /* CE does not handle 0 length messages */
> > > + if (!req->cryptlen)
> > > + return -EOPNOTSUPP;
> > > +
> >
> > For the algorithms in question, the correct behavior is to return 0.
>
> What do you mean? The driver should return a 0 ?
Yes, there is nothing to do for empty inputs, so just return 0 (success).
> > Aren't the tests catching that difference?
>
> I was anyways planning on sending an email to the list with these queries.
> But since you asked, these are my observations with fuzz testing which I
> have been doing quite a bit now (I am also working on adding a few qualcomm
> AEAD algorithms support in mainline).
>
> - if the generic algorithm supports 0 length messages and the transformation
> I am testing does not, the test framework throws an error and stops.
> - key support mismatch between the generic algorithm vs my algorithm /engine
> also does the same thing.For eg, Qualcomm CE engine does not support any
> three keys being same for triple des algorithms. Where as a two key 3des is
> a valid scenario for generic algorithm(k1=k3). Another example is hardware
> engine not supporting AES192.
>
> How are these scenarios usually handled ? Why not allow the test framework
> to proceed with the testing if the algorithm does not support a particular
> scenario ?
Omitting support for certain inputs isn't allowed. Anyone in the kernel who
wants to use a particular algorithm could get this driver for it, and if they
happen to use inputs which the driver decided not to support, things will break.
The way that drivers handle this is to use a fallback cipher for inputs they
don't support.
- Eric
On 2/4/21 7:26 PM, Eric Biggers wrote:
> On Thu, Feb 04, 2021 at 07:09:53PM -0500, Thara Gopinath wrote:
>>>> @@ -260,6 +261,10 @@ 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;
>>>> + /* CE does not handle 0 length messages */
>>>> + if (!req->cryptlen)
>>>> + return -EOPNOTSUPP;
>>>> +
>>>
>>> For the algorithms in question, the correct behavior is to return 0.
>>
>> What do you mean? The driver should return a 0 ?
Ok. I will re-spin the series once more with this change..
>
> Yes, there is nothing to do for empty inputs, so just return 0 (success).
>
>>> Aren't the tests catching that difference?
>>
>> I was anyways planning on sending an email to the list with these queries.
>> But since you asked, these are my observations with fuzz testing which I
>> have been doing quite a bit now (I am also working on adding a few qualcomm
>> AEAD algorithms support in mainline).
>>
>> - if the generic algorithm supports 0 length messages and the transformation
>> I am testing does not, the test framework throws an error and stops.
>> - key support mismatch between the generic algorithm vs my algorithm /engine
>> also does the same thing.For eg, Qualcomm CE engine does not support any
>> three keys being same for triple des algorithms. Where as a two key 3des is
>> a valid scenario for generic algorithm(k1=k3). Another example is hardware
>> engine not supporting AES192.
>>
>> How are these scenarios usually handled ? Why not allow the test framework
>> to proceed with the testing if the algorithm does not support a particular
>> scenario ?
>
> Omitting support for certain inputs isn't allowed. Anyone in the kernel who
> wants to use a particular algorithm could get this driver for it, and if they
> happen to use inputs which the driver decided not to support, things will break.
Ya sounds reasonable.
>
> The way that drivers handle this is to use a fallback cipher for inputs they
> don't support.
Ok. So I will add this to my todo and make sure to have fallback ciphers
for all the non-supported inputs. I will send this as a separate series
and not this one.
In this case, though not supporting 0 length messages for encryption is
valid. I don't think I have to have a fallback for this. I could have
sworn that the test framework throws up an error for this. But I have
been testing a lot and may be I am just confused. I will double check this.
>
> - Eric
>
--
Warm Regards
Thara