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
