diff mbox series

mtd: rawnand: fsl_upm: Fix an off-by one test in fun_exec_op()

Message ID cd01cba1c7eda58bdabaae174c78c067325803d2.1689803636.git.christophe.jaillet@wanadoo.fr
State New
Headers
Series mtd: rawnand: fsl_upm: Fix an off-by one test in fun_exec_op() |

Commit Message

Christophe JAILLET July 19, 2023, 9:55 p.m. UTC 
  '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(-)

Comments

Dan Carpenter July 20, 2023, 7:04 a.m. UTC | #1 
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
Christophe JAILLET July 20, 2023, 6:27 p.m. UTC | #2 
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
Dan Carpenter July 21, 2023, 5:37 a.m. UTC | #3 
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
Dan Carpenter July 21, 2023, 6:55 a.m. UTC | #4 
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
Dan Carpenter July 21, 2023, 6:59 a.m. UTC | #5 
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
Miquel Raynal July 27, 2023, 3:17 p.m. UTC | #6 
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 mbox series

Patch

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)