Re: [PATCH v3 4/5] mtd: rawnand: brcmnand: Fix potential out-of-bounds access in oob write

Miquel Raynal
2023-07-04T15:28:33Z

Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:37 -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")

Wrong Fixes.

Missing Cc stable.

> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
>
> ---
>
> Changes in v3:
> - Fix kernel test robot sparse warning:
> drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: expected unsigned int [usertype] data
> drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: got restricted __be32 [usertype]
>
> Changes in v2:
> - Handle the remaining unaligned oob data after the oob data write loop
>
> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index ea03104692bf..407bf79cbaf4 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1477,19 +1477,28 @@ static int write_oob_to_regs(struct brcmnand_controller *ctrl, int i,
> const u8 *oob, int sas, int sector_1k)
> {
> int tbytes = sas << sector_1k;
> - int j;
> + int j, k = 0;
> + u32 last = 0xffffffff;
> + u8 *plast = (u8 *)&last;
>
> /* Adjust OOB values for 1K sector size */
> if (sector_1k && (i & 0x01))
> tbytes = max(0, tbytes - (int)ctrl->max_oob);
> tbytes = min_t(int, tbytes, ctrl->max_oob);
>
> - for (j = 0; j < tbytes; j += 4)
> + for (j = 0; (j + 3) < tbytes; j += 4)

Maybe a comment here as well to mention that you stop at the last
iteration? Otherwise, just reading the line does not make you choice
obvious.

> oob_reg_write(ctrl, j,
> (oob[j + 0] << 24) |
> (oob[j + 1] << 16) |
> (oob[j + 2] << 8) |
> (oob[j + 3] << 0));
> +
> + while (j < tbytes)
> + plast[k++] = oob[j++];
> +
> + if (tbytes & 0x3)
> + oob_reg_write(ctrl, (tbytes & ~0x3), (__force u32)cpu_to_be32(last));
> +
> return tbytes;
> }
>

Thanks,
Miquèl