Re: [bug report] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Wed, 04 Jul 2018 09:43:44 +0200
Stefan Agner <stefan@agner.ch> wrote:

> On 03.07.2018 22:04, Boris Brezillon wrote:
> > On Tue, 3 Jul 2018 17:19:57 +0300
> > Dan Carpenter <dan.carpenter@oracle.com> 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().