drivers/mtd/mtdcore.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
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.
Cc: stable@vger.kernel.org
Fixes: 5cdd929da53d ("mtd: Add sanity checks in mtd_write/read_oob()")
Reviewed-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
Changes in v2:
- add Reviewed-by tag from Daniel
- add stable and Fixes tags
- Link to v1: https://lore.kernel.org/r/20250831-mtd-validate-ooboffs-v1-1-d3fdce7a8698@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>On Mon, 01 Sep 2025 16:24:35 +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.
>
> [...]
Applied to mtd/next, thanks!
[1/1] mtd: core: always verify OOB offset in mtd_check_oob_ops()
commit: bf7d0543b2602be5cb450d8ec5a8710787806f88
Patche(s) should be available on mtd/linux.git and will be
part of the next PR (provided that no robot complains by then).
Kind regards,
Miquèl
Hello, On 05/09/25 20:25, Miquel Raynal wrote: > On Mon, 01 Sep 2025 16:24:35 +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. >> >> [...] > > Applied to mtd/next, thanks! > > [1/1] mtd: core: always verify OOB offset in mtd_check_oob_ops() > commit: bf7d0543b2602be5cb450d8ec5a8710787806f88 I'm seeing a failure in SPI NOR flashes due to this patch: (Tested on AM62x SK with S28HS512T OSPI NOR flash) root@am62xx-evm:~# uname -a Linux am62xx-evm 6.17.0-rc1-00011-gbf7d0543b260 #3 SMP PREEMPT Wed Sep 10 20:44:34 IST 2025 aarch64 GNU/Linux root@am62xx-evm:~# dmesg | grep mtd [ 8.018107] I/O error, dev mtdblock6, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2 [ 8.032806] I/O error, dev mtdblock6, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2 [ 8.043229] Buffer I/O error on dev mtdblock6, logical block 0, async page read [ 8.055082] I/O error, dev mtdblock4, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2 [ 8.065883] I/O error, dev mtdblock4, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2 [ 8.075022] Buffer I/O error on dev mtdblock4, logical block 0, async page read [ 8.381213] I/O error, dev mtdblock6, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2 [ 8.394621] I/O error, dev mtdblock2, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2 [ 8.394704] I/O error, dev mtdblock2, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2 [ 8.394714] Buffer I/O error on dev mtdblock2, logical block 0, async page read [ 8.410152] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2 [ 8.456064] I/O error, dev mtdblock0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2 [ 8.465774] Buffer I/O error on dev mtdblock0, logical block 0, async page read [ 8.469771] I/O error, dev mtdblock6, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2 [ 8.469804] Buffer I/O error on dev mtdblock6, logical block 0, async page read [ 8.505866] Buffer I/O error on dev mtdblock5, logical block 0, async page read [ 8.522665] Buffer I/O error on dev mtdblock4, logical block 0, async page read [ 8.845572] Buffer I/O error on dev mtdblock3, logical block 0, async page read [ 8.855938] Buffer I/O error on dev mtdblock1, logical block 0, async page read [ 8.878292] Buffer I/O error on dev mtdblock2, logical block 0, async page read root@am62xx-evm:~# hexdump /dev/mtd6 hexdump: /dev/mtd6: Invalid argument root@am62xx-evm:~# Reverting this works fine: root@am62xx-evm:~# uname -a Linux am62xx-evm 6.17.0-rc5-next-20250910-00001-g5f216cdf2764 #5 SMP PREEMPT Thu Sep 11 11:38:06 IST 2025 aarch64 GNU/Linux root@am62xx-evm:~# dmesg | grep mtd root@am62xx-evm:~# hexdump /dev/mtd6 0000000 ffff ffff ffff ffff ffff ffff ffff ffff * 0040000 root@am62xx-evm:~# Regards, Santhosh. > > Patche(s) should be available on mtd/linux.git and will be > part of the next PR (provided that no robot complains by then). > > Kind regards, > Miquèl > >
Hello, On 11/09/2025 at 11:52:27 +0530, Santhosh Kumar K <s-k6@ti.com> wrote: > Hello, > > On 05/09/25 20:25, Miquel Raynal wrote: >> On Mon, 01 Sep 2025 16:24:35 +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. >>> >>> [...] >> Applied to mtd/next, thanks! >> [1/1] mtd: core: always verify OOB offset in mtd_check_oob_ops() >> commit: bf7d0543b2602be5cb450d8ec5a8710787806f88 > > I'm seeing a failure in SPI NOR flashes due to this patch: > (Tested on AM62x SK with S28HS512T OSPI NOR flash) Gabor, can you check what happens with mtdblock? Otherwise this will need to be reverted. Thanks, Miquèl
Hi Miquel, Santhosh, 2025. 09. 11. 10:00 keltezéssel, Miquel Raynal írta: > Hello, > > On 11/09/2025 at 11:52:27 +0530, Santhosh Kumar K <s-k6@ti.com> wrote: > >> Hello, >> >> On 05/09/25 20:25, Miquel Raynal wrote: >>> On Mon, 01 Sep 2025 16:24:35 +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. >>>> >>>> [...] >>> Applied to mtd/next, thanks! >>> [1/1] mtd: core: always verify OOB offset in mtd_check_oob_ops() >>> commit: bf7d0543b2602be5cb450d8ec5a8710787806f88 >> >> I'm seeing a failure in SPI NOR flashes due to this patch: >> (Tested on AM62x SK with S28HS512T OSPI NOR flash) Sorry for the inconvenience. > Gabor, can you check what happens with mtdblock? The strange thing is that it works with (SPI) NAND flashes: # cat /sys/class/mtd/mtd0/type nand # cat /sys/class/mtd/mtd0/oobavail 0 # # hexdump -n 2048 /dev/mtd0 0000000 0f0f 0f0f 0f0f 0f0f 0f0f 0f0f 0f0f 0f0f * 0000800 # I will check why it fails with NOR devices. > Otherwise this will need to be reverted. Please drop the patch for now, or revert it if dropping not possible. Either I will send a fixed version, or we will have to live without the change. Regards, Gabor
Hello, >>>> Applied to mtd/next, thanks! >>>> [1/1] mtd: core: always verify OOB offset in mtd_check_oob_ops() >>>> commit: bf7d0543b2602be5cb450d8ec5a8710787806f88 >>> >>> I'm seeing a failure in SPI NOR flashes due to this patch: >>> (Tested on AM62x SK with S28HS512T OSPI NOR flash) > > Sorry for the inconvenience. > >> Gabor, can you check what happens with mtdblock? > > The strange thing is that it works with (SPI) NAND flashes: > > # cat /sys/class/mtd/mtd0/type > nand > # cat /sys/class/mtd/mtd0/oobavail > 0 > # > # hexdump -n 2048 /dev/mtd0 This is not mtdblock, the report was using mtdblock, not mtd directly. I don't know if that actually makes a difference, but it is worth the try. Santhosh, please send a revert for now. Thanks, Miquèl
On Thu, Sep 11 2025, Gabor Juhos wrote: > Hi Miquel, Santhosh, > > 2025. 09. 11. 10:00 keltezéssel, Miquel Raynal írta: >> Hello, >> >> On 11/09/2025 at 11:52:27 +0530, Santhosh Kumar K <s-k6@ti.com> wrote: >> >>> Hello, >>> >>> On 05/09/25 20:25, Miquel Raynal wrote: >>>> On Mon, 01 Sep 2025 16:24:35 +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. >>>>> >>>>> [...] >>>> Applied to mtd/next, thanks! >>>> [1/1] mtd: core: always verify OOB offset in mtd_check_oob_ops() >>>> commit: bf7d0543b2602be5cb450d8ec5a8710787806f88 >>> >>> I'm seeing a failure in SPI NOR flashes due to this patch: >>> (Tested on AM62x SK with S28HS512T OSPI NOR flash) > > Sorry for the inconvenience. > >> Gabor, can you check what happens with mtdblock? My guess from a quick look at the code is that NOR devices have mtd->oobsize == 0 and mtd_read() sets ops->ooboffs and ops->ooblen to 0. So now that this check is not guarded by if (ops->ooblen), it gets triggered for NOR devices on the mtd_read() path and essentially turns into an if (0 >= 0), returning -EINVAL. Maybe a better check is if ((ops->ooboffs + ops->ooblen) > mtd_oobavail())? Note that the equality is not an error in this case. I haven't worked with the OOB code much so I am not sure if this condition makes sense, but seems to do so at first glance at least. [...] -- Regards, Pratyush Yadav
>> Sorry for the inconvenience. >> >>> Gabor, can you check what happens with mtdblock? > > My guess from a quick look at the code is that NOR devices have > mtd->oobsize == 0 and mtd_read() sets ops->ooboffs and ops->ooblen to 0. > So now that this check is not guarded by if (ops->ooblen), it gets > triggered for NOR devices on the mtd_read() path and essentially turns > into an if (0 >= 0), returning -EINVAL. > > Maybe a better check is if ((ops->ooboffs + ops->ooblen) > > mtd_oobavail())? Interesting, might make sense to do it this way. Thanks Pratyush for the suggestion, it is worth the try. Miquèl
On 11/09/2025 at 16:05:31 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote: >>> Sorry for the inconvenience. >>> >>>> Gabor, can you check what happens with mtdblock? >> >> My guess from a quick look at the code is that NOR devices have >> mtd->oobsize == 0 and mtd_read() sets ops->ooboffs and ops->ooblen to 0. >> So now that this check is not guarded by if (ops->ooblen), it gets >> triggered for NOR devices on the mtd_read() path and essentially turns >> into an if (0 >= 0), returning -EINVAL. >> >> Maybe a better check is if ((ops->ooboffs + ops->ooblen) > >> mtd_oobavail())? > > Interesting, might make sense to do it this way. > > Thanks Pratyush for the suggestion, it is worth the try. I actually have another patch series to remove and I don't have more time to dedicate to these issues at the moment, so I will force push and drop all the problematic patches. More testing is needed. Thanks, Miquèl
© 2016 - 2025 Red Hat, Inc.