LinuxLists.cc - [PATCH] crypto: caam: fix unchecked return value error

2023-08-08 16:09:51

Subject: [PATCH] crypto: caam: fix unchecked return value error

From: Gaurav Jain <[email protected]>

error:
Unchecked return value (CHECKED_RETURN)
check_return: Calling sg_miter_next without checking return value

fix:
added check if(!sg_miter_next)

Fixes: 8a2a0dd35f2e ("crypto: caam - strip input zeros from RSA input buffer")
Signed-off-by: Gaurav Jain <[email protected]>
Signed-off-by: Meenakshi Aggarwal <[email protected]>
---
drivers/crypto/caam/caampkc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index 72afc249d42f..7e08af751e4e 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -225,7 +225,9 @@ static int caam_rsa_count_leading_zeros(struct scatterlist *sgl,
if (len && *buff)
break;

- sg_miter_next(&miter);
+ if (!sg_miter_next(&miter))
+ break;
+
buff = miter.addr;
len = miter.length;

--
2.25.1

2023-08-08 16:36:04

by Meenakshi Aggarwal

[permalink] [raw]

Subject: [PATCH] crypto: caam/jr - fix shared IRQ line handling

From: Horia Geantă <[email protected]>

There are cases when the interrupt status register (JRINTR) is non-zero,
even though:
1. An interrupt was generated, but it was masked OR
2. There was no interrupt generated at all
for the corresponding job ring.

1. The case when interrupt is masked (JRCFGR_LS[IMSK]=1b'1)
while other events have happened and are being accounted for, e.g.
-JRINTR[HALT]=2b'10 - input job ring underwent a flush of all on-going
jobs and processing of still-existing jobs (sitting in the ring) has been
halted
-JRINTR[HALT]=2b'01 - input job ring is currently undergoing a flush
-JRINTR[ENTER_FAIL]=1b'1 - SecMon / SNVS transitioned to FAIL MODE
It doesn't matter whether these events would assert the interrupt signal
or not, interrupt is anyhow masked.

2. The case when interrupt is not masked (JRCFGR_LS[IMSK]=1b'0), however
the events accounted for in JRINTR do not generate interrupts, e.g.:
-JRINTR[HALT]=2b'01
-JRINTR[ENTER_FAIL]=1b'1 and JRCFGR_MS[FAIL_MODE]=1b'0

Currently in these cases, when the JR interrupt handler is invoked (as a
consequence of JR sharing the interrupt line with other devices - e.g.
the two JRs on i.MX7ULP) it continues execution instead of returning
IRQ_NONE.
This could lead to situations like interrupt handler clearing JRINTR (and
thus also the JRINTR[HALT] field) while corresponding job ring is
suspended and then that job ring failing on resume path, due to expecting
JRINTR[HALT]=b'10 and reading instead JRINTR[HALT]=b'00.

Fix this by checking status of JRINTR[JRI] in the JR interrupt handler.
If JRINTR[JRI]=1b'0, there was no interrupt generated for this JR and
handler must return IRQ_NONE.

Signed-off-by: Horia Geantă <[email protected]>
Signed-off-by: Meenakshi Aggarwal <[email protected]>
---
drivers/crypto/caam/jr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 5507d5d34a4c..07ec2f27cc68 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -232,7 +232,7 @@ static irqreturn_t caam_jr_interrupt(int irq, void *st_dev)
* tasklet if jobs done.
*/
irqstate = rd_reg32(&jrp->rregs->jrintstatus);
- if (!irqstate)
+ if (!(irqstate & JRINT_JR_INT))
return IRQ_NONE;

/*
--
2.25.1

2023-08-14 07:13:05

by Gaurav Jain

[permalink] [raw]

Subject: RE: [PATCH] crypto: caam/jr - fix shared IRQ line handling

Reviewed-by: Gaurav Jain <[email protected]>

> -----Original Message-----
> From: Meenakshi Aggarwal <[email protected]>
> Sent: Tuesday, August 8, 2023 4:25 PM
> To: Horia Geanta <[email protected]>; Varun Sethi <[email protected]>;
> Pankaj Gupta <[email protected]>; Gaurav Jain <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Cc: Meenakshi Aggarwal <[email protected]>
> Subject: [PATCH] crypto: caam/jr - fix shared IRQ line handling
>
> From: Horia Geantă <[email protected]>
>
> There are cases when the interrupt status register (JRINTR) is non-zero, even
> though:
> 1. An interrupt was generated, but it was masked OR 2. There was no interrupt
> generated at all for the corresponding job ring.
>
> 1. The case when interrupt is masked (JRCFGR_LS[IMSK]=1b'1) while other
> events have happened and are being accounted for, e.g.
> -JRINTR[HALT]=2b'10 - input job ring underwent a flush of all on-going jobs and
> processing of still-existing jobs (sitting in the ring) has been halted
> -JRINTR[HALT]=2b'01 - input job ring is currently undergoing a flush
> -JRINTR[ENTER_FAIL]=1b'1 - SecMon / SNVS transitioned to FAIL MODE It
> doesn't matter whether these events would assert the interrupt signal or not,
> interrupt is anyhow masked.
>
> 2. The case when interrupt is not masked (JRCFGR_LS[IMSK]=1b'0), however the
> events accounted for in JRINTR do not generate interrupts, e.g.:
> -JRINTR[HALT]=2b'01
> -JRINTR[ENTER_FAIL]=1b'1 and JRCFGR_MS[FAIL_MODE]=1b'0
>
> Currently in these cases, when the JR interrupt handler is invoked (as a
> consequence of JR sharing the interrupt line with other devices - e.g.
> the two JRs on i.MX7ULP) it continues execution instead of returning IRQ_NONE.
> This could lead to situations like interrupt handler clearing JRINTR (and thus also
> the JRINTR[HALT] field) while corresponding job ring is suspended and then that
> job ring failing on resume path, due to expecting
> JRINTR[HALT]=b'10 and reading instead JRINTR[HALT]=b'00.
>
> Fix this by checking status of JRINTR[JRI] in the JR interrupt handler.
> If JRINTR[JRI]=1b'0, there was no interrupt generated for this JR and handler
> must return IRQ_NONE.
>
> Signed-off-by: Horia Geantă <[email protected]>
> Signed-off-by: Meenakshi Aggarwal <[email protected]>
> ---
> drivers/crypto/caam/jr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index
> 5507d5d34a4c..07ec2f27cc68 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -232,7 +232,7 @@ static irqreturn_t caam_jr_interrupt(int irq, void *st_dev)
> * tasklet if jobs done.
> */
> irqstate = rd_reg32(&jrp->rregs->jrintstatus);
> - if (!irqstate)
> + if (!(irqstate & JRINT_JR_INT))
> return IRQ_NONE;
>
> /*
> --
> 2.25.1

2023-08-14 07:15:01

by Gaurav Jain

[permalink] [raw]

Subject: RE: [PATCH] crypto: caam: fix unchecked return value error

2023-08-20 14:02:37

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH] crypto: caam: fix unchecked return value error

On Tue, Aug 08, 2023 at 12:55:25PM +0200, [email protected] wrote:
> From: Gaurav Jain <[email protected]>
>
> error:
> Unchecked return value (CHECKED_RETURN)
> check_return: Calling sg_miter_next without checking return value
>
> fix:
> added check if(!sg_miter_next)
>
> Fixes: 8a2a0dd35f2e ("crypto: caam - strip input zeros from RSA input buffer")
> Signed-off-by: Gaurav Jain <[email protected]>
> Signed-off-by: Meenakshi Aggarwal <[email protected]>
> ---
> drivers/crypto/caam/caampkc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)

Patch 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