diff mbox series

[v4] mtd: cfi_cmdset_0001: Byte swap OTP info

Message ID 20231020-mtd-otp-byteswap-v4-1-0d132c06aa9d@linaro.org
State Accepted
Headers show
Series [v4] mtd: cfi_cmdset_0001: Byte swap OTP info | expand

Commit Message

Linus Walleij Oct. 20, 2023, 8:30 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 a small byte swapping loop for the OTP in read_pri_intelext()
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: stable@vger.kernel.org
Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Collected Nico's ACK.
- Stalled since june! Has this been missed?
ChangeLog v2->v3:
- Link to v3:
  https://lore.kernel.org/linux-mtd/20230602204359.3493320-1-linus.walleij@linaro.org/
- Move the byte swapping to a small loop in read_pri_intelext()
  so all bytes are swapped as we reach cfi_intelext_otp_walk().
ChangeLog v1->v2:
- Drill deeper and discover a big endian compatibility issue.
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)


---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20231020-mtd-otp-byteswap-7ba47316ed73

Best regards,

Comments

Miquel Raynal Oct. 23, 2023, 8:25 a.m. UTC | #1 
Hi Linus,

linus.walleij@linaro.org wrote on Fri, 20 Oct 2023 22:30:29 +0200:

> 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 a small byte swapping loop for the OTP in read_pri_intelext()
> 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: stable@vger.kernel.org

Would you like to add a Fixes tag as well? Or is this skipped on
purpose?

> Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v3->v4:
> - Collected Nico's ACK.
> - Stalled since june! Has this been missed?

Our current organization relies on Vignesh to pick-up (or tell me to
pick-up) cfi patches. But he is slightly less active these days, so if I
don't get any feedback from him soon I will take it for the next merge
window.

Sorry for the delay anyway.

Cheers,
Miquèl
Linus Walleij Oct. 23, 2023, 8:35 a.m. UTC | #2 
On Mon, Oct 23, 2023 at 10:25 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:

> linus.walleij@linaro.org wrote on Fri, 20 Oct 2023 22:30:29 +0200:
>
> > 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 a small byte swapping loop for the OTP in read_pri_intelext()
> > 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: stable@vger.kernel.org
>
> Would you like to add a Fixes tag as well? Or is this skipped on
> purpose?

Yes the actual commit predates existing git history... :/

Git blame looks like that:

^1da177e4c3f4 (Linus Torvalds         2005-04-16 15:20:36 -0700  423)
^1da177e4c3f4 (Linus Torvalds         2005-04-16 15:20:36 -0700  424)
         /* Protection Register info */
b359ed5184aeb (Jean-Philippe Brucker  2020-04-17 16:23:26 +0200  425)
         if (extp->NumProtectionFields)
b359ed5184aeb (Jean-Philippe Brucker  2020-04-17 16:23:26 +0200  426)
                 extra_size += (extp->NumProtectionFields - 1) *
b359ed5184aeb (Jean-Philippe Brucker  2020-04-17 16:23:26 +0200  427)
                               sizeof(struct cfi_intelext_otpinfo);

Jean-Philippe's patch is not the root cause AFAICT, but something
predating it, and predating git history.

The fix easily applies to all maintained stable kernels.

> > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > ChangeLog v3->v4:
> > - Collected Nico's ACK.
> > - Stalled since june! Has this been missed?
>
> Our current organization relies on Vignesh to pick-up (or tell me to
> pick-up) cfi patches. But he is slightly less active these days, so if I
> don't get any feedback from him soon I will take it for the next merge
> window.
>
> Sorry for the delay anyway.

No worries it's not like the patch is urgent.

Yours,
Linus Walleij
Miquel Raynal Oct. 23, 2023, 8:37 a.m. UTC | #3 
Hi Linus,

linus.walleij@linaro.org wrote on Mon, 23 Oct 2023 10:35:24 +0200:

> On Mon, Oct 23, 2023 at 10:25 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> 
> > linus.walleij@linaro.org wrote on Fri, 20 Oct 2023 22:30:29 +0200:
> >  
> > > 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 a small byte swapping loop for the OTP in read_pri_intelext()
> > > 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: stable@vger.kernel.org  
> >
> > Would you like to add a Fixes tag as well? Or is this skipped on
> > purpose?  
> 
> Yes the actual commit predates existing git history... :/
> 
> Git blame looks like that:
> 
> ^1da177e4c3f4 (Linus Torvalds         2005-04-16 15:20:36 -0700  423)
> ^1da177e4c3f4 (Linus Torvalds         2005-04-16 15:20:36 -0700  424)
>          /* Protection Register info */
> b359ed5184aeb (Jean-Philippe Brucker  2020-04-17 16:23:26 +0200  425)
>          if (extp->NumProtectionFields)
> b359ed5184aeb (Jean-Philippe Brucker  2020-04-17 16:23:26 +0200  426)
>                  extra_size += (extp->NumProtectionFields - 1) *
> b359ed5184aeb (Jean-Philippe Brucker  2020-04-17 16:23:26 +0200  427)
>                                sizeof(struct cfi_intelext_otpinfo);
> 
> Jean-Philippe's patch is not the root cause AFAICT, but something
> predating it, and predating git history.
> 
> The fix easily applies to all maintained stable kernels.

Ack.

> > > Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > > ---
> > > ChangeLog v3->v4:
> > > - Collected Nico's ACK.
> > > - Stalled since june! Has this been missed?  
> >
> > Our current organization relies on Vignesh to pick-up (or tell me to
> > pick-up) cfi patches. But he is slightly less active these days, so if I
> > don't get any feedback from him soon I will take it for the next merge
> > window.
> >
> > Sorry for the delay anyway.  
> 
> No worries it's not like the patch is urgent.

Haha, well, yes :)

Thanks,
Miquèl
Miquel Raynal Oct. 27, 2023, 5:47 p.m. UTC | #4 
On Fri, 2023-10-20 at 20:30:29 UTC, 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.
> 
> Adding a small byte swapping loop for the OTP in read_pri_intelext()
> 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: stable@vger.kernel.org
> Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel
diff mbox series

Patch

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 11b06fefaa0e..c10693ba265b 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -422,9 +422,25 @@  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);
+				sizeof(struct cfi_intelext_otpinfo);
+
+			if (extp_size >= sizeof(*extp) + extra_size) {
+				int i;
+
+				/* Do some byteswapping if necessary */
+				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);
+					otp++;
+				}
+			}
+		}
 	}
 
 	if (extp->MinorVersion >= '1') {