[PATCH v2 1/2] crypto: inside_secure - Avoid dma map if size is zero

[PATCH v2 1/2] crypto: inside_secure - Avoid dma map if size is zero

[Peter Harliman Liem]

From commit d03c54419274 ("dma-mapping: disallow .map_sg
operations from returning zero on error"), dma_map_sg()
produces warning if size is 0. This results in visible
warnings if crypto length is zero.
To avoid that, we avoid calling dma_map_sg if size is zero.

Fixes: d03c54419274 ("dma-mapping: disallow .map_sg operations from returning zero on error")
Signed-off-by: Peter Harliman Liem <pliem@maxlinear.com>
---
v2:
 Add fixes tag

 drivers/crypto/inside-secure/safexcel_cipher.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index d68ef16650d4..3775497775e0 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -737,14 +737,17 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 				max(totlen_src, totlen_dst));
 			return -EINVAL;
 		}
-		dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);
+		if (sreq->nr_src > 0)
+			dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);
 	} else {
 		if (unlikely(totlen_src && (sreq->nr_src <= 0))) {
 			dev_err(priv->dev, "Source buffer not large enough (need %d bytes)!",
 				totlen_src);
 			return -EINVAL;
 		}
-		dma_map_sg(priv->dev, src, sreq->nr_src, DMA_TO_DEVICE);
+
+		if (sreq->nr_src > 0)
+			dma_map_sg(priv->dev, src, sreq->nr_src, DMA_TO_DEVICE);
 
 		if (unlikely(totlen_dst && (sreq->nr_dst <= 0))) {
 			dev_err(priv->dev, "Dest buffer not large enough (need %d bytes)!",
@@ -753,7 +756,9 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
 				     DMA_TO_DEVICE);
 			return -EINVAL;
 		}
-		dma_map_sg(priv->dev, dst, sreq->nr_dst, DMA_FROM_DEVICE);
+
+		if (sreq->nr_dst > 0)
+			dma_map_sg(priv->dev, dst, sreq->nr_dst, DMA_FROM_DEVICE);
 	}
 
 	memcpy(ctx->base.ctxr->data, ctx->key, ctx->key_len);
-- 
2.17.1

[PATCH v2 2/2] crypto: inside-secure - Select CRYPTO_AES config

[Peter Harliman Liem]

CRYPTO_AES is needed for aes-related algo (e.g.
safexcel-gcm-aes, safexcel-xcbc-aes, safexcel-cmac-aes).
Without it, we observe failures when allocating transform
for those algo.

Fixes: 363a90c2d517 ("crypto: safexcel/aes - switch to library version of key expansion routine")
Signed-off-by: Peter Harliman Liem <pliem@maxlinear.com>
---
v2:
 Add fixes tag

 drivers/crypto/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 3e6aa319920b..b12d222e49a1 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -740,6 +740,7 @@ config CRYPTO_DEV_SAFEXCEL
 	select CRYPTO_SHA512
 	select CRYPTO_CHACHA20POLY1305
 	select CRYPTO_SHA3
+	select CRYPTO_AES
 	help
 	  This driver interfaces with the SafeXcel EIP-97 and EIP-197 cryptographic
 	  engines designed by Inside Secure. It currently accelerates DES, 3DES and
-- 
2.17.1

Re: [PATCH v2 1/2] crypto: inside_secure - Avoid dma map if size is zero

[Antoine Tenart]

Quoting Peter Harliman Liem (2022-09-06 04:51:49)
> From commit d03c54419274 ("dma-mapping: disallow .map_sg
> operations from returning zero on error"), dma_map_sg()
> produces warning if size is 0. This results in visible
> warnings if crypto length is zero.
> To avoid that, we avoid calling dma_map_sg if size is zero.
> 
> Fixes: d03c54419274 ("dma-mapping: disallow .map_sg operations from returning zero on error")

You can't reference the commit above, it's not introducing the issue but
the warning itself. The actual commit introducing the below logic should
be referenced.

Alternatively since the warning was introduced latter than the logic and
this is not a huge issue, you might resend it w/o the Fixes tag as well.

> Signed-off-by: Peter Harliman Liem <pliem@maxlinear.com>

With the Fixes: tag fixed,

Acked-by: Antoine Tenart <atenart@kernel.org>

Thanks!

> ---
> v2:
>  Add fixes tag
> 
>  drivers/crypto/inside-secure/safexcel_cipher.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
> index d68ef16650d4..3775497775e0 100644
> --- a/drivers/crypto/inside-secure/safexcel_cipher.c
> +++ b/drivers/crypto/inside-secure/safexcel_cipher.c
> @@ -737,14 +737,17 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
>                                 max(totlen_src, totlen_dst));
>                         return -EINVAL;
>                 }
> -               dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);
> +               if (sreq->nr_src > 0)
> +                       dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);
>         } else {
>                 if (unlikely(totlen_src && (sreq->nr_src <= 0))) {
>                         dev_err(priv->dev, "Source buffer not large enough (need %d bytes)!",
>                                 totlen_src);
>                         return -EINVAL;
>                 }
> -               dma_map_sg(priv->dev, src, sreq->nr_src, DMA_TO_DEVICE);
> +
> +               if (sreq->nr_src > 0)
> +                       dma_map_sg(priv->dev, src, sreq->nr_src, DMA_TO_DEVICE);
>  
>                 if (unlikely(totlen_dst && (sreq->nr_dst <= 0))) {
>                         dev_err(priv->dev, "Dest buffer not large enough (need %d bytes)!",
> @@ -753,7 +756,9 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
>                                      DMA_TO_DEVICE);
>                         return -EINVAL;
>                 }
> -               dma_map_sg(priv->dev, dst, sreq->nr_dst, DMA_FROM_DEVICE);
> +
> +               if (sreq->nr_dst > 0)
> +                       dma_map_sg(priv->dev, dst, sreq->nr_dst, DMA_FROM_DEVICE);
>         }
>  
>         memcpy(ctx->base.ctxr->data, ctx->key, ctx->key_len);
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/2] crypto: inside-secure - Select CRYPTO_AES config

[Antoine Tenart]

Quoting Peter Harliman Liem (2022-09-06 04:51:50)
> CRYPTO_AES is needed for aes-related algo (e.g.
> safexcel-gcm-aes, safexcel-xcbc-aes, safexcel-cmac-aes).
> Without it, we observe failures when allocating transform
> for those algo.
> 
> Fixes: 363a90c2d517 ("crypto: safexcel/aes - switch to library version of key expansion routine")

The above commit explicitly switched crypto drivers to use the AES
library instead of the generic AES cipher one, which seems like a good
move. What are the issues you're encountering and why the AES lib makes
the driver to fail?

> Signed-off-by: Peter Harliman Liem <pliem@maxlinear.com>
> ---
> v2:
>  Add fixes tag
> 
>  drivers/crypto/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 3e6aa319920b..b12d222e49a1 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -740,6 +740,7 @@ config CRYPTO_DEV_SAFEXCEL
>         select CRYPTO_SHA512
>         select CRYPTO_CHACHA20POLY1305
>         select CRYPTO_SHA3
> +       select CRYPTO_AES
>         help
>           This driver interfaces with the SafeXcel EIP-97 and EIP-197 cryptographic
>           engines designed by Inside Secure. It currently accelerates DES, 3DES and
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/2] crypto: inside_secure - Avoid dma map if size is zero

[Peter Harliman Liem]

