[PATCH] mtd: core: always verify OOB offset in mtd_check_oob_ops()

[PATCH] mtd: core: always verify OOB offset in mtd_check_oob_ops()

[Gabor Juhos]

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@gmail.com>
---
 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@gmail.com>

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

[Daniel Golle]

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@gmail.com>

Reviewed-by: Daniel Golle <daniel@makrotopia.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@gmail.com>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Miquel Raynal]

Hi Gabor,

On 31/08/2025 at 16:40:10 +02, Gabor Juhos <j4g8y7@gmail.com> 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@gmail.com>

Thanks a lot for this contribution, I'm ready to take it. Just one
question, do you consider it should be backported? I would tend to
answer yes to this question, which would involve you sending a v2 with:

       Fixes:
       Cc: stable...

Otherwise I can take it as-is if you convince me it is not so relevant
:-)

Cheers,
Miquèl

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

[Gabor Juhos]

Hi Miquel,

2025. 09. 01. 14:39 keltezéssel, Miquel Raynal írta:

...

>> 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@gmail.com>
> 
> Thanks a lot for this contribution, I'm ready to take it. Just one
> question, do you consider it should be backported? I would tend to
> answer yes to this question, which would involve you sending a v2 with:
> 
>        Fixes:
>        Cc: stable...
> 
> Otherwise I can take it as-is if you convince me it is not so relevant
> :-)

I can't convince even myself about that, so will send a v2. :)

Regards,
Gabor