| Message ID | 20180704091310.22003-1-stefan@agner.ch |
|---|---|
| State | Accepted |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: rawnand: tegra: check bounds of die_nr properly | expand |
Hi Stefan, Stefan Agner <stefan@agner.ch> wrote on Wed, 4 Jul 2018 11:13:10 +0200: > The Tegra driver currently only support a single chip select, hence > check boundaries accordingly. This fixes a off by one issue catched > with Smatch: > drivers/mtd/nand/raw/tegra_nand.c:476 tegra_nand_select_chip() > warn: array off by one? 'nand->cs[die_nr]' > > Also warn in case the stack asks for a chip select we currently do > not support. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- Applied to nand/next. Thanks, Miquèl
On Wed, 2018-07-04 at 11:13 +0200, Stefan Agner wrote: > The Tegra driver currently only support a single chip select, hence > check boundaries accordingly. This fixes a off by one issue catched > with Smatch: > drivers/mtd/nand/raw/tegra_nand.c:476 tegra_nand_select_chip() > warn: array off by one? 'nand->cs[die_nr]' > > Also warn in case the stack asks for a chip select we currently do > not support. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > drivers/mtd/nand/raw/tegra_nand.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/tegra_nand.c > b/drivers/mtd/nand/raw/tegra_nand.c > index 4daa88d814134..e65ef584df0b9 100644 > --- a/drivers/mtd/nand/raw/tegra_nand.c > +++ b/drivers/mtd/nand/raw/tegra_nand.c > @@ -468,7 +468,9 @@ static void tegra_nand_select_chip(struct > mtd_info *mtd, int die_nr) > struct tegra_nand_chip *nand = to_tegra_chip(chip); > struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip- > >controller); > > - if (die_nr < 0 || die_nr > 1) { > + WARN_ON(die_nr >= ARRAY_SIZE(nand->cs)); Unfortunately, that has a tiny little issue as die_nr is a signed integer and ARRAY_SIZE of course is unsigned. While I could have sworn my shirt off that the compiler would have to promote this to signed this is not quite what happens and upon deselecting with -1 this warning gets triggered! I will send an updated patch explicitly casting the ARRAY_SIZE side to int as well shortly. > + > + if (die_nr < 0 || die_nr > 0) { > ctrl->cur_cs = -1; > return; > }
On Thu, Jul 12, 2018 at 01:31:57PM +0000, Marcel Ziswiler wrote: > On Wed, 2018-07-04 at 11:13 +0200, Stefan Agner wrote: > > The Tegra driver currently only support a single chip select, hence > > check boundaries accordingly. This fixes a off by one issue catched > > with Smatch: > > drivers/mtd/nand/raw/tegra_nand.c:476 tegra_nand_select_chip() > > warn: array off by one? 'nand->cs[die_nr]' > > > > Also warn in case the stack asks for a chip select we currently do > > not support. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Stefan Agner <stefan@agner.ch> > > --- > > drivers/mtd/nand/raw/tegra_nand.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/raw/tegra_nand.c > > b/drivers/mtd/nand/raw/tegra_nand.c > > index 4daa88d814134..e65ef584df0b9 100644 > > --- a/drivers/mtd/nand/raw/tegra_nand.c > > +++ b/drivers/mtd/nand/raw/tegra_nand.c > > @@ -468,7 +468,9 @@ static void tegra_nand_select_chip(struct > > mtd_info *mtd, int die_nr) > > struct tegra_nand_chip *nand = to_tegra_chip(chip); > > struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip- > > >controller); > > > > - if (die_nr < 0 || die_nr > 1) { > > + WARN_ON(die_nr >= ARRAY_SIZE(nand->cs)); > > Unfortunately, that has a tiny little issue as die_nr is a signed > integer and ARRAY_SIZE of course is unsigned. While I could have sworn > my shirt off that the compiler would have to promote this to signed > this is not quite what happens and upon deselecting with -1 this > warning gets triggered! Sorry, I didn't realize we were passing -1 as die_nr. I should have reviewed the code more carefully. The type is promoted to the which ever side has more positive bits or a minimum of int. So if you compare an int to a long long it gets promoted to long long. If you compare an int to unsigned int, it is promoted to unsigned int. If both sides are smaller than int then it is type promoted to int. For compares, I can't think how type promoting to int can be a problem but where it might matter is that that people sometimes want 0xfe and they do "~(char)0x1" but it becomes 0xfffffffe. The other trickiness is that x << shift is type x. Sometime people think if they make shift a u64 and x an int, the result will be u64 but it's still int. I think that's basically everything with type promotion. regards, dan carpenter
diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c index 4daa88d814134..e65ef584df0b9 100644 --- a/drivers/mtd/nand/raw/tegra_nand.c +++ b/drivers/mtd/nand/raw/tegra_nand.c @@ -468,7 +468,9 @@ static void tegra_nand_select_chip(struct mtd_info *mtd, int die_nr) struct tegra_nand_chip *nand = to_tegra_chip(chip); struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip->controller); - if (die_nr < 0 || die_nr > 1) { + WARN_ON(die_nr >= ARRAY_SIZE(nand->cs)); + + if (die_nr < 0 || die_nr > 0) { ctrl->cur_cs = -1; return; }
The Tegra driver currently only support a single chip select, hence check boundaries accordingly. This fixes a off by one issue catched with Smatch: drivers/mtd/nand/raw/tegra_nand.c:476 tegra_nand_select_chip() warn: array off by one? 'nand->cs[die_nr]' Also warn in case the stack asks for a chip select we currently do not support. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/mtd/nand/raw/tegra_nand.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)