[PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Christophe JAILLET]

This error handling looks really strange.
Check if the string has been truncated instead.

Fixes: 02ab994635eb ("crypto: hisilicon - Fixed some tiny bugs of HPRE")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/crypto/hisilicon/hpre/hpre_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c b/drivers/crypto/hisilicon/hpre/hpre_main.c
index 39297ce70f44..db44d889438a 100644
--- a/drivers/crypto/hisilicon/hpre/hpre_main.c
+++ b/drivers/crypto/hisilicon/hpre/hpre_main.c
@@ -1033,7 +1033,7 @@ static int hpre_cluster_debugfs_init(struct hisi_qm *qm)
 
 	for (i = 0; i < clusters_num; i++) {
 		ret = snprintf(buf, HPRE_DBGFS_VAL_MAX_LEN, "cluster%d", i);
-		if (ret < 0)
+		if (ret >= HPRE_DBGFS_VAL_MAX_LEN)
 			return -EINVAL;
 		tmp_d = debugfs_create_dir(buf, qm->debug.debug_root);
 
-- 
2.34.1

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Herbert Xu]

On Mon, Sep 04, 2023 at 10:17:29PM +0200, Christophe JAILLET wrote:
> This error handling looks really strange.
> Check if the string has been truncated instead.
> 
> Fixes: 02ab994635eb ("crypto: hisilicon - Fixed some tiny bugs of HPRE")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/crypto/hisilicon/hpre/hpre_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c b/drivers/crypto/hisilicon/hpre/hpre_main.c
> index 39297ce70f44..db44d889438a 100644
> --- a/drivers/crypto/hisilicon/hpre/hpre_main.c
> +++ b/drivers/crypto/hisilicon/hpre/hpre_main.c
> @@ -1033,7 +1033,7 @@ static int hpre_cluster_debugfs_init(struct hisi_qm *qm)
>  
>  	for (i = 0; i < clusters_num; i++) {
>  		ret = snprintf(buf, HPRE_DBGFS_VAL_MAX_LEN, "cluster%d", i);
> -		if (ret < 0)
> +		if (ret >= HPRE_DBGFS_VAL_MAX_LEN)
>  			return -EINVAL;
>  		tmp_d = debugfs_create_dir(buf, qm->debug.debug_root);

Who is going to free the allocated memory in case of error?

The other snprintf in the same file also looks suspect.

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: hisilicon/hpre - Fix a erroneous check after snprintf()

[Marion & Christophe JAILLET]

Le 05/09/2023 à 04:27, Herbert Xu a écrit :
> On Mon, Sep 04, 2023 at 10:17:29PM +0200, Christophe JAILLET wrote:
>> This error handling looks really strange.
>> Check if the string has been truncated instead.
>>
>> Fixes: 02ab994635eb ("crypto: hisilicon - Fixed some tiny bugs of HPRE")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/crypto/hisilicon/hpre/hpre_main.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c b/drivers/crypto/hisilicon/hpre/hpre_main.c
>> index 39297ce70f44..db44d889438a 100644
>> --- a/drivers/crypto/hisilicon/hpre/hpre_main.c
>> +++ b/drivers/crypto/hisilicon/hpre/hpre_main.c
>> @@ -1033,7 +1033,7 @@ static int hpre_cluster_debugfs_init(struct hisi_qm *qm)
>>   
>>   	for (i = 0; i < clusters_num; i++) {
>>   		ret = snprintf(buf, HPRE_DBGFS_VAL_MAX_LEN, "cluster%d", i);
>> -		if (ret < 0)
>> +		if (ret >= HPRE_DBGFS_VAL_MAX_LEN)
>>   			return -EINVAL;
>>   		tmp_d = debugfs_create_dir(buf, qm->debug.debug_root);
> Who is going to free the allocated memory in case of error?

Not sure to understand.

The memory is allocated with devm_kzalloc(), so it will be freed by the 
framework.

Some debugfs dir of file way be left around. Is it what your are talking 
about?

>
> The other snprintf in the same file also looks suspect.

It looks correct to me.

And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The 
string can't be truncated with just a "%u\n".

CJ

>
> Thanks,

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Herbert Xu]

On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
>
> Some debugfs dir of file way be left around. Is it what your are talking
> about?

Yes all allocated resources should be freed on the error path.

> > The other snprintf in the same file also looks suspect.
> 
> It looks correct to me.
> 
> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
> can't be truncated with just a "%u\n".

Well if you're going to go with that line of reasoning then this
case ("cluster%d") can't overflow either, no?

Cheers,
-- 
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: hisilicon/hpre - Fix a erroneous check after snprintf()

[liulongfang]

On 2023/9/5 16:17, Herbert Xu wrote:
> On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
>>
>> Some debugfs dir of file way be left around. Is it what your are talking
>> about?
> 
> Yes all allocated resources should be freed on the error path.
> 
>>> The other snprintf in the same file also looks suspect.
>>
>> It looks correct to me.
>>
>> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
>> can't be truncated with just a "%u\n".
> 
> Well if you're going to go with that line of reasoning then this
> case ("cluster%d") can't overflow either, no?
>

First, I checked the calling code of the snprintf function in all driver files in
the hisilicon directory. Only here is the processing of return value judgment.
This treatment is indeed problematic and needs to be modified.

Then, I don't quite agree with your modification plan.
The modification of this solution is not complete.
As Herbert said, ("cluster%d") may still have overflow problems.

In the end, my proposed modification scheme is this:
...
	int ret;
	u8 i;

	for (i = 0; i < clusters_num; i++) {
		snprintf(buf, HPRE_DBGFS_VAL_MAX_LEN, "cluster%u", i);
		tmp_d = debugfs_create_dir(buf, qm->debug.debug_root);
		...
	}
...

Thanks,
Longfang.

> Cheers,
> 

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Dan Carpenter]

