Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] crypto: hisilicon/hpre - Fix a erroneous check after
 snprintf()
To:     Herbert Xu <herbert@gondor.apana.org.au>,
        Marion & Christophe JAILLET <christophe.jaillet@wanadoo.fr>
CC:     "David S. Miller" <davem@davemloft.net>,
        Zaibo Xu <xuzaibo@huawei.com>, <linux-kernel@vger.kernel.org>,
        <kernel-janitors@vger.kernel.org>, <linux-crypto@vger.kernel.org>
References: <73534cb1713f58228d54ea53a8a137f4ef939bad.1693858632.git.christophe.jaillet@wanadoo.fr>
 <ZPaSCOX1F9b36rxV@gondor.apana.org.au>
 <00bdcfec-6cc1-e521-ceaa-d16d6341ca16@wanadoo.fr>
 <ZPbkJOPsx4as96D/@gondor.apana.org.au>
From:   liulongfang <liulongfang@huawei.com>
Message-ID: <f4379fee-22c2-3b94-1725-70a317cc9baf@huawei.com>
Date:   Wed, 6 Sep 2023 10:04:42 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <ZPbkJOPsx4as96D/@gondor.apana.org.au>
Content-Type: text/plain; charset="gbk"
Content-Transfer-Encoding: 7bit
Precedence: bulk

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,
>