| Message ID | 73534cb1713f58228d54ea53a8a137f4ef939bad.1693858632.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Accepted |
| Commit | c977950146720abff14e46d8c53f5638b06a9182 |
| Headers | show |
| Series | crypto: hisilicon/hpre - Fix a erroneous check after snprintf() | expand |
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,
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,
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,
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, >
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
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, >> >
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, >>> >> > > . >
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 > > > . >
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
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
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,
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);
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(-)