On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
> > 
> > The other snprintf in the same file also looks suspect.
> 
> It looks correct to me.
> 
> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
> can't be truncated with just a "%u\n".
> 

drivers/crypto/hisilicon/hpre/hpre_main.c
   884          ret = snprintf(tbuf, HPRE_DBGFS_VAL_MAX_LEN, "%u\n", val);
   885          return simple_read_from_buffer(buf, count, pos, tbuf, ret);

You can't pass the return value from snprintf() to simple_read_from_buffer().
Otherwise the snprintf() checking turned a sprintf() write overflow into
a read overflow, which is less bad but not ideal.  It needs to be
scnprintf().

regards,
dan carpenter

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Christophe JAILLET]

Le 06/09/2023 à 04:04, liulongfang a écrit :
> On 2023/9/5 16:17, Herbert Xu wrote:
>> On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
>>>
>>> Some debugfs dir of file way be left around. Is it what your are talking
>>> about?
>>
>> Yes all allocated resources should be freed on the error path.
>>
>>>> The other snprintf in the same file also looks suspect.
>>>
>>> It looks correct to me.
>>>
>>> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
>>> can't be truncated with just a "%u\n".
>>
>> Well if you're going to go with that line of reasoning then this
>> case ("cluster%d") can't overflow either, no?
>>
> 
> First, I checked the calling code of the snprintf function in all driver files in
> the hisilicon directory. Only here is the processing of return value judgment.
> This treatment is indeed problematic and needs to be modified.
> 
> Then, I don't quite agree with your modification plan.
> The modification of this solution is not complete.
> As Herbert said, ("cluster%d") may still have overflow problems.

Herbert said the contrary, and I agree with him.

HPRE_DBGFS_VAL_MAX_LEN is 20.

cluster%u will be at max:
	strlen("cluster") + strlen("4294967295") + 1 = 17

(unless some system have 64 bits int?)

I do agree that it is safe to remove the test after snprintf(), but 
there is no need from my POV to turn "i" into a u8.

CJ

> 
> In the end, my proposed modification scheme is this:
> ...
> 	int ret;
> 	u8 i;
> 
> 	for (i = 0; i < clusters_num; i++) {
> 		snprintf(buf, HPRE_DBGFS_VAL_MAX_LEN, "cluster%u", i);
> 		tmp_d = debugfs_create_dir(buf, qm->debug.debug_root);
> 		...
> 	}
> ...
> 
> Thanks,
> Longfang.
> 
>> Cheers,
>>
> 

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[liulongfang]

On 2023/9/9 0:11, Christophe JAILLET wrote:
> Le 06/09/2023 à 04:04, liulongfang a écrit :
>> On 2023/9/5 16:17, Herbert Xu wrote:
>>> On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
>>>>
>>>> Some debugfs dir of file way be left around. Is it what your are talking
>>>> about?
>>>
>>> Yes all allocated resources should be freed on the error path.
>>>
>>>>> The other snprintf in the same file also looks suspect.
>>>>
>>>> It looks correct to me.
>>>>
>>>> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
>>>> can't be truncated with just a "%u\n".
>>>
>>> Well if you're going to go with that line of reasoning then this
>>> case ("cluster%d") can't overflow either, no?
>>>
>>
>> First, I checked the calling code of the snprintf function in all driver files in
>> the hisilicon directory. Only here is the processing of return value judgment.
>> This treatment is indeed problematic and needs to be modified.
>>
>> Then, I don't quite agree with your modification plan.
>> The modification of this solution is not complete.
>> As Herbert said, ("cluster%d") may still have overflow problems.
> 
> Herbert said the contrary, and I agree with him.
> 
> HPRE_DBGFS_VAL_MAX_LEN is 20.
> 
> cluster%u will be at max:
>     strlen("cluster") + strlen("4294967295") + 1 = 17
> 
> (unless some system have 64 bits int?)
> 
> I do agree that it is safe to remove the test after snprintf(), but there is no need from my POV to turn "i" into a u8.
>

