[PATCH] crypto: caam: fix error reporting

[PATCH] crypto: caam: fix error reporting

[Cristian Stoica]

The error code returned by hardware is four bits wide with an expected
zero MSB. A hardware error condition where the error code can get between
0x8 and 0xf will trigger an out of bound array access on the error
message table.
This patch fixes the invalid array access following such an error and
reports the condition.

Signed-off-by: Cristian Stoica <cristian.stoica@freescale.com>
---
 drivers/crypto/caam/error.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
index 6531054..c4483ad 100644
--- a/drivers/crypto/caam/error.c
+++ b/drivers/crypto/caam/error.c
@@ -213,7 +213,7 @@ void caam_jr_strstatus(struct device *jrdev, u32 status)
 		void (*report_ssed)(struct device *jrdev, const u32 status,
 				    const char *error);
 		const char *error;
-	} status_src[] = {
+	} status_src[16] = {
 		{ NULL, "No error" },
 		{ NULL, NULL },
 		{ report_ccb_status, "CCB" },
@@ -222,18 +222,23 @@ void caam_jr_strstatus(struct device *jrdev, u32 status)
 		{ NULL, NULL },
 		{ report_jr_status, "Job Ring" },
 		{ report_cond_code_status, "Condition Code" },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
 	};
 	u32 ssrc = status >> JRSTA_SSRC_SHIFT;
 	const char *error = status_src[ssrc].error;
 
-	/*
-	 * If there is no further error handling function, just
-	 * print the error code, error string and exit. Otherwise
-	 * call the handler function.
-	 */
-	if (!status_src[ssrc].report_ssed)
-		dev_err(jrdev, "%08x: %s: \n", status, status_src[ssrc].error);
-	else
+	if (status_src[ssrc].report_ssed)
 		status_src[ssrc].report_ssed(jrdev, status, error);
+	else if (error)
+		dev_err(jrdev, "%d: %s\n", ssrc, error);
+	else
+		dev_err(jrdev, "%d: unknown error code\n", ssrc);
 }
 EXPORT_SYMBOL(caam_jr_strstatus);
-- 
1.8.3.1

Re: [PATCH] crypto: caam: fix error reporting

[Kim Phillips]

On Fri, 31 Oct 2014 18:57:33 +0200
Cristian Stoica <cristian.stoica@freescale.com> wrote:

> The error code returned by hardware is four bits wide with an expected
> zero MSB. A hardware error condition where the error code can get between
> 0x8 and 0xf will trigger an out of bound array access on the error
> message table.

If this issue was brought up by h/w, the appropriate new error codes
should be being introduced.

Otherwise, I'm assuming it was brought up by a static code analyser,
which technically could be ignored, but...

> -	/*
> -	 * If there is no further error handling function, just
> -	 * print the error code, error string and exit. Otherwise
> -	 * call the handler function.
> -	 */

why remove the comment?  It's still valid.

> -	if (!status_src[ssrc].report_ssed)
> -		dev_err(jrdev, "%08x: %s: \n", status, status_src[ssrc].error);
> -	else
> +	if (status_src[ssrc].report_ssed)
>  		status_src[ssrc].report_ssed(jrdev, status, error);
> +	else if (error)
> +		dev_err(jrdev, "%d: %s\n", ssrc, error);
> +	else
> +		dev_err(jrdev, "%d: unknown error code\n", ssrc);

This is simpler:

diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
index 6531054..6f4a148 100644
--- a/drivers/crypto/caam/error.c
+++ b/drivers/crypto/caam/error.c
@@ -224,7 +224,12 @@ void caam_jr_strstatus(struct device *jrdev, u32 status)
                { report_cond_code_status, "Condition Code" },
        };
        u32 ssrc = status >> JRSTA_SSRC_SHIFT;
-       const char *error = status_src[ssrc].error;
+       const char *error;
+
+       if (ssrc >= ARRAY_SIZE(status_src)) {
+               dev_err(jrdev, "unknown error status source %d\n", ssrc);
+               return;
+       }
 
        /*
         * If there is no further error handling function, just

Kim

Re: [PATCH] crypto: caam: fix error reporting

[Marek Vasut]

On Friday, October 31, 2014 at 07:22:09 PM, Kim Phillips wrote:
> On Fri, 31 Oct 2014 18:57:33 +0200
> 
> Cristian Stoica <cristian.stoica@freescale.com> wrote:
> > The error code returned by hardware is four bits wide with an expected
> > zero MSB. A hardware error condition where the error code can get between
> > 0x8 and 0xf will trigger an out of bound array access on the error
> > message table.
> 
> If this issue was brought up by h/w, the appropriate new error codes
> should be being introduced.
> 
> Otherwise, I'm assuming it was brought up by a static code analyser,
> which technically could be ignored, but...
> 
> > -	/*
> > -	 * If there is no further error handling function, just
> > -	 * print the error code, error string and exit. Otherwise
> > -	 * call the handler function.
> > -	 */
> 
> why remove the comment?  It's still valid.
> 
> > -	if (!status_src[ssrc].report_ssed)
> > -		dev_err(jrdev, "%08x: %s: \n", status, status_src[ssrc].error);
> > -	else
> > +	if (status_src[ssrc].report_ssed)
> > 
> >  		status_src[ssrc].report_ssed(jrdev, status, error);
> > 
> > +	else if (error)
> > +		dev_err(jrdev, "%d: %s\n", ssrc, error);
> > +	else
> > +		dev_err(jrdev, "%d: unknown error code\n", ssrc);
> 
> This is simpler:
> 
> diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
> index 6531054..6f4a148 100644
> --- a/drivers/crypto/caam/error.c
> +++ b/drivers/crypto/caam/error.c
> @@ -224,7 +224,12 @@ void caam_jr_strstatus(struct device *jrdev, u32
> status) { report_cond_code_status, "Condition Code" },
>         };
>         u32 ssrc = status >> JRSTA_SSRC_SHIFT;
> -       const char *error = status_src[ssrc].error;
> +       const char *error;
> +
> +       if (ssrc >= ARRAY_SIZE(status_src)) {
> +               dev_err(jrdev, "unknown error status source %d\n", ssrc);
> +               return;
> +       }
> 
>         /*
>          * If there is no further error handling function, just

This indeed makes sense.

Best regards,
Marek Vasut

Re: [PATCH] crypto: caam: fix error reporting

[Cristian Stoica]

Hi Kim,

On 10/31/2014 08:22 PM, Kim Phillips wrote:
> On Fri, 31 Oct 2014 18:57:33 +0200
> Cristian Stoica <cristian.stoica@freescale.com> wrote:
> 
> If this issue was brought up by h/w, the appropriate new error codes
> should be being introduced.

If you have the new error codes please send them to me and I'll make an
update.

> Otherwise, I'm assuming it was brought up by a static code analyser,
> which technically could be ignored, but...

Actually, our static code analyzer did not see this one.

>> -	/*
>> -	 * If there is no further error handling function, just
>> -	 * print the error code, error string and exit. Otherwise
>> -	 * call the handler function.
>> -	 */
> 
> why remove the comment?  It's still valid.

The comment was disagreeing with the new code, so I just removed it.

>> -	if (!status_src[ssrc].report_ssed)
>> -		dev_err(jrdev, "%08x: %s: \n", status, status_src[ssrc].error);
>> -	else
>> +	if (status_src[ssrc].report_ssed)
>>  		status_src[ssrc].report_ssed(jrdev, status, error);
>> +	else if (error)
>> +		dev_err(jrdev, "%d: %s\n", ssrc, error);
>> +	else
>> +		dev_err(jrdev, "%d: unknown error code\n", ssrc);
> 
> This is simpler:
> 
> diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
> index 6531054..6f4a148 100644
> --- a/drivers/crypto/caam/error.c
> +++ b/drivers/crypto/caam/error.c
> @@ -224,7 +224,12 @@ void caam_jr_strstatus(struct device *jrdev, u32 status)
>                 { report_cond_code_status, "Condition Code" },
>         };
>         u32 ssrc = status >> JRSTA_SSRC_SHIFT;
> -       const char *error = status_src[ssrc].error;
> +       const char *error;
> +
> +       if (ssrc >= ARRAY_SIZE(status_src)) {
> +               dev_err(jrdev, "unknown error status source %d\n", ssrc);
> +               return;
> +       }

