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: <f4379fee-22c2-3b94-1725-70a317cc9baf@huawei.com>
Date:   Wed, 6 Sep 2023 10:04:42 +0800
From:   liulongfang <liulongfang@...wei.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>,
        Marion & Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC:     "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 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,
>