On 04.07.2018 09:52, Boris Brezillon wrote:

On Wed, 04 Jul 2018 09:43:44 +0200
Stefan Agner [off-list ref] wrote:

quoted

On 03.07.2018 22:04, Boris Brezillon wrote:

quoted

On Tue, 3 Jul 2018 17:19:57 +0300
Dan Carpenter [off-list ref] wrote:

quoted

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/