diff mbox series

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

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

Commit Message

William Zhang June 27, 2023, 7:37 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>

---

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(-)

Comments

Miquel Raynal July 4, 2023, 3:28 p.m. UTC | #1 
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
William Zhang July 5, 2023, 12:43 a.m. UTC | #2 
Hi Miquel,

On 07/04/2023 08:28 AM, Miquel Raynal wrote:
> 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.
Same here.  The function write_oob_to_regs was added by this commit and 
no change since then.


> 
> Missing Cc stable.
Will add
> 
>> 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.
> 
Will add comment.

>>   		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
>
diff mbox series

Patch

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)
 		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;
 }