diff mbox series

[04/12] mtd: rawnand: brcmnand: Fix potential out-of-bounds access in oob write

Message ID 20230606231252.94838-5-william.zhang@broadcom.com
State New
Headers
Series mtd: rawnand: brcmnand: driver and doc updates |

Commit Message

William Zhang June 6, 2023, 11:12 p.m. UTC 
  When the oob buffer length is not in multiple of words, the oob write
function does out-of-bounds read on the oob source buffer at the last
iteration. Fix that by always checking length limit on the oob buffer
read and fill with 0xff when reaching the end of the buffer to the oob
registers.

Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Miquel Raynal June 7, 2023, 8:09 a.m. UTC | #1 
Hi William,

william.zhang@broadcom.com wrote on Tue,  6 Jun 2023 16:12:44 -0700:

> When the oob buffer length is not in multiple of words, the oob write
> function does out-of-bounds read on the oob source buffer at the last
> iteration. Fix that by always checking length limit on the oob buffer
> read and fill with 0xff when reaching the end of the buffer to the oob
> registers.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 20832857c4aa..d920e88c7f5b 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1486,10 +1486,10 @@ static int write_oob_to_regs(struct brcmnand_controller *ctrl, int i,
>  
>  	for (j = 0; j < tbytes; j += 4)
>  		oob_reg_write(ctrl, j,
> -				(oob[j + 0] << 24) |
> -				(oob[j + 1] << 16) |
> -				(oob[j + 2] <<  8) |
> -				(oob[j + 3] <<  0));
> +				(((j < tbytes) ? oob[j] : 0xff) << 24) |
> +				(((j + 1 < tbytes) ? oob[j + 1] : 0xff) << 16) |
> +				(((j + 2 < tbytes) ? oob[j + 2] : 0xff) << 8) |
> +				((j + 3 < tbytes) ? oob[j + 3] : 0xff));

This is a lot of additional operations which most of the time are not
relevant. I would instead got for one less iteration in the for loop
when there is unaligned data, and then dedicated if/else to fill the
missing bytes.

>  	return tbytes;
>  }
>  


Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 20832857c4aa..d920e88c7f5b 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1486,10 +1486,10 @@  static int write_oob_to_regs(struct brcmnand_controller *ctrl, int i,
 
 	for (j = 0; j < tbytes; j += 4)
 		oob_reg_write(ctrl, j,
-				(oob[j + 0] << 24) |
-				(oob[j + 1] << 16) |
-				(oob[j + 2] <<  8) |
-				(oob[j + 3] <<  0));
+				(((j < tbytes) ? oob[j] : 0xff) << 24) |
+				(((j + 1 < tbytes) ? oob[j + 1] : 0xff) << 16) |
+				(((j + 2 < tbytes) ? oob[j + 2] : 0xff) << 8) |
+				((j + 3 < tbytes) ? oob[j + 3] : 0xff));
 	return tbytes;
 }