linux-kernel - Re: [PATCH 02/19] mtd: spi-nor: swp: Improve locking user experience

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bjkye667.fsf@bootlin.com>
Date: Wed, 19 Nov 2025 10:13:36 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: "Michael Walle" <mwalle@...nel.org>
Cc: "Tudor Ambarus" <tudor.ambarus@...aro.org>,  "Pratyush Yadav"
 <pratyush@...nel.org>,  "Richard Weinberger" <richard@....at>,  "Vignesh
 Raghavendra" <vigneshr@...com>,  "Jonathan Corbet" <corbet@....net>,
  "Sean Anderson" <sean.anderson@...ux.dev>,  "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>,  "Steam Lin" <STLin2@...bond.com>,
  <linux-mtd@...ts.infradead.org>,  <linux-kernel@...r.kernel.org>,
  <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 02/19] mtd: spi-nor: swp: Improve locking user experience

On 18/11/2025 at 10:17:55 +01, "Michael Walle" <mwalle@...nel.org> wrote:

> On Fri Nov 14, 2025 at 6:53 PM CET, Miquel Raynal wrote:
>> In the case of a single block being locked, if the user want to fully
>> unlock the device it has two possibilities:
>> - either it asks to unlock the entire device, and this works;
>> - or it asks to unlock just the blocks that are currently locked, which
>> fails.
>>
>> It fails because the conditions "can_be_top" and "can_be_bottom" are
>> true. Indeed, in this case, we unlock everything, to the TB bit does not
>> matter. However in the current implementation, use_top would be true (as
>> this is the favourite option) and lock_len, which in practice should be
>> reduced down to 0, is set to "nor->params->size - (ofs + len)" which is
>> a positive number. This is wrong.
>
> This only happens if you try to unlock the first sector, correct? If
> my maths are correct, trying it on the last sector, lock_len should
> be 0, i.e in that case "ofs + len == size".
>
> If it's the first sector (or sectors), lock_len will end up with
> "size - N * 64k", which is clearly wrong.

That's it. Actually I forgot to mention it was happening only with the
first sectors, not the last ones, so yes you are correct, it matches my
maths and experiments.

>> An easy way is to simply add an extra condition. In the unlock() path,
>> if we can achieve the results from both sides, it means we unlock
>> everything and lock_len must simply be 0.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.com>
>> ---
>> For me, this result was clearly unexpected, but I am not sure this
>> qualifies as a fix.
>
> That's definetly a bug, esp. because it will lock an entire
> unrelated region. And it seems to go back all the to commit
> 3dd8012a8eeb "mtd: spi-nor: add TB (Top/Bottom) protect support").
>
>> ---
>>  drivers/mtd/spi-nor/swp.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
>> index 9b07f83aeac76dce2109f90dfa1534c9bd93330d..9bc5a356444665ad8824e9e12d679fd551b3e67d 100644
>> --- a/drivers/mtd/spi-nor/swp.c
>> +++ b/drivers/mtd/spi-nor/swp.c
>> @@ -281,7 +281,9 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
>>  	use_top = can_be_top;
>>  
>>  	/* lock_len: length of region that should remain locked */
>> -	if (use_top)
>> +	if (can_be_top && can_be_bottom)
>> +		lock_len = 0;
>
> Could you please add a comment stating that if both are true, it
> means that both adjacent regions are unlocked and thus the entire
> flash will be unlocked.

Ofc.

Thanks,
Miquèl