On 6/9/2022 10:01 pm, Antoine Tenart wrote:
> Quoting Peter Harliman Liem (2022-09-06 04:51:49)
>> From commit d03c54419274 ("dma-mapping: disallow .map_sg
>> operations from returning zero on error"), dma_map_sg()
>> produces warning if size is 0. This results in visible
>> warnings if crypto length is zero.
>> To avoid that, we avoid calling dma_map_sg if size is zero.
>>
>> Fixes: d03c54419274 ("dma-mapping: disallow .map_sg operations from returning zero on error")
> 
> You can't reference the commit above, it's not introducing the issue but
> the warning itself. The actual commit introducing the below logic should
> be referenced.
> 
> Alternatively since the warning was introduced latter than the logic and
> this is not a huge issue, you might resend it w/o the Fixes tag as well.
Noted.
I will remove the tag in v3.

Thanks!

Re: [PATCH v2 2/2] crypto: inside-secure - Select CRYPTO_AES config

[Peter Harliman Liem]

On 6/9/2022 10:05 pm, Antoine Tenart wrote:
>> CRYPTO_AES is needed for aes-related algo (e.g.
>> safexcel-gcm-aes, safexcel-xcbc-aes, safexcel-cmac-aes).
>> Without it, we observe failures when allocating transform
>> for those algo.
>>
>> Fixes: 363a90c2d517 ("crypto: safexcel/aes - switch to library version of key expansion routine")
> 
> The above commit explicitly switched crypto drivers to use the AES
> library instead of the generic AES cipher one, which seems like a good
> move. What are the issues you're encountering and why the AES lib makes
> the driver to fail?

If I load the kernel module (CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not
set), I am getting failure messages below.
IMHO this happens because some functions in the driver still rely on
generic AES cipher (e.g. refer to safexcel_aead_gcm_cra_init() or
safexcel_xcbcmac_cra_init()), therefore CONFIG_CRYPTO_AES is still needed.

Maybe the alternative is to switch all of them to use AES lib instead?
Let me know if you prefer this.

Thanks!

[  157.683462] alg: aead: failed to allocate transform for
safexcel-gcm-aes: -2
[  157.689054] ------------[ cut here ]------------
[  157.693650] alg: self-tests for safexcel-gcm-aes (gcm(aes)) failed
(rc=-2)
[  157.693696] WARNING: CPU: 3 PID: 164 at crypto/testmgr.c:5804
alg_test.part.0+0xd1/0x2f0 [cryptomgr]
[  157.709505] Modules linked in: crypto_safexcel(+) md5 sha512_generic
libdes sha256_generic libsha256 sha1_gene
ric libsha1 libaes authenc cryptomgr akcipher crypto_acompress kpp rng
crypto_null crypto_hash skcipher aead cryp
to_algapi
[  157.729991] CPU: 3 PID: 164 Comm: cryptomgr_test Not tainted
6.0.0-rc3+ #42
[  157.736900] RIP: 0010:alg_test.part.0+0xd1/0x2f0 [cryptomgr]
[  157.742512] Code: 6c 85 db 0f 84 89 00 00 00 80 3d 59 6c 05 00 00 0f
85 08 40 00 00 89 d9 4c 89 f2 48 89 ee 48
 c7 c7 40 1c 09 a0 e8 cc 17 28 e2 <0f> 0b 48 8b 84 24 90 00 00 00 65 48
33 04 25 28 00 00 00 0f 85 eb
[  157.761177] RSP: 0018:ffffc90000557e40 EFLAGS: 00010246
[  157.766359] RAX: 0000000000000000 RBX: 00000000fffffffe RCX:
0000000000000000
[  157.773440] RDX: 0000000000000001 RSI: 00000000ffffdfff RDI:
00000000ffffffff
[  157.780528] RBP: ffff8880046ed200 R08: 0000000000000000 R09:
ffffc90000557ce8
[  157.787610] R10: 0000000000000001 R11: 0000000000000003 R12:
0000000000011083
[  157.794699] R13: 0000000000000400 R14: ffff8880046ed280 R15:
ffffffffffffffff
[  157.801780] FS:  0000000000000000(0000) GS:ffff88807a580000(0000)
knlGS:0000000000000000
[  157.809818] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  157.815516] CR2: 0000560cece6c0d8 CR3: 0000000002a0a000 CR4:
00000000001006a0
[  157.822608] Call Trace:
[  157.825018]  <TASK>
[  157.827104]  ? _raw_spin_unlock+0xd/0x30
[  157.830989]  ? finish_task_switch+0x8a/0x260
[  157.835221]  ? __schedule+0x27c/0x670
[  157.838855]  ? 0xffffffffa004c000
[  157.842131]  cryptomgr_test+0x22/0x50 [cryptomgr]
[  157.846804]  kthread+0xd0/0x100
[  157.849908]  ? kthread_complete_and_exit+0x20/0x20
[  157.854664]  ret_from_fork+0x1f/0x30
[  157.858209]  </TASK>
[  157.860346] ---[ end trace 0000000000000000 ]---
[  157.908463] alg: hash: failed to allocate transform for
safexcel-xcbc-aes: -2
[  157.914132] ------------[ cut here ]------------

Re: [PATCH v2 1/2] crypto: inside_secure - Avoid dma map if size is zero

[Herbert Xu]

On Tue, Sep 06, 2022 at 10:51:49AM +0800, Peter Harliman Liem wrote:
>
> diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
> index d68ef16650d4..3775497775e0 100644
> --- a/drivers/crypto/inside-secure/safexcel_cipher.c
> +++ b/drivers/crypto/inside-secure/safexcel_cipher.c
> @@ -737,14 +737,17 @@ static int safexcel_send_req(struct crypto_async_request *base, int ring,
>  				max(totlen_src, totlen_dst));
>  			return -EINVAL;
>  		}
> -		dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);
> +		if (sreq->nr_src > 0)
> +			dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);

Where is the corresponding check on unmap?

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

Re: [PATCH v2 2/2] crypto: inside-secure - Select CRYPTO_AES config

