linux-kernel - Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20201001063421.qcjdikj2tje3jn6k@ti.com>
Date:   Thu, 1 Oct 2020 12:04:21 +0530
From:   Pratyush Yadav <p.yadav@...com>
To:     Bert Vermeulen <bert@...t.com>
CC:     <tudor.ambarus@...rochip.com>, <miquel.raynal@...tlin.com>,
        <richard@....at>, <vigneshr@...com>,
        <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: Fix 3-or-4 address byte mode logic

Hi,

On 01/10/20 01:56AM, Bert Vermeulen wrote:
> Flash chips that announce BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 capability
> get an addr_width of 3. This breaks when the flash chip is actually
> larger than 16MB, since that requires a 4-byte address. The MX25L25635F
> does exactly this, breaking anything over 16MB.
> 
> spi-nor only enables 4-byte opcodes or 4-byte address mode if addr_width
> is 4, so no 4-byte mode is ever enabled. The > 16MB check in
> spi_nor_set_addr_width() only works if addr_width wasn't already set
> by the SFDP, which it was.
> 
> It could be fixed in a post_bfpt fixup for the MX25L25635F, but setting
> addr_width to 4 when BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 is found fixes the
> problem for all such cases.

JESD216D.01 says: "01b: 3- or 4-Byte addressing (e.g., defaults to 
3-Byte mode; enters 4-Byte mode on command)"

So using an address width of 4 here is not necessarily the right thing 
to do. This change would break SMPT parsing for all flashes that use 
3-byte addressing by default because SMPT parsing can involve register 
reads/writes. One such device is the Cypress S28HS flash. In fact, this 
was what prompted me to write the patch [0].

Before that patch, how did MX25L25635F decide to use 4-byte addressing? 
Coming out of BFPT parsing addr_width would still be 0. My guess is that 
it would go into spi_nor_set_addr_width() with addr_width still 0 and 
then the check for (nor->mtd.size > 0x1000000) would set it to 4. Do I 
guess correctly?

In that case maybe we can do a better job of deciding what gets priority 
in the if-else chain. For example, giving addr_width from nor->info 
precedence over the one configured by SFDP can solve this problem. Then 
all you have to do is set the addr_width in the info struct, which is 
certainly easier than adding a fixup hook. There may be a more elegant 
solution to this but I haven't given it much thought.

So from my side, NACK!

> 
> Signed-off-by: Bert Vermeulen <bert@...t.com>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index e2a43d39eb5f..6fedc425bcf7 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -456,10 +456,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  	/* Number of address bytes. */
>  	switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
>  	case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
> -	case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>  		nor->addr_width = 3;
>  		break;
>  
> +	case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
>  	case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
>  		nor->addr_width = 4;
>  		break;

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9acd7fa80be6ee14aecdc54429f2a48e56224e8

-- 
Regards,
Pratyush Yadav
Texas Instruments India