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 <[email protected]>
---
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
On Fri, 31 Oct 2014 18:57:33 +0200
Cristian Stoica <[email protected]> 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
On Friday, October 31, 2014 at 07:22:09 PM, Kim Phillips wrote:
> On Fri, 31 Oct 2014 18:57:33 +0200
>
> Cristian Stoica <[email protected]> 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
Hi Kim,
On 10/31/2014 08:22 PM, Kim Phillips wrote:
> On Fri, 31 Oct 2014 18:57:33 +0200
> Cristian Stoica <[email protected]> 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.
On Mon, 3 Nov 2014 11:18:36 +0200
Cristian Stoica <[email protected]> wrote:
> On 10/31/2014 08:22 PM, Kim Phillips wrote:
> > On Fri, 31 Oct 2014 18:57:33 +0200
> > Cristian Stoica <[email protected]> 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
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.
On Tue, 4 Nov 2014 10:57:57 +0200
Cristian Stoica <[email protected]> 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
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 <[email protected]>
---
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
Hi Kim,
On 11/04/2014 06:57 PM, Kim Phillips wrote:
> On Tue, 4 Nov 2014 10:57:57 +0200
> Cristian Stoica <[email protected]> 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.
On Wed, 5 Nov 2014 11:21:24 +0200
Cristian Stoica <[email protected]> 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 <[email protected]>
> ---
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/[email protected]/msg12105.html
still applies.
Thanks,
Kim
Hi Kim, Herbert,
On 11/05/2014 06:43 PM, Kim Phillips wrote:
> On Wed, 5 Nov 2014 11:21:24 +0200
> Cristian Stoica <[email protected]> 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 <[email protected]>
>> ---
> 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/[email protected]/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.
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 <[email protected]>
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