It is indeed simpler but does it consider also the missing error codes
at index 1 and 5? Just checking for an upper bound is not enough.

On the other hand, if the error field is only three bits wide instead of
four as stated by the documentation, a better fix means using a three
bit mask instead of reporting an invalid error code.


Thanks for review,

Cristian S.

Re: [PATCH] crypto: caam: fix error reporting

[Kim Phillips]

On Mon, 3 Nov 2014 11:18:36 +0200
Cristian Stoica <cristian.stoica@freescale.com> wrote:

> On 10/31/2014 08:22 PM, Kim Phillips wrote:
> > On Fri, 31 Oct 2014 18:57:33 +0200
> > Cristian Stoica <cristian.stoica@freescale.com> wrote:
> > 
> > If this issue was brought up by h/w, the appropriate new error codes
> > should be being introduced.
> 
> If you have the new error codes please send them to me and I'll make an
> update.

I was mainly inquiring as to the motive of the patch.  fwiw, I don't
see error codes with the most significant bit set in the latest
documentation (which is available from STC).

> > Otherwise, I'm assuming it was brought up by a static code analyser,
> > which technically could be ignored, but...
> 
> Actually, our static code analyzer did not see this one.

ok, so the patch technically isn't fixing anything broken, then.

> >> -	/*
> >> -	 * If there is no further error handling function, just
> >> -	 * print the error code, error string and exit. Otherwise
> >> -	 * call the handler function.
> >> -	 */
> > 
> > why remove the comment?  It's still valid.
> 
> The comment was disagreeing with the new code, so I just removed it.

the new code just added a new condition, which doesn't invalidate
the comment.  And simply removing the comment as opposed to amending
it is a bit overkill.

> >> -	if (!status_src[ssrc].report_ssed)
> >> -		dev_err(jrdev, "%08x: %s: \n", status, status_src[ssrc].error);
> >> -	else
> >> +	if (status_src[ssrc].report_ssed)
> >>  		status_src[ssrc].report_ssed(jrdev, status, error);
> >> +	else if (error)
> >> +		dev_err(jrdev, "%d: %s\n", ssrc, error);
> >> +	else
> >> +		dev_err(jrdev, "%d: unknown error code\n", ssrc);
> > 
> > This is simpler:
> > 
> > diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
> > index 6531054..6f4a148 100644
> > --- a/drivers/crypto/caam/error.c
> > +++ b/drivers/crypto/caam/error.c
> > @@ -224,7 +224,12 @@ void caam_jr_strstatus(struct device *jrdev, u32 status)
> >                 { report_cond_code_status, "Condition Code" },
> >         };
> >         u32 ssrc = status >> JRSTA_SSRC_SHIFT;
> > -       const char *error = status_src[ssrc].error;
> > +       const char *error;
> > +
> > +       if (ssrc >= ARRAY_SIZE(status_src)) {
> > +               dev_err(jrdev, "unknown error status source %d\n", ssrc);
> > +               return;
> > +       }
> 
> It is indeed simpler but does it consider also the missing error codes
> at index 1 and 5? Just checking for an upper bound is not enough.

no, the existing code already handles that.  Note that newer
documentation fills the 1 and 5 slots, too.

> On the other hand, if the error field is only three bits wide instead of
> four as stated by the documentation, a better fix means using a three
> bit mask instead of reporting an invalid error code.

true, but then we'd introduce a direct discrepancy with the
documentation, and thus h/w.

Kim

Re: [PATCH] crypto: caam: fix error reporting

[Cristian Stoica]

Hi Kim,

>> Actually, our static code analyzer did not see this one.
> 
> ok, so the patch technically isn't fixing anything broken, then.

Are you saying the code isn't broken _because_ a static tool analyser
did not see anything wrong here?


> the new code just added a new condition, which doesn't invalidate
> the comment.  And simply removing the comment as opposed to amending
> it is a bit overkill.

You are partially right, but will the staggering lack of comments in the
caam driver be fixed by duplicating a cascade of three ifs into english?


>> It is indeed simpler but does it consider also the missing error codes
>> at index 1 and 5? Just checking for an upper bound is not enough.
>
> no, the existing code already handles that.  Note that newer
> documentation fills the 1 and 5 slots, too.