[Antoine Tenart]

Quoting Peter Harliman Liem (2022-09-07 08:46:32)
> On 6/9/2022 10:05 pm, Antoine Tenart wrote:
> >> CRYPTO_AES is needed for aes-related algo (e.g.
> >> safexcel-gcm-aes, safexcel-xcbc-aes, safexcel-cmac-aes).
> >> Without it, we observe failures when allocating transform
> >> for those algo.
> >>
> >> Fixes: 363a90c2d517 ("crypto: safexcel/aes - switch to library version of key expansion routine")
> > 
> > The above commit explicitly switched crypto drivers to use the AES
> > library instead of the generic AES cipher one, which seems like a good
> > move. What are the issues you're encountering and why the AES lib makes
> > the driver to fail?
> 
> If I load the kernel module (CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not
> set), I am getting failure messages below.
> IMHO this happens because some functions in the driver still rely on
> generic AES cipher (e.g. refer to safexcel_aead_gcm_cra_init() or
> safexcel_xcbcmac_cra_init()), therefore CONFIG_CRYPTO_AES is still needed.

That's possible, and the right fix might be what you proposed. I think
it would be nice to understand what is failing and where, so we have a
good argument for restoring the AES dependency (or not).

> Maybe the alternative is to switch all of them to use AES lib instead?
> Let me know if you prefer this.

If the AES lib can be used instead of the AES generic implementation
that would be great yes. If that's possible, depending on what is
actually failing, yes please go for this solution. Otherwise restoring
the AES dependency with a good explanation should work.

Thanks!
Antoine

Re: [PATCH v2 1/2] crypto: inside_secure - Avoid dma map if size is zero

[Peter Harliman Liem]

On 7/9/2022 2:52 pm, Herbert Xu wrote:
> On Tue, Sep 06, 2022 at 10:51:49AM +0800, Peter Harliman Liem wrote:
>  >
>  > diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c 
> b/drivers/crypto/inside-secure/safexcel_cipher.c
>  > index d68ef16650d4..3775497775e0 100644
>  > --- a/drivers/crypto/inside-secure/safexcel_cipher.c
>  > +++ b/drivers/crypto/inside-secure/safexcel_cipher.c
>  > @@ -737,14 +737,17 @@ static int safexcel_send_req(struct 
> crypto_async_request *base, int ring,
>  > max(totlen_src, totlen_dst));
>  > return -EINVAL;
>  > }
>  > - dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);
>  > + if (sreq->nr_src > 0)
>  > + dma_map_sg(priv->dev, src, sreq->nr_src, DMA_BIDIRECTIONAL);
> 
> Where is the corresponding check on unmap?

Added in v3.

Thanks!

Re: [PATCH v2 2/2] crypto: inside-secure - Select CRYPTO_AES config

[Peter Harliman Liem]

On 7/9/2022 4:48 pm, Antoine Tenart wrote:
> Quoting Peter Harliman Liem (2022-09-07 08:46:32)
>> On 6/9/2022 10:05 pm, Antoine Tenart wrote:
>>>> CRYPTO_AES is needed for aes-related algo (e.g.
>>>> safexcel-gcm-aes, safexcel-xcbc-aes, safexcel-cmac-aes).
>>>> Without it, we observe failures when allocating transform
>>>> for those algo.
>>>>
>>>> Fixes: 363a90c2d517 ("crypto: safexcel/aes - switch to library version of key expansion routine")
>>>
>>> The above commit explicitly switched crypto drivers to use the AES
>>> library instead of the generic AES cipher one, which seems like a good
>>> move. What are the issues you're encountering and why the AES lib makes
>>> the driver to fail?
>>
>> If I load the kernel module (CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not
>> set), I am getting failure messages below.
>> IMHO this happens because some functions in the driver still rely on
>> generic AES cipher (e.g. refer to safexcel_aead_gcm_cra_init() or
>> safexcel_xcbcmac_cra_init()), therefore CONFIG_CRYPTO_AES is still needed.
> 
> That's possible, and the right fix might be what you proposed. I think
> it would be nice to understand what is failing and where, so we have a
> good argument for restoring the AES dependency (or not).
> 
>> Maybe the alternative is to switch all of them to use AES lib instead?
>> Let me know if you prefer this.
> 
> If the AES lib can be used instead of the AES generic implementation
> that would be great yes. If that's possible, depending on what is
> actually failing, yes please go for this solution. Otherwise restoring
> the AES dependency with a good explanation should work.

I have replaced this commit in v3 with the change to AES lib instead.

Thanks!