diff mbox series

[v2] mtd: cfi_cmdset_0001: Byte swap OTP info

Message ID 20230601194123.3408902-1-linus.walleij@linaro.org
State New
Delegated to: Vignesh R
Headers show
Series [v2] mtd: cfi_cmdset_0001: Byte swap OTP info | expand

Commit Message

Linus Walleij June 1, 2023, 7:41 p.m. UTC
Currently the offset into the device when looking for OTP
bits can go outside of the address of the MTD NOR devices,
and if that memory isn't readable, bad things happen
on the IXP4xx (added prints that illustrate the problem before
the crash):

cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x00000100
ixp4xx_copy_from copy from 0x00000100 to 0xc880dd78
cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x12000000
ixp4xx_copy_from copy from 0x12000000 to 0xc880dd78
8<--- cut here ---
Unable to handle kernel paging request at virtual address db000000
[db000000] *pgd=00000000
(...)

This happens in this case because the IXP4xx is big endian and
the 32- and 16-bit fields in the struct cfi_intelext_otpinfo are not
properly byteswapped. Compare to how the code in read_pri_intelext()
byteswaps the fields in struct cfi_pri_intelext.

Adding some byte swapping after casting the &extp->extra[0] into
a struct cfi_intelext_otpinfo * pointer, and the crash goes away.

The problem went unnoticed for many years until I enabled
CONFIG_MTD_OTP on the IXP4xx as well, triggering the bug.

Cc: Nicolas Pitre <npitre@baylibre.com>
Cc: stable@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Drill deeper and discover a big endian compatibility issue.
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Nicolas Pitre June 1, 2023, 9:22 p.m. UTC | #1
On Thu, 1 Jun 2023, Linus Walleij wrote:

> Currently the offset into the device when looking for OTP
> bits can go outside of the address of the MTD NOR devices,
> and if that memory isn't readable, bad things happen
> on the IXP4xx (added prints that illustrate the problem before
> the crash):
> 
> cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x00000100
> ixp4xx_copy_from copy from 0x00000100 to 0xc880dd78
> cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x12000000
> ixp4xx_copy_from copy from 0x12000000 to 0xc880dd78
> 8<--- cut here ---
> Unable to handle kernel paging request at virtual address db000000
> [db000000] *pgd=00000000
> (...)
> 
> This happens in this case because the IXP4xx is big endian and
> the 32- and 16-bit fields in the struct cfi_intelext_otpinfo are not
> properly byteswapped. Compare to how the code in read_pri_intelext()
> byteswaps the fields in struct cfi_pri_intelext.

DOH!

And to confirm, in include/linux/mtd/cfi.h we can see:

/* NB: We keep these structures in memory in HOST byteorder, except
 * where individually noted.
 */

> Adding some byte swapping after casting the &extp->extra[0] into
> a struct cfi_intelext_otpinfo * pointer, and the crash goes away.

But this is wrong to do so in cfi_intelext_otp_walk(). That function is 
a helper applying given operation callback on given range. Each time it 
is called those values will be swapped back and forth. You want to do 
the swap only once during init in read_pri_intelext().

Something like this (completely untested):

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 54f92d09d9..723dd6473c 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -421,9 +421,20 @@ read_pri_intelext(struct map_info *map, __u16 adr)
 		extra_size = 0;
 
 		/* Protection Register info */
-		if (extp->NumProtectionFields)
+		if (extp->NumProtectionFields) {
+			struct cfi_intelext_otpinfo *otp =
+				(struct cfi_intelext_otpinfo *)&extp->extra[0]; 
+
 			extra_size += (extp->NumProtectionFields - 1) *
 				      sizeof(struct cfi_intelext_otpinfo);
+			if (extp_size >= sizeof(*extp) + extra_size) {
+				for (i = 0; i < extp->NumProtectionFields - 1; i++) {
+					otp->ProtRegAddr = le32_to_cpu(otp->ProtRegAddr);
+					otp->FactGroups = le16_to_cpu(otp->FactGroups);
+					otp->UserGroups = le16_to_cpu(otp->UserGroups);
+				}
+			}
+		}
 	}
 
 	if (extp->MinorVersion >= '1') {
Nicolas Pitre June 1, 2023, 9:26 p.m. UTC | #2
On Thu, 1 Jun 2023, Nicolas Pitre wrote:

> On Thu, 1 Jun 2023, Linus Walleij wrote:
> 
> > Currently the offset into the device when looking for OTP
> > bits can go outside of the address of the MTD NOR devices,
> > and if that memory isn't readable, bad things happen
> > on the IXP4xx (added prints that illustrate the problem before
> > the crash):
> > 
> > cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x00000100
> > ixp4xx_copy_from copy from 0x00000100 to 0xc880dd78
> > cfi_intelext_otp_walk walk OTP on chip 0 start at reg_prot_offset 0x12000000
> > ixp4xx_copy_from copy from 0x12000000 to 0xc880dd78
> > 8<--- cut here ---
> > Unable to handle kernel paging request at virtual address db000000
> > [db000000] *pgd=00000000
> > (...)
> > 
> > This happens in this case because the IXP4xx is big endian and
> > the 32- and 16-bit fields in the struct cfi_intelext_otpinfo are not
> > properly byteswapped. Compare to how the code in read_pri_intelext()
> > byteswaps the fields in struct cfi_pri_intelext.
> 
> DOH!
> 
> And to confirm, in include/linux/mtd/cfi.h we can see:
> 
> /* NB: We keep these structures in memory in HOST byteorder, except
>  * where individually noted.
>  */
> 
> > Adding some byte swapping after casting the &extp->extra[0] into
> > a struct cfi_intelext_otpinfo * pointer, and the crash goes away.
> 
> But this is wrong to do so in cfi_intelext_otp_walk(). That function is 
> a helper applying given operation callback on given range. Each time it 
> is called those values will be swapped back and forth. You want to do 
> the swap only once during init in read_pri_intelext().
> 
> Something like this (completely untested):
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index 54f92d09d9..723dd6473c 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -421,9 +421,20 @@ read_pri_intelext(struct map_info *map, __u16 adr)
>  		extra_size = 0;
>  
>  		/* Protection Register info */
> -		if (extp->NumProtectionFields)
> +		if (extp->NumProtectionFields) {
> +			struct cfi_intelext_otpinfo *otp =
> +				(struct cfi_intelext_otpinfo *)&extp->extra[0]; 
> +
>  			extra_size += (extp->NumProtectionFields - 1) *
>  				      sizeof(struct cfi_intelext_otpinfo);
> +			if (extp_size >= sizeof(*extp) + extra_size) {
> +				for (i = 0; i < extp->NumProtectionFields - 1; i++) {
> +					otp->ProtRegAddr = le32_to_cpu(otp->ProtRegAddr);
> +					otp->FactGroups = le16_to_cpu(otp->FactGroups);
> +					otp->UserGroups = le16_to_cpu(otp->UserGroups);

---> plus the missing otp++ here;

> +				}
> +			}
> +		}
>  	}
>  
>  	if (extp->MinorVersion >= '1') {
>
diff mbox series

Patch

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 54f92d09d9cf..7603b0052a16 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2336,6 +2336,11 @@  static int cfi_intelext_otp_walk(struct mtd_info *mtd, loff_t from, size_t len,
 		chip = &cfi->chips[chip_num];
 		otp = (struct cfi_intelext_otpinfo *)&extp->extra[0];
 
+		/* Do some byteswapping if necessary */
+		otp->ProtRegAddr = le32_to_cpu(otp->ProtRegAddr);
+		otp->FactGroups = le16_to_cpu(otp->FactGroups);
+		otp->UserGroups = le16_to_cpu(otp->UserGroups);
+
 		/* first OTP region */
 		field = 0;
 		reg_prot_offset = extp->ProtRegAddr;