| Message ID | cd01cba1c7eda58bdabaae174c78c067325803d2.1689803636.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: rawnand: fsl_upm: Fix an off-by one test in fun_exec_op() | expand |
On Wed, Jul 19, 2023 at 11:55:01PM +0200, Christophe JAILLET wrote: > 'op-cs' is copied in 'fun->mchip_number' which is used to access the > 'mchip_offsets' and the 'rnb_gpio' arrays. > These arrays have NAND_MAX_CHIPS elements, so the index must be below this > limit. > > Fix the sanity check in order to avoid the NAND_MAX_CHIPS value. This > would lead to out-of-bound accesses. > > Fixes: 54309d657767 ("mtd: rawnand: fsl_upm: Implement exec_op()") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> Good eye. I'm assuming you did something like this: #!/bin/bash FILE=$1 WORDS=$(cat $FILE | perl -ne 'if ($_ =~ /\[([\w_]+)\];/) { print "$1\n" }' | sort -u) for i in $WORDS ; do grep -Hn " > $i" $FILE done regards, dan carpenter
Le 20/07/2023 à 09:04, Dan Carpenter a écrit : > On Wed, Jul 19, 2023 at 11:55:01PM +0200, Christophe JAILLET wrote: >> 'op-cs' is copied in 'fun->mchip_number' which is used to access the >> 'mchip_offsets' and the 'rnb_gpio' arrays. >> These arrays have NAND_MAX_CHIPS elements, so the index must be below this >> limit. >> >> Fix the sanity check in order to avoid the NAND_MAX_CHIPS value. This >> would lead to out-of-bound accesses. >> >> Fixes: 54309d657767 ("mtd: rawnand: fsl_upm: Implement exec_op()") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- > > Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> > > Good eye. I'm assuming you did something like this: > > #!/bin/bash > > FILE=$1 > WORDS=$(cat $FILE | perl -ne 'if ($_ =~ /\[([\w_]+)\];/) { print "$1\n" }' | sort -u) > for i in $WORDS ; do > grep -Hn " > $i" $FILE > done > > regards, > dan carpenter > I did with the help of a coccinelle script. (which can certainly be improved, so adding Julia in cc: :) ) This gives decent results. I only post patches for what I confirm as an issue. So it is likely that some good candidates will be left as-is. So if anyone wants to give it a try, you are welcomed :) CJ cocci: ===== @rule1@ identifier C =~ "[A-Z]+"; expression e; statement S; @@ if (e < C) S @ok1 depends on rule1@ identifier C = rule1.C; expression e; statement S; @@ * if (e <= C) S @rule2@ identifier C =~ "[A-Z]+"; expression e; statement S; @@ if (e <= C) S @ok2 depends on rule2@ identifier C = rule2.C; expression e; statement S; @@ * if (e < C) S @rule3@ identifier C =~ "[A-Z]+"; expression e; statement S; @@ if (e > C) S @ok3 depends on rule3@ identifier C = rule3.C; expression e; statement S; @@ * if (e >= C) S @rule4@ identifier C =~ "[A-Z]+"; expression e; statement S; @@ if (e >= C) S @ok4 depends on rule4@ identifier C = rule4.C; expression e; statement S; @@ * if (e > C) S CJ
On Thu, Jul 20, 2023 at 08:27:28PM +0200, Christophe JAILLET wrote: > Le 20/07/2023 à 09:04, Dan Carpenter a écrit : > > On Wed, Jul 19, 2023 at 11:55:01PM +0200, Christophe JAILLET wrote: > > > 'op-cs' is copied in 'fun->mchip_number' which is used to access the > > > 'mchip_offsets' and the 'rnb_gpio' arrays. > > > These arrays have NAND_MAX_CHIPS elements, so the index must be below this > > > limit. > > > > > > Fix the sanity check in order to avoid the NAND_MAX_CHIPS value. This > > > would lead to out-of-bound accesses. > > > > > > Fixes: 54309d657767 ("mtd: rawnand: fsl_upm: Implement exec_op()") > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > > --- > > > > Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> > > > > Good eye. I'm assuming you did something like this: > > > > #!/bin/bash > > > > FILE=$1 > > WORDS=$(cat $FILE | perl -ne 'if ($_ =~ /\[([\w_]+)\];/) { print "$1\n" }' | sort -u) > > for i in $WORDS ; do > > grep -Hn " > $i" $FILE > > done > > > > regards, > > dan carpenter > > > > I did with the help of a coccinelle script. > (which can certainly be improved, so adding Julia in cc: :) ) Harshit and I were mucking with this about yesterday. He made Coccinelle script as well. We found three bugs in max9286_parse_dt(), hisi_inno_phy_probe() and jent_testing_store(). Only the one in hisi_inno_phy_probe() is real life bad because the others do an out of bounds check followed by a mask. regards, dan carpenter
On Fri, Jul 21, 2023 at 08:37:02AM +0300, Dan Carpenter wrote: > Harshit and I were mucking with this about yesterday. He made > Coccinelle script as well. > > We found three bugs in max9286_parse_dt(), hisi_inno_phy_probe() and > jent_testing_store(). Only the one in hisi_inno_phy_probe() is real > life bad because the others do an out of bounds check followed by a > mask. Actually wait. hisi_inno_phy_probe() is not a bug at all. MAX9286_NUM_GMSL and MAX9286_NUM_GMSL are both 4. I was wondering why Smatch didn't find this bug. regards, dan carpenter
On Fri, Jul 21, 2023 at 09:55:02AM +0300, Dan Carpenter wrote: > On Fri, Jul 21, 2023 at 08:37:02AM +0300, Dan Carpenter wrote: > > Harshit and I were mucking with this about yesterday. He made > > Coccinelle script as well. > > > > We found three bugs in max9286_parse_dt(), hisi_inno_phy_probe() and > > jent_testing_store(). Only the one in hisi_inno_phy_probe() is real > > life bad because the others do an out of bounds check followed by a > > mask. > > Actually wait. hisi_inno_phy_probe() is not a bug at all. > MAX9286_NUM_GMSL and MAX9286_NUM_GMSL are both 4. Gar, wow. hisi_inno_phy_probe() is a bug. max9286_parse_dt() is not a bug. Smatch would have found max9286_parse_dt() if it had been a bug. The reason why smatch didn't find hisi_inno_phy_probe() is because of how Smatch takes a short cut when parsing loops. regards, dan carpenter
On Wed, 2023-07-19 at 21:55:01 UTC, Christophe JAILLET wrote: > 'op-cs' is copied in 'fun->mchip_number' which is used to access the > 'mchip_offsets' and the 'rnb_gpio' arrays. > These arrays have NAND_MAX_CHIPS elements, so the index must be below this > limit. > > Fix the sanity check in order to avoid the NAND_MAX_CHIPS value. This > would lead to out-of-bound accesses. > > Fixes: 54309d657767 ("mtd: rawnand: fsl_upm: Implement exec_op()") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks. Miquel
diff --git a/drivers/mtd/nand/raw/fsl_upm.c b/drivers/mtd/nand/raw/fsl_upm.c index f1810e2f2c07..0c67fd1347c4 100644 --- a/drivers/mtd/nand/raw/fsl_upm.c +++ b/drivers/mtd/nand/raw/fsl_upm.c @@ -135,7 +135,7 @@ static int fun_exec_op(struct nand_chip *chip, const struct nand_operation *op, unsigned int i; int ret; - if (op->cs > NAND_MAX_CHIPS) + if (op->cs >= NAND_MAX_CHIPS) return -EINVAL; if (check_only)
'op-cs' is copied in 'fun->mchip_number' which is used to access the 'mchip_offsets' and the 'rnb_gpio' arrays. These arrays have NAND_MAX_CHIPS elements, so the index must be below this limit. Fix the sanity check in order to avoid the NAND_MAX_CHIPS value. This would lead to out-of-bound accesses. Fixes: 54309d657767 ("mtd: rawnand: fsl_upm: Implement exec_op()") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/mtd/nand/raw/fsl_upm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)