Re: [PATCH] mtd: core: avoid overflow when checking data bounds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mtd: core: mtd_check_oob_ops() allows offs == mtd->size and OOB underflow
--

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

mtd: core: avoid overflow when checking data bounds

This commit updates mtd_check_oob_ops() to validate the starting offset
against the device size before comparing ops->len with the remaining
capacity, avoiding an integer overflow in offs + ops->len.

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 491a27d0315fa..dda976ef52c9b 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1670,7 +1670,7 @@ static int mtd_check_oob_ops(struct mtd_info *mtd, loff_t offs,
>  	if (!ops->oobbuf)
>  		ops->ooblen = 0;
>  
> -	if (offs < 0 || offs + ops->len > mtd->size)
> +	if (offs < 0 || offs > mtd->size || ops->len > mtd->size - offs)
>  		return -EINVAL;

[Severity: High]
Does allowing offs == mtd->size when ops->len == 0 here leave two
issues when requests are made at the exact end of the device?

First, for an OOB-only request (ops->datbuf == NULL, ops->len == 0,
ops->ooblen > 0) at offs == mtd->size, execution enters the if (ops->ooblen)
block in mtd_check_oob_ops():

	if (ops->ooblen) {
		size_t maxooblen;

		if (ops->ooboffs >= mtd_oobavail(mtd, ops))
			return -EINVAL;

		maxooblen = ((size_t)(mtd_div_by_ws(mtd->size, mtd) -
				      mtd_div_by_ws(offs, mtd)) *
			     mtd_oobavail(mtd, ops)) - ops->ooboffs;
		if (ops->ooblen > maxooblen)
			return -EINVAL;
	}

When offs == mtd->size, mtd_div_by_ws(mtd->size, mtd) - mtd_div_by_ws(offs, mtd)
is 0. If 0 < ops->ooboffs < mtd_oobavail(mtd, ops), subtracting ops->ooboffs
from 0 wraps the unsigned size_t maxooblen to SIZE_MAX - ops->ooboffs + 1.
The ops->ooblen > maxooblen check then fails to reject the request, and
mtd_read_oob() or mtd_write_oob() forwards an out-of-range OOB operation at
offs == mtd->size to the driver. This can be triggered from userspace via
mtdchar_readoob() or mtdchar_writeoob() when start == mtd->size + 1 and
length == 1:

	ops.ooblen = length;
	ops.ooboffs = start & (mtd->writesize - 1);
	ops.datbuf = NULL;
	...
	start &= ~((uint64_t)mtd->writesize - 1);
	ret = mtd_write_oob(mtd, start, &ops);

Second, even when both ops->len == 0 and ops->ooblen == 0 at offs == mtd->size,
neither mtd_check_oob_ops() nor mtd_read_oob() / mtd_write_oob() short-circuits
zero-length requests before calling into the driver via mtd_read_oob_std() or
mtd_write_oob_std(). Drivers such as nand_do_read_ops() and nand_do_read_oob()
in drivers/mtd/nand/raw/nand_base.c immediately compute the target chip from
the offset before checking the length:

	chipnr = (int)(from >> chip->chip_shift);
	nand_select_target(chip, chipnr);

When from == mtd->size on the master device, chipnr equals
nanddev_ntargets(&chip->base), triggering the WARN_ON in nand_select_target()
and issuing an out-of-bounds page operation.

Should mtd_check_oob_ops() reject offs >= mtd->size (or short-circuit empty
operations in mtd_read_oob() and mtd_write_oob() while rejecting
offs >= mtd->size for any non-empty data or OOB request), similar to how
mtd_point(), mtd_unpoint(), and mtd_panic_write() check offs >= mtd->size?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-submit-mtd-oob-bounds-v1-v1-1-05209fce5a3e@gmail.com?part=1

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