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

[Gabor Juhos <j4g8y7@gmail.com>]

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