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

Gabor Juhos
2025-05-26T20:21:26Z

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