Re: [PATCH] mtd: rawnand: nandsim: validate overridesize before shifting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] mtd: rawnand: nandsim: unsigned integer overflow in
  ns_init() [1]
--

commit 172aea198c2ee0259ce74734f4b5dab1d08fa195
Author: James Hilliard <james.hilliard1@gmail.com>

mtd: rawnand: nandsim: validate overridesize before shifting

This commit validates the overridesize shift count using
check_shl_overflow() before computing the eraseblock count and total MTD
device size.

> diff --git a/drivers/mtd/nand/raw/nandsim.c b/drivers/mtd/nand/raw/nandsim.c
> index fe968037f75a2..6716eafb4b004 100644
> --- a/drivers/mtd/nand/raw/nandsim.c
> +++ b/drivers/mtd/nand/raw/nandsim.c

[ ... ]

> @@ -2341,21 +2342,21 @@ static int __init ns_init_module(void)
>  	}
>  
>  	if (overridesize) {
> -		uint64_t new_size = (uint64_t)nsmtd->erasesize << overridesize;
>  		struct nand_memory_organization *memorg;
> +		unsigned int eraseblocks;
>  		u64 targetsize;
>  
>  		memorg = nanddev_get_memorg(&chip->base);
>  
> -		if (new_size >> overridesize != nsmtd->erasesize) {
> +		if (check_shl_overflow(1U, overridesize, &eraseblocks)) {
>  			NS_ERR("overridesize is too big\n");
>  			ret = -EINVAL;
>  			goto cleanup_nand;
>  		}
>  
>  		/* N.B. This relies on nand_scan not doing anything with the size before we change it */
> -		nsmtd->size = new_size;
> -		memorg->eraseblocks_per_lun = 1 << overridesize;
> +		nsmtd->size = (u64)nsmtd->erasesize * eraseblocks;
> +		memorg->eraseblocks_per_lun = eraseblocks;

[Severity: Critical]
This problem wasn't introduced by this patch, but can a large
overridesize that passes check_shl_overflow() still overflow the 32-bit
ns->geom.pgnum in ns_init() [1]?

drivers/mtd/nand/raw/nandsim.c:ns_init() {
	...
	ns->geom.totsz    = mtd->size;
	ns->geom.pgsz     = mtd->writesize;
	...
	ns->geom.pgnum    = div_u64(ns->geom.totsz, ns->geom.pgsz);
	...
}

Because each eraseblock contains multiple pages, an eraseblock count that
fits in unsigned int can still produce a total page count exceeding
UINT_MAX.  Truncating ns->geom.pgnum causes ns_init() to allocate a
smaller page array than mtd->size requires, leading to out-of-bounds
accesses during simulated flash operations.

[1] https://sashiko.dev/#/bug/linux-3c379a73-65df-4b5f-8179-95ceb2f44717

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-submit-nandsim-overridesize-bound-v1-v1-1-9cde7657a69b@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/