linux-kernel - Re: [PATCH 2/2] mtd: nand: qpic_common: prevent out of bounds access of BAM arrays

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: <c73a0d7d-5618-4e57-af70-1d25ab38a7ad@gmail.com>
Date: Mon, 26 May 2025 22:21:17 +0200
From: Gabor Juhos <j4g8y7@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Mark Brown <broonie@...nel.org>, Md Sadre Alam <quic_mdalam@...cinc.com>,
 Varadarajan Narayanan <quic_varada@...cinc.com>,
 Sricharan Ramabadhran <quic_srichara@...cinc.com>,
 Richard Weinberger <richard@....at>, Vignesh Raghavendra <vigneshr@...com>,
 linux-spi@...r.kernel.org, linux-mtd@...ts.infradead.org,
 linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mtd: nand: qpic_common: prevent out of bounds access
 of BAM arrays

Hi Miquel,

[...]
>> ---
>> Preferably, this should go in via the SPI tree along with the previous
>> patch. It is not a strict requirement though, in the case it gets
>> included separately through the mtd tree it reveals the bug fixed in
>> the first patch.
> 
> Sorry, didn't see that in the first place. Fine by me.

Sorry for the inconvenience. Should I add these kind of notes into the cover
letter next time?

> 
>> ---
>>  drivers/mtd/nand/qpic_common.c       | 28 ++++++++++++++++++++++++----
>>  include/linux/mtd/nand-qpic-common.h |  8 ++++++++
>>  2 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c
>> index e0ed25b5afea9b289b767cd3d9c2d7572ed52008..fb1f81e4bdacaa3e81660a20e164926c64633513 100644
>> --- a/drivers/mtd/nand/qpic_common.c
>> +++ b/drivers/mtd/nand/qpic_common.c
>> @@ -15,6 +15,13 @@
>>  #include <linux/slab.h>
>>  #include <linux/mtd/nand-qpic-common.h>
>>  
>> +static inline int qcom_err_bam_array_full(struct qcom_nand_controller *nandc,
>> +					  const char *name)
>> +{
>> +	dev_err(nandc->dev, "BAM %s array is full\n", name);
>> +	return -EINVAL;
>> +}
> 
> This is rather uncommon, I don't know if it's very relevant to do
> that. Please drop this static inline function.

The purpose of the inline function is a bit similar to dev_err_probe().
It helps to standardize the error message, and it allows to print the message
and return with a value in one go.

So this kind of statement ...

	if (bam_txn->bam_ce_pos + size > bam_txn->bam_ce_nitems) {
		dev_err(nandc->dev, "BAM %s array is full\n", "CE");
		return -EINVAL;
	}

... can be simplied like this:

	if (bam_txn->bam_ce_pos + size > bam_txn->bam_ce_nitems)
		return qcom_err_bam_array_full(nandc, "CE");

The latter is cleaner for me, but no problem, I will remove that.

Regards,
Gabor