If you have the new error codes please send them to me for an update.


>> On the other hand, if the error field is only three bits wide instead of
>> four as stated by the documentation, a better fix means using a three
>> bit mask instead of reporting an invalid error code.
> 
> true, but then we'd introduce a direct discrepancy with the
> documentation, and thus h/w.

You basically ask me to agree that if there are no _documented_ error
codes between 0x8 and 0xf then I should trust that they will never come
up on a 4 bit field.

Do you want me to drop the patch and pretend there is nothing to see?


Cristian S.

Re: [PATCH] crypto: caam: fix error reporting

[Kim Phillips]

On Tue, 4 Nov 2014 10:57:57 +0200
Cristian Stoica <cristian.stoica@freescale.com> wrote:

> Hi Kim,
> 
> >> Actually, our static code analyzer did not see this one.
> > 
> > ok, so the patch technically isn't fixing anything broken, then.
> 
> Are you saying the code isn't broken _because_ a static tool analyser
> did not see anything wrong here?

no: I'm saying there was no symptom - something I'd expect to gather
from the original commit message.

> > the new code just added a new condition, which doesn't invalidate
> > the comment.  And simply removing the comment as opposed to amending
> > it is a bit overkill.
> 
> You are partially right, but will the staggering lack of comments in the
> caam driver be fixed by duplicating a cascade of three ifs into english?

well, given that preamble, it would be better than removing the
existing two :), but the simpler version makes it a moot point.

> >> It is indeed simpler but does it consider also the missing error codes
> >> at index 1 and 5? Just checking for an upper bound is not enough.
> >
> > no, the existing code already handles that.  Note that newer
> > documentation fills the 1 and 5 slots, too.
> 
> If you have the new error codes please send them to me for an update.

surely you have access to the documentation, if not, let me know.

> >> On the other hand, if the error field is only three bits wide instead of
> >> four as stated by the documentation, a better fix means using a three
> >> bit mask instead of reporting an invalid error code.
> > 
> > true, but then we'd introduce a direct discrepancy with the
> > documentation, and thus h/w.
> 
> You basically ask me to agree that if there are no _documented_ error
> codes between 0x8 and 0xf then I should trust that they will never come
> up on a 4 bit field.

they may, depending on future revs of the h/w, but that's not what
this patch is about.

> Do you want me to drop the patch and pretend there is nothing to see?

no, fixing potential bugs preemptively is fine; I'd just like to
know that's the case: it wasn't clear from the original commit
message whether the problem occurred on a new h/w revision, in which
case I'd have like to have seen the driver updated with support for
the new error codes.

Kim

[PATCH v2] crypto: caam: fix error reporting

[Cristian Stoica]

The error code returned by hardware is four bits wide with an expected
zero MSB. A hardware error condition where the error code can get between
0x8 and 0xf will trigger an out of bound array access on the error
message table.
This patch fixes the invalid array access following such an error and
reports the condition.

Signed-off-by: Cristian Stoica <cristian.stoica@freescale.com>
---
 drivers/crypto/caam/error.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
index 6531054..66d73bf 100644
--- a/drivers/crypto/caam/error.c
+++ b/drivers/crypto/caam/error.c
@@ -213,27 +213,36 @@ void caam_jr_strstatus(struct device *jrdev, u32 status)
 		void (*report_ssed)(struct device *jrdev, const u32 status,
 				    const char *error);
 		const char *error;
-	} status_src[] = {
+	} status_src[16] = {
 		{ NULL, "No error" },
 		{ NULL, NULL },
 		{ report_ccb_status, "CCB" },
 		{ report_jump_status, "Jump" },
 		{ report_deco_status, "DECO" },
-		{ NULL, NULL },
+		{ NULL, "Queue Manager Interface" },
 		{ report_jr_status, "Job Ring" },
 		{ report_cond_code_status, "Condition Code" },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
+		{ NULL, NULL },
 	};
 	u32 ssrc = status >> JRSTA_SSRC_SHIFT;
 	const char *error = status_src[ssrc].error;
 
 	/*
-	 * If there is no further error handling function, just
-	 * print the error code, error string and exit. Otherwise
-	 * call the handler function.
+	 * If there is an error handling function, call it to report the error.
+	 * Otherwise print the error source name.
 	 */
