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

[Dan Carpenter <dan.carpenter@linaro.org>]

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