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: <DEBPI49KKW00.3MSWMX9HQL7JZ@kernel.org>
Date: Tue, 18 Nov 2025 10:17:55 +0100
From: "Michael Walle" <mwalle@...nel.org>
To: "Miquel Raynal" <miquel.raynal@...tlin.com>, "Tudor Ambarus"
 <tudor.ambarus@...aro.org>, "Pratyush Yadav" <pratyush@...nel.org>,
 "Richard Weinberger" <richard@....at>, "Vignesh Raghavendra"
 <vigneshr@...com>, "Jonathan Corbet" <corbet@....net>
Cc: "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 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.

> 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.

-michael

> +	else if (use_top)
>  		lock_len = nor->params->size - (ofs + len);
>  	else
>  		lock_len = ofs;


Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)