-	if (!status_src[ssrc].report_ssed)
-		dev_err(jrdev, "%08x: %s: \n", status, status_src[ssrc].error);
-	else
+	if (status_src[ssrc].report_ssed)
 		status_src[ssrc].report_ssed(jrdev, status, error);
+	else if (error)
+		dev_err(jrdev, "%d: %s\n", ssrc, error);
+	else
+		dev_err(jrdev, "%d: unknown error source\n", ssrc);
 }
 EXPORT_SYMBOL(caam_jr_strstatus);
-- 
1.8.3.1

Re: [PATCH v2] crypto: caam: fix error reporting

[Kim Phillips]

On Wed, 5 Nov 2014 11:21:24 +0200
Cristian Stoica <cristian.stoica@freescale.com> wrote:

> The error code returned by hardware is four bits wide with an expected
> zero MSB. A hardware error condition where the error code can get between
> 0x8 and 0xf will trigger an out of bound array access on the error
> message table.
> This patch fixes the invalid array access following such an error and
> reports the condition.
> 
> Signed-off-by: Cristian Stoica <cristian.stoica@freescale.com>
> ---

no v2 change info?  All I noticed is the additional string for
"queue manager interface", which, without its implementation fn,
intoduces an inconsistency wrt NULL checking, so this comment:

http://www.mail-archive.com/linux-crypto@vger.kernel.org/msg12105.html

still applies.

Thanks,

Kim

Re: [PATCH v2] crypto: caam: fix error reporting

[Cristian Stoica]

Hi Kim, Herbert,


On 11/05/2014 06:43 PM, Kim Phillips wrote:
> On Wed, 5 Nov 2014 11:21:24 +0200
> Cristian Stoica <cristian.stoica@freescale.com> wrote:
> 
>> The error code returned by hardware is four bits wide with an expected
>> zero MSB. A hardware error condition where the error code can get between
>> 0x8 and 0xf will trigger an out of bound array access on the error
>> message table.
>> This patch fixes the invalid array access following such an error and
>> reports the condition.
>>
>> Signed-off-by: Cristian Stoica <cristian.stoica@freescale.com>
>> ---

> no v2 change info?  All I noticed is the additional string for
> "queue manager interface", which, without its implementation fn,

Yes. Thanks for review. That and the updated comment.

> intoduces an inconsistency wrt NULL checking, so this comment:

No. There is no inconsistency in the handling. Please be more specific.

> 
> http://www.mail-archive.com/linux-crypto@vger.kernel.org/msg12105.html
>
> still applies.

Which comment? You refer to a whole email. My rebutal still applies
though and I think your version is inferior wrt handling the issue.

Feel free to send a competing patch for review though.

Herbert, consider that without a fix one way or another, there is a
potential kernel attack vector through the error reporting function of
the caam driver.


Cristian S.

Re: [PATCH v2] crypto: caam: fix error reporting

[Herbert Xu]

On Wed, Nov 05, 2014 at 11:21:24AM +0200, Cristian Stoica wrote:
> The error code returned by hardware is four bits wide with an expected
> zero MSB. A hardware error condition where the error code can get between
> 0x8 and 0xf will trigger an out of bound array access on the error
> message table.
> This patch fixes the invalid array access following such an error and
> reports the condition.
> 
> Signed-off-by: Cristian Stoica <cristian.stoica@freescale.com>

Patch applied.  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] crypto: caam: fix error reporting

[Cristian Stoica]

Hi Kim,

On 11/04/2014 06:57 PM, Kim Phillips wrote:
> On Tue, 4 Nov 2014 10:57:57 +0200
> Cristian Stoica <cristian.stoica@freescale.com> wrote:

>> Do you want me to drop the patch and pretend there is nothing to see?
> 
> no, fixing potential bugs preemptively is fine; I'd just like to
> know that's the case: it wasn't clear from the original commit
> message whether the problem occurred on a new h/w revision, in which
> case I'd have like to have seen the driver updated with support for
> the new error codes.

This is a preemptive fix. I did not see a problem in the field with any
hardware, recent or old.

Cristian S.