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

From: Pratyush Yadav <pratyush@kernel.org>
To: Gabor Juhos <j4g8y7@gmail.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	 Santhosh Kumar K <s-k6@ti.com>,
	 Richard Weinberger <richard@nod.at>,
	 Vignesh Raghavendra <vigneshr@ti.com>,
	 linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	 stable@vger.kernel.org,  Daniel Golle <daniel@makrotopia.org>
Subject: Re: [PATCH v2] mtd: core: always verify OOB offset in mtd_check_oob_ops()
Date: Thu, 11 Sep 2025 15:03:58 +0200	[thread overview]
Message-ID: <mafs0v7lpi1j5.fsf@kernel.org> (raw)
In-Reply-To: <a208824c-acf6-4a48-8fde-f9926a6e4db5@gmail.com>

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

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