linux-kernel - Re: [PATCH] crypto: caam: fix error reporting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 3 Nov 2014 11:18:36 +0200
From:	Cristian Stoica <cristian.stoica@...escale.com>
To:	Kim Phillips <kim.phillips@...escale.com>
CC:	<herbert@...dor.apana.org.au>, <davem@...emloft.net>,
	<linux-crypto@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<horia.geanta@...escale.com>, <marex@...x.de>
Subject: Re: [PATCH] crypto: caam: fix error reporting

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@...escale.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/