OK, your analysis makes sense.

Thanks.
Longfang.

> CJ
> 
>>
>> In the end, my proposed modification scheme is this:
>> ...
>>     int ret;
>>     u8 i;
>>
>>     for (i = 0; i < clusters_num; i++) {
>>         snprintf(buf, HPRE_DBGFS_VAL_MAX_LEN, "cluster%u", i);
>>         tmp_d = debugfs_create_dir(buf, qm->debug.debug_root);
>>         ...
>>     }
>> ...
>>
>> Thanks,
>> Longfang.
>>
>>> Cheers,
>>>
>>
> 
> .
> 

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[liulongfang]

On 2023/9/7 19:15, Dan Carpenter wrote:
> On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
>>>
>>> The other snprintf in the same file also looks suspect.
>>
>> It looks correct to me.
>>
>> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
>> can't be truncated with just a "%u\n".
>>
> 
> drivers/crypto/hisilicon/hpre/hpre_main.c
>    884          ret = snprintf(tbuf, HPRE_DBGFS_VAL_MAX_LEN, "%u\n", val);
>    885          return simple_read_from_buffer(buf, count, pos, tbuf, ret);
> 
> You can't pass the return value from snprintf() to simple_read_from_buffer().
> Otherwise the snprintf() checking turned a sprintf() write overflow into
> a read overflow, which is less bad but not ideal.  It needs to be
> scnprintf().
>

Here only one "%u" data is written to buf, the return value ret cannot exceed 10,
and the length of tbuf is 20.
How did the overflow you mentioned occur?

Thanks,
Longfang.
> regards,
> dan carpenter
> 
> 
> .
> 

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Dan Carpenter]

On Mon, Sep 11, 2023 at 09:58:56AM +0800, liulongfang wrote:
> On 2023/9/7 19:15, Dan Carpenter wrote:
> > On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
> >>>
> >>> The other snprintf in the same file also looks suspect.
> >>
> >> It looks correct to me.
> >>
> >> And HPRE_DBGFS_VAL_MAX_LEN being 20, it doesn't really matter. The string
> >> can't be truncated with just a "%u\n".
> >>
> > 
> > drivers/crypto/hisilicon/hpre/hpre_main.c
> >    884          ret = snprintf(tbuf, HPRE_DBGFS_VAL_MAX_LEN, "%u\n", val);
> >    885          return simple_read_from_buffer(buf, count, pos, tbuf, ret);
> > 
> > You can't pass the return value from snprintf() to simple_read_from_buffer().
> > Otherwise the snprintf() checking turned a sprintf() write overflow into
> > a read overflow, which is less bad but not ideal.  It needs to be
> > scnprintf().
> >
> 
> Here only one "%u" data is written to buf, the return value ret cannot exceed 10,
> and the length of tbuf is 20.
> How did the overflow you mentioned occur?

Why are we using snprintf() if the overflow can't occur?  We could just
use sprintf().

The reason why we prefer to use snprintf() is because we are trying
extra hard to avoid buffer overflows.  Belt and suspenders.  The
overflow can't happen because we measured but even if we messed up we
are still safe.

We should apply that same logic to the next line.  Even if an overflow
occurs, then we still want to be safe.  And the way to do that is to
change snprintf() to scnprintf().

It is always incorrect to assume that snprintf() cannot overflow.  It is
a mismatch.  snprintf() is for careful people, and if we are going to be
careful then we have to be careful everywhere within the function
boundary.  Outside of the function boundary then we can have different
assumptions, but within the function boundary then we have to logically
consistent.

It's the same logic as checking for NULL consistently.

	foo->bar = frob();
	if (!foo)
		return -EINVAL;

This code is wrong.  Even if foo can never be NULL and the code can
never crash, it is a logic inconsistency so it is wrong.

regards,
dan carpenter

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Dan Carpenter]

On Tue, Sep 05, 2023 at 07:27:47AM +0200, Marion & Christophe JAILLET wrote:
> 
> Some debugfs dir of file way be left around. Is it what your are talking
> about?
> 

No debugfs files are left.  There is a remove recursive in
hpre_debugfs_init().

regards,
dan carpenter

Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after snprintf()

[Herbert Xu]

On Tue, Sep 12, 2023 at 09:39:18AM +0300, Dan Carpenter wrote:
> 
> No debugfs files are left.  There is a remove recursive in
> hpre_debugfs_init().

Good catch.  I'll move the patch back onto the queue.

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: hisilicon/hpre - Fix a erroneous check after snprintf()

[Herbert Xu]

On Mon, Sep 04, 2023 at 10:17:29PM +0200, Christophe JAILLET wrote:
> This error handling looks really strange.
> Check if the string has been truncated instead.
> 
> Fixes: 02ab994635eb ("crypto: hisilicon - Fixed some tiny bugs of HPRE")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/crypto/hisilicon/hpre/hpre_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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