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

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]

Message-ID: <6b9ada4d-0bfc-43c6-9793-be6762c1bbba@kadam.mountain>
Date:   Mon, 11 Sep 2023 10:27:25 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     liulongfang <liulongfang@...wei.com>
Cc:     Marion & Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Zaibo Xu <xuzaibo@...wei.com>, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org, linux-crypto@...r.kernel.org
Subject: Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after
 snprintf()

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