diff mbox series

[v3,2/2] crypto: hisilicon/sec2 - fix for aead invalid authsize

Message ID 20241102025559.2256734-3-huangchenghai2@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series crypto: hisilicon - fix the authsize and icv problems of aead in sec | expand

Commit Message

Chenghai Huang Nov. 2, 2024, 2:55 a.m. UTC 
From: Wenkai Lin <linwenkai6@hisilicon.com>

When the digest alg is HMAC-SHAx or another, the authsize may be less
than 4 bytes and mac_len of the BD is set to zero, the hardware considers
it a BD configuration error and reports a ras error, so the sec driver
needs to switch to software calculation in this case, this patch add a
check for it and remove unnecessary check that has been done by crypto.

Fixes: 2f072d75d1ab ("crypto: hisilicon - Add aead support on SEC2")
Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
---
 drivers/crypto/hisilicon/sec2/sec_crypto.c | 59 ++++++++++++----------
 1 file changed, 31 insertions(+), 28 deletions(-)

Comments

Herbert Xu Nov. 10, 2024, 3:36 a.m. UTC | #1 
On Sat, Nov 02, 2024 at 10:55:59AM +0800, Chenghai Huang wrote:
>
> @@ -2226,15 +2236,15 @@ static int sec_aead_spec_check(struct sec_ctx *ctx, struct sec_req *sreq)
>  	struct device *dev = ctx->dev;
>  	int ret;
>  
> -	if (unlikely(req->cryptlen + req->assoclen > MAX_INPUT_DATA_LEN ||
> -	    req->assoclen > SEC_MAX_AAD_LEN)) {
> -		dev_err(dev, "aead input spec error!\n");
> +	/* Hardware does not handle cases where authsize is less than 4 bytes */
> +	if (unlikely(sz < MIN_MAC_LEN)) {
> +		ctx->a_ctx.fallback = true;

This is broken.  sec_aead_spec_check is a per-request function,
called without any locking.  Therefore it must not modify any
field in the tfm context (at least not without additional locking),
because multiple requests can be issued on the same tfm at any time.

I suppose for this field in particular you could move it to
set_authsize and there it would be safe to change the tfm context.

Cheers,
linwenkai (C) Nov. 12, 2024, 2:46 a.m. UTC | #2 
在 2024/11/10 11:36, Herbert Xu 写道:
> On Sat, Nov 02, 2024 at 10:55:59AM +0800, Chenghai Huang wrote:
>> @@ -2226,15 +2236,15 @@ static int sec_aead_spec_check(struct sec_ctx *ctx, struct sec_req *sreq)
>>   	struct device *dev = ctx->dev;
>>   	int ret;
>>   
>> -	if (unlikely(req->cryptlen + req->assoclen > MAX_INPUT_DATA_LEN ||
>> -	    req->assoclen > SEC_MAX_AAD_LEN)) {
>> -		dev_err(dev, "aead input spec error!\n");
>> +	/* Hardware does not handle cases where authsize is less than 4 bytes */
>> +	if (unlikely(sz < MIN_MAC_LEN)) {
>> +		ctx->a_ctx.fallback = true;
> This is broken.  sec_aead_spec_check is a per-request function,
> called without any locking.  Therefore it must not modify any
> field in the tfm context (at least not without additional locking),
> because multiple requests can be issued on the same tfm at any time.
>
> I suppose for this field in particular you could move it to
> set_authsize and there it would be safe to change the tfm context.
>
> Cheers,
Hi,
Thanks for your advice, it's better to move the setup to set_authsize, I 
will send a new patchset soon.
linwenkai (C) Nov. 14, 2024, 12:47 p.m. UTC | #3 
在 2024/11/10 11:36, Herbert Xu 写道:
> On Sat, Nov 02, 2024 at 10:55:59AM +0800, Chenghai Huang wrote:
>> @@ -2226,15 +2236,15 @@ static int sec_aead_spec_check(struct sec_ctx *ctx, struct sec_req *sreq)
>>   	struct device *dev = ctx->dev;
>>   	int ret;
>>   
>> -	if (unlikely(req->cryptlen + req->assoclen > MAX_INPUT_DATA_LEN ||
>> -	    req->assoclen > SEC_MAX_AAD_LEN)) {
>> -		dev_err(dev, "aead input spec error!\n");
>> +	/* Hardware does not handle cases where authsize is less than 4 bytes */
>> +	if (unlikely(sz < MIN_MAC_LEN)) {
>> +		ctx->a_ctx.fallback = true;
> This is broken.  sec_aead_spec_check is a per-request function,
> called without any locking.  Therefore it must not modify any
> field in the tfm context (at least not without additional locking),
> because multiple requests can be issued on the same tfm at any time.
>
> I suppose for this field in particular you could move it to
> set_authsize and there it would be safe to change the tfm context.
>
> Cheers,

Hi,

I have found another setup for fallback in the sec_aead_param_check 
function, so I need to fix it right too.

The orignal code:

static int sec_aead_param_check(struct sec_ctx *ctx, struct sec_req *sreq)
{

         if (ctx->sec->qm.ver == QM_HW_V2) {
                 if (unlikely(!req->cryptlen || (!sreq->c_req.encrypt &&
                              req->cryptlen <= authsize))) {
                         ctx->a_ctx.fallback = true;
                         return -EINVAL;
                 }
         }
}

After the modification, I used a temporary variable fallback to save the 
state:

static int sec_aead_param_check(struct sec_ctx *ctx, struct sec_req 
*sreq, bool *fallback)
{

         if (ctx->sec->qm.ver == QM_HW_V2) {
                 if (unlikely(!req->cryptlen || (!sreq->c_req.encrypt &&
                              req->cryptlen <= authsize))) {
                         *fallback = true;
                         return -EINVAL;
                 }
         }
}

Same with  the sec_aead_spec_check function.

static int sec_aead_spec_check(struct sec_ctx *ctx, struct sec_req 
*sreq, bool *fallback) {

         /* Hardware does not handle cases where authsize is less than 4 
bytes */
         if (unlikely(sz < MIN_MAC_LEN)) {
                 *fallback = true;
                 return -EINVAL;
         }

}

Do you think it is a better way?

Thanks,
diff mbox series

Patch

diff --git a/drivers/crypto/hisilicon/sec2/sec_crypto.c b/drivers/crypto/hisilicon/sec2/sec_crypto.c
index 8db995279545..2d260cdda584 100644
--- a/drivers/crypto/hisilicon/sec2/sec_crypto.c
+++ b/drivers/crypto/hisilicon/sec2/sec_crypto.c
@@ -1119,10 +1119,7 @@  static int sec_aead_setauthsize(struct crypto_aead *aead, unsigned int authsize)
 	struct sec_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct sec_auth_ctx *a_ctx = &ctx->a_ctx;
 
-	if (unlikely(a_ctx->fallback_aead_tfm))
-		return crypto_aead_setauthsize(a_ctx->fallback_aead_tfm, authsize);
-
-	return 0;
+	return crypto_aead_setauthsize(a_ctx->fallback_aead_tfm, authsize);
 }
 
 static int sec_aead_fallback_setkey(struct sec_auth_ctx *a_ctx,
@@ -1159,13 +1156,7 @@  static int sec_aead_setkey(struct crypto_aead *tfm, const u8 *key,
 		}
 		memcpy(c_ctx->c_key, key, keylen);
 
-		if (unlikely(a_ctx->fallback_aead_tfm)) {
-			ret = sec_aead_fallback_setkey(a_ctx, tfm, key, keylen);
-			if (ret)
-				return ret;
-		}
-
-		return 0;
+		return sec_aead_fallback_setkey(a_ctx, tfm, key, keylen);
 	}
 
 	ret = crypto_authenc_extractkeys(&keys, key, keylen);
@@ -1190,6 +1181,12 @@  static int sec_aead_setkey(struct crypto_aead *tfm, const u8 *key,
 		goto bad_key;
 	}
 
+	ret = sec_aead_fallback_setkey(a_ctx, tfm, key, keylen);
+	if (ret) {
+		dev_err(dev, "set sec fallback key err!\n");
+		goto bad_key;
+	}
+
 	return 0;
 
 bad_key:
@@ -1917,8 +1914,10 @@  static void sec_aead_exit(struct crypto_aead *tfm)
 
 static int sec_aead_ctx_init(struct crypto_aead *tfm, const char *hash_name)
 {
+	struct aead_alg *alg = crypto_aead_alg(tfm);
 	struct sec_ctx *ctx = crypto_aead_ctx(tfm);
-	struct sec_auth_ctx *auth_ctx = &ctx->a_ctx;
+	struct sec_auth_ctx *a_ctx = &ctx->a_ctx;
+	const char *aead_name = alg->base.cra_name;
 	int ret;
 
 	ret = sec_aead_init(tfm);
@@ -1927,12 +1926,22 @@  static int sec_aead_ctx_init(struct crypto_aead *tfm, const char *hash_name)
 		return ret;
 	}
 
-	auth_ctx->hash_tfm = crypto_alloc_shash(hash_name, 0, 0);
-	if (IS_ERR(auth_ctx->hash_tfm)) {
+	a_ctx->hash_tfm = crypto_alloc_shash(hash_name, 0, 0);
+	if (IS_ERR(a_ctx->hash_tfm)) {
 		dev_err(ctx->dev, "aead alloc shash error!\n");
 		sec_aead_exit(tfm);
-		return PTR_ERR(auth_ctx->hash_tfm);
+		return PTR_ERR(a_ctx->hash_tfm);
+	}
+
+	a_ctx->fallback_aead_tfm = crypto_alloc_aead(aead_name, 0,
+						     CRYPTO_ALG_NEED_FALLBACK | CRYPTO_ALG_ASYNC);
+	if (IS_ERR(a_ctx->fallback_aead_tfm)) {
+		dev_err(ctx->dev, "aead driver alloc fallback tfm error!\n");
+		crypto_free_shash(ctx->a_ctx.hash_tfm);
+		sec_aead_exit(tfm);
+		return PTR_ERR(a_ctx->fallback_aead_tfm);
 	}
+	a_ctx->fallback = false;
 
 	return 0;
 }
@@ -1941,6 +1950,7 @@  static void sec_aead_ctx_exit(struct crypto_aead *tfm)
 {
 	struct sec_ctx *ctx = crypto_aead_ctx(tfm);
 
+	crypto_free_aead(ctx->a_ctx.fallback_aead_tfm);
 	crypto_free_shash(ctx->a_ctx.hash_tfm);
 	sec_aead_exit(tfm);
 }
@@ -2226,15 +2236,15 @@  static int sec_aead_spec_check(struct sec_ctx *ctx, struct sec_req *sreq)
 	struct device *dev = ctx->dev;
 	int ret;
 
-	if (unlikely(req->cryptlen + req->assoclen > MAX_INPUT_DATA_LEN ||
-	    req->assoclen > SEC_MAX_AAD_LEN)) {
-		dev_err(dev, "aead input spec error!\n");
+	/* Hardware does not handle cases where authsize is less than 4 bytes */
+	if (unlikely(sz < MIN_MAC_LEN)) {
+		ctx->a_ctx.fallback = true;
 		return -EINVAL;
 	}
 
-	if (unlikely((c_mode == SEC_CMODE_GCM && sz < DES_BLOCK_SIZE) ||
-		     (c_mode == SEC_CMODE_CCM && (sz < MIN_MAC_LEN || sz & MAC_LEN_MASK)))) {
-		dev_err(dev, "aead input mac length error!\n");
+	if (unlikely(req->cryptlen + req->assoclen > MAX_INPUT_DATA_LEN ||
+	    req->assoclen > SEC_MAX_AAD_LEN)) {
+		dev_err(dev, "aead input spec error!\n");
 		return -EINVAL;
 	}
 
@@ -2308,16 +2318,9 @@  static int sec_aead_soft_crypto(struct sec_ctx *ctx,
 				bool encrypt)
 {
 	struct sec_auth_ctx *a_ctx = &ctx->a_ctx;
-	struct device *dev = ctx->dev;
 	struct aead_request *subreq;
 	int ret;
 
-	/* Kunpeng920 aead mode not support input 0 size */
-	if (!a_ctx->fallback_aead_tfm) {
-		dev_err(dev, "aead fallback tfm is NULL!\n");
-		return -EINVAL;
-	}
-
 	subreq = aead_request_alloc(a_ctx->fallback_aead_tfm, GFP_KERNEL);
 	if (!subreq)
 		return -ENOMEM;