On 04.07.2018 09:52, Boris Brezillon wrote:
> On Wed, 04 Jul 2018 09:43:44 +0200
> Stefan Agner <stefan@xxxxxxxx> wrote:
>
>> On 03.07.2018 22:04, Boris Brezillon wrote:
>> > On Tue, 3 Jul 2018 17:19:57 +0300
>> > Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>> >
>> >> Hello Stefan Agner,
>> >>
>> >> The patch d7d9f8ec77fe: "mtd: rawnand: add NVIDIA Tegra NAND Flash
>> >> controller driver" from Jun 24, 2018, leads to the following static
>> >> checker warning:
>> >>
>> >> drivers/mtd/nand/raw/tegra_nand.c:476 tegra_nand_select_chip()
>> >> warn: array off by one? 'nand->cs[die_nr]'
>> >>
>> >> drivers/mtd/nand/raw/tegra_nand.c
>> >> 465 static void tegra_nand_select_chip(struct mtd_info *mtd, int die_nr)
>> >> 466 {
>> >> 467 struct nand_chip *chip = mtd_to_nand(mtd);
>> >> 468 struct tegra_nand_chip *nand = to_tegra_chip(chip);
>> >> 469 struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip->controller);
>> >> 470
>> >> 471 if (die_nr < 0 || die_nr > 1) {
>> >> 472 ctrl->cur_cs = -1;
>> >> 473 return;
>> >> 474 }
>> >> 475
>> >> 476 ctrl->cur_cs = nand->cs[die_nr];
>> >> 477 }
>> >>
>> >> The story is that nand->cs[] is a one element array. Some people use
>> >> one element arrays like this as variable size arrays. It's better to
>> >> use a zero size array, but I think that might be a GCC feature and not
>> >> everyone knows you can do that. Smatch treats this one as unknown size
>> >> because apparently it can't tie it back to the kmalloc().
>> >>
>> >> But it really is a one element array and the condition is off by one.
>> >
>> > I don't see where it's off by one? With the above test, die_nr is
>> > guaranteed to be 0 when you reach the
>> > "ctrl->cur_cs = nand->cs[die_nr];" statement, right? Am I missing
>> > something?
>> >
>>
>> Yeah I had to look twice too. But die_nr can be 1 according to this
>> code...
>>
>> It should be:
>> if (die_nr < 0 || die_nr >= 1) {
>
> Oh, brain fart on my end. Indeed, now that I see the fix it's
> obvious :-).
>
> You should probably add a WARN_ON(die_nr >= ARRAY_SIZE(nand->cs)),
> because that would clearly be a bug in the core if you're passed a CS
> that is not 0 or -1 since you pass max_chipselect = 1 to nand_scan().
IMHO checking whether the stack behaves in a driver should not be
necessary...
The stack could ask for cs = 1 because the driver miss informs the stack
(wrong max_chipselect). So I guess a runtime check might be sensible.
--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
