linux-kernel - Re: [PATCH] mtd: core: always verify OOB offset in mtd_check_oob_ops()

Open Source and information security mailing list archives

Message-ID: <aLSDB5ySaZv8yfHg@pidgin.makrotopia.org>
Date: Sun, 31 Aug 2025 18:14:47 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Gabor Juhos <j4g8y7@...il.com>
Cc: Miquel Raynal <miquel.raynal@...tlin.com>,
	Richard Weinberger <richard@....at>,
	Vignesh Raghavendra <vigneshr@...com>,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd: core: always verify OOB offset in
 mtd_check_oob_ops()

On Sun, Aug 31, 2025 at 04:40:10PM +0200, Gabor Juhos wrote:
> Using an OOB offset past end of the available OOB data is invalid,
> irregardless of whether the 'ooblen' is set in the ops or not. Move
> the relevant check out from the if statement to always verify that.
> 
> The 'oobtest' module executes four tests to verify how reading/writing
> OOB data past end of the devices is handled. It expects errors in case
> of these tests, but this expectation fails in the last two tests on
> MTD devices, which have no OOB bytes available.
> 
> This is indicated in the test output like the following:
> 
>     [  212.059416] mtd_oobtest: attempting to write past end of device
>     [  212.060379] mtd_oobtest: an error is expected...
>     [  212.066353] mtd_oobtest: error: wrote past end of device
>     [  212.071142] mtd_oobtest: attempting to read past end of device
>     [  212.076507] mtd_oobtest: an error is expected...
>     [  212.082080] mtd_oobtest: error: read past end of device
>     ...
>     [  212.330508] mtd_oobtest: finished with 2 errors
> 
> For reference, here is the corresponding code from the oobtest module:
> 
>     /* Attempt to write off end of device */
>     ops.mode      = MTD_OPS_AUTO_OOB;
>     ops.len       = 0;
>     ops.retlen    = 0;
>     ops.ooblen    = mtd->oobavail;
>     ops.oobretlen = 0;
>     ops.ooboffs   = 1;
>     ops.datbuf    = NULL;
>     ops.oobbuf    = writebuf;
>     pr_info("attempting to write past end of device\n");
>     pr_info("an error is expected...\n");
>     err = mtd_write_oob(mtd, mtd->size - mtd->writesize, &ops);
>     if (err) {
>             pr_info("error occurred as expected\n");
>     } else {
>             pr_err("error: wrote past end of device\n");
>             errcnt += 1;
>     }
> 
> As it can be seen, the code sets 'ooboffs' to 1, and 'ooblen' to
> mtd->oobavail which is zero in our case.
> 
> Since the mtd_check_oob_ops() function only verifies 'ooboffs' if 'ooblen'
> is not zero, the 'ooboffs' value does not gets validated and the function
> returns success whereas it should fail.
> 
> After the change, the oobtest module will bail out early with an error if
> there are no OOB bytes available on the MDT device under test:
> 
>     # cat /sys/class/mtd/mtd0/oobavail
>     0
>     # insmod mtd_test; insmod mtd_oobtest dev=0
>     [  943.606228]
>     [  943.606259] =================================================
>     [  943.606784] mtd_oobtest: MTD device: 0
>     [  943.612660] mtd_oobtest: MTD device size 524288, eraseblock size 131072, page size 2048, count of eraseblocks 4, pages per eraseblock 64, OOB size 128
>     [  943.616091] mtd_test: scanning for bad eraseblocks
>     [  943.629571] mtd_test: scanned 4 eraseblocks, 0 are bad
>     [  943.634313] mtd_oobtest: test 1 of 5
>     [  943.653402] mtd_oobtest: writing OOBs of whole device
>     [  943.653424] mtd_oobtest: error: writeoob failed at 0x0
>     [  943.657419] mtd_oobtest: error: use_len 0, use_offset 0
>     [  943.662493] mtd_oobtest: error -22 occurred
>     [  943.667574] =================================================
> 
> This behaviour is more accurate than the current one where most tests
> are indicating successful writing of OOB data even that in fact nothing
> gets written into the device, which is quite misleading.
> 
> Signed-off-by: Gabor Juhos <j4g8y7@...il.com>

Reviewed-by: Daniel Golle <daniel@...rotopia.org>


> ---
>  drivers/mtd/mtdcore.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 5ba9a741f5ac3c297ae21329c2827baf5dc471f0..9a3c9f163219bcb9fde66839f228fd8d38310f2d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1590,12 +1590,12 @@ static int mtd_check_oob_ops(struct mtd_info *mtd, loff_t offs,
>  	if (offs < 0 || offs + ops->len > mtd->size)
>  		return -EINVAL;
>  
> +	if (ops->ooboffs >= mtd_oobavail(mtd, ops))
> +		return -EINVAL;
> +
>  	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;
> 
> ---
> base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
> change-id: 20250831-mtd-validate-ooboffs-e35c796540fe
> 
> Best regards,
> -- 
> Gabor Juhos <j4g8y7@...il.com>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives