[PATCH] mtd: core: reject OOB transfers starting at the device end

[PATCH] mtd: core: reject OOB transfers starting at the device end

[James Hilliard]

mtd_check_oob_ops() permits offs == mtd->size when the data length is
zero. For an OOB-only request with a nonzero ooboffs, maxooblen then
underflows because there are no pages remaining. The wrapped value lets
the request pass the OOB length check.

The OOB ioctls can reach this case by encoding the OOB offset in the low
bits of start. On a partition ending before its master device, the
subsequent address translation allows the request to read or write OOB
in the page immediately following the partition.

Reject nonempty OOB transfers starting at or beyond the end of the MTD
device before calculating the remaining OOB capacity. Keep empty
requests and data-only operations unchanged.

Fixes: d82c36821684 ("mtd: Fix mtd_check_oob_ops()")
Cc: stable@vger.kernel.org
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 drivers/mtd/mtdcore.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 491a27d0315f..b82fd7ccd111 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1676,6 +1676,9 @@ static int mtd_check_oob_ops(struct mtd_info *mtd, loff_t offs,
 	if (ops->ooblen) {
 		size_t maxooblen;
 
+		if (offs >= mtd->size)
+			return -EINVAL;
+
 		if (ops->ooboffs >= mtd_oobavail(mtd, ops))
 			return -EINVAL;
 

---
base-commit: 1c1a342aceec79528b3ba51f376eca2be5928fe7
change-id: 20261104-submit-mtd-oob-end-v1-f4dc6fbfcfca

Best regards,
--  
James Hilliard <james.hilliard1@gmail.com>


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

Re: [PATCH] mtd: core: reject OOB transfers starting at the device end

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mtd: core: maxooblen underflow in mtd_check_oob_ops() on unaligned size
--

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

mtd: core: reject OOB transfers starting at the device end

This commit updates mtd_check_oob_ops() to reject nonempty OOB transfers
starting at or beyond mtd->size so that maxooblen does not underflow when
ops->ooboffs is nonzero.

> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 491a27d0315fa..b82fd7ccd111d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1676,6 +1676,9 @@ static int mtd_check_oob_ops(struct mtd_info *mtd, loff_t offs,
>  	if (ops->ooblen) {
>  		size_t maxooblen;
>  
> +		if (offs >= mtd->size)
> +			return -EINVAL;

[Severity: Medium]
Can maxooblen still underflow in mtd_check_oob_ops() when mtd->size is not
aligned to mtd->writesize?

In drivers/mtd/mtdpart.c, allocate_partition() allows registering read-only
partitions whose size is not a multiple of writesize:

drivers/mtd/mtdpart.c:allocate_partition() {
	...
	tmp = mtd_get_master_ofs(child, 0) + child->part.size;
	remainder = do_div(tmp, wr_alignment);
	if ((child->flags & MTD_WRITEABLE) && remainder) {
		child->flags &= ~MTD_WRITEABLE;
		printk(KERN_WARNING"mtd: partition \"%s\" doesn't end on an erase/write block -- force read-only\n",
			part->name);
	}

	child->size = child->part.size;
	...
}

And mtd_check_oob_ops() computes maxooblen using floor division via
mtd_div_by_ws():

drivers/mtd/mtdcore.c:mtd_check_oob_ops() {
	...
		if (offs >= mtd->size)
			return -EINVAL;

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

When a request arrives via mtdchar_readoob() -> mtd_read_oob() ->
mtd_check_oob_ops() with an offset in the trailing partial page
[mtd_div_by_ws(mtd->size, mtd) * mtd->writesize, mtd->size) and a nonzero
ops->ooboffs, offs < mtd->size holds, but mtd_div_by_ws(offs, mtd) equals
mtd_div_by_ws(mtd->size, mtd). Subtracting ops->ooboffs from zero then
underflows maxooblen and allows reading OOB data from the page extending
past the partition end.

Would checking mtd_div_by_ws(offs, mtd) >= mtd_div_by_ws(mtd->size, mtd)
instead of offs >= mtd->size avoid this underflow for unaligned partition
sizes as well?

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

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