Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 4 Jul 2023 17:28:17 +0200
From:   Miquel Raynal <miquel.raynal@bootlin.com>
To:     William Zhang <william.zhang@broadcom.com>
Cc:     Broadcom Kernel List <bcm-kernel-feedback-list@broadcom.com>,
        Linux MTD List <linux-mtd@lists.infradead.org>,
        f.fainelli@gmail.com, rafal@milecki.pl, kursad.oney@broadcom.com,
        joel.peshkin@broadcom.com, computersforpeace@gmail.com,
        anand.gore@broadcom.com, dregan@mail.com, kamal.dasu@broadcom.com,
        tomer.yacoby@broadcom.com, dan.beygelman@broadcom.com,
        Florian Fainelli <florian.fainelli@broadcom.com>,
        linux-kernel@vger.kernel.org,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Richard Weinberger <richard@nod.at>,
        Kamal Dasu <kdasu.kdev@gmail.com>
Subject: Re: [PATCH v3 4/5] mtd: rawnand: brcmnand: Fix potential
 out-of-bounds access in oob write
Message-ID: <20230704172817.02e4cdb8@xps-13>
In-Reply-To: <20230627193738.19596-5-william.zhang@broadcom.com>
References: <20230627193738.19596-1-william.zhang@broadcom.com>
        <20230627193738.19596-5-william.zhang@broadcom.com>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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.
>=20
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom S=
TB NAND controller")

Wrong Fixes.

Missing Cc stable.

> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
>=20
> ---
>=20
> Changes in v3:
> - Fix kernel test robot sparse warning:
>    drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: expected uns=
igned int [usertype] data
>    drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: got restrict=
ed __be32 [usertype]
>=20
> Changes in v2:
> - Handle the remaining unaligned oob data after the oob data write loop
>=20
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>=20
> 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_cont=
roller *ctrl, int i,
>  			     const u8 *oob, int sas, int sector_1k)
>  {
>  	int tbytes =3D sas << sector_1k;
> -	int j;
> +	int j, k =3D 0;
> +	u32 last =3D 0xffffffff;
> +	u8 *plast =3D (u8 *)&last;
> =20
>  	/* Adjust OOB values for 1K sector size */
>  	if (sector_1k && (i & 0x01))
>  		tbytes =3D max(0, tbytes - (int)ctrl->max_oob);
>  	tbytes =3D min_t(int, tbytes, ctrl->max_oob);
> =20
> -	for (j =3D 0; j < tbytes; j +=3D 4)
> +	for (j =3D 0; (j + 3) < tbytes; j +=3D 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++] =3D oob[j++];
> +
> +	if (tbytes & 0x3)
> +		oob_reg_write(ctrl, (tbytes & ~0x3), (__force u32)cpu_to_be32(last));
> +
>  	return tbytes;
>  }
> =20


Thanks,
Miqu=C3=A8l