| Message ID | 20230530205146.3200321-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | mtd: cfi_cmdset_0001: Do not check for OTP outside device | expand |
On Tue, May 30, 2023 at 10:51 PM Linus Walleij <linus.walleij@linaro.org> 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): BTW if this fix is correct, this should probably be tagged for stable. Cc: stable@vger.kernel.org Yours, Linus Walleij
On Tue, 30 May 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 flash memory ends at > 0x11ffffff, so 0x12000000 is outside the range of the MTD > device. > > Breaking the while loop of we offset outside the size of the > MTD device fixes the issue. > > Cc: Nicolas Pitre <npitre@baylibre.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Well well... I apparently wrote that code... 23 years ago! Dang! Obviously I barely remember the details. Still, I have reservations about your fix which looks like it is papering over the real issue. Looking at the original code, we can see this from line 2418: /* next OTP region */ if (++field == extp->NumProtectionFields) break; That's where the loop break should be happening. Why isn't it the case for you? Especially given that 0x12000000 is completely bogus for sure, which is likely random memory content outside the actual extp record. Next time that random memory content may well be <= mtd->size and your fix won't do anything. This needs more investigation. > --- > drivers/mtd/chips/cfi_cmdset_0001.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c > index 54f92d09d9cf..a979e0316b31 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0001.c > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c > @@ -2352,6 +2352,9 @@ static int cfi_intelext_otp_walk(struct mtd_info *mtd, loff_t from, size_t len, > reg_fact_size *= cfi->interleave; > reg_user_size *= cfi->interleave; > > + if (reg_prot_offset >= mtd->size) > + break; > + > if (user_regs) { > groups = reg_user_groups; > groupsize = reg_user_size; > -- > 2.40.1 > >
diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c index 54f92d09d9cf..a979e0316b31 100644 --- a/drivers/mtd/chips/cfi_cmdset_0001.c +++ b/drivers/mtd/chips/cfi_cmdset_0001.c @@ -2352,6 +2352,9 @@ static int cfi_intelext_otp_walk(struct mtd_info *mtd, loff_t from, size_t len, reg_fact_size *= cfi->interleave; reg_user_size *= cfi->interleave; + if (reg_prot_offset >= mtd->size) + break; + if (user_regs) { groups = reg_user_groups; groupsize = reg_user_size;
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 flash memory ends at 0x11ffffff, so 0x12000000 is outside the range of the MTD device. Breaking the while loop of we offset outside the size of the MTD device fixes the issue. Cc: Nicolas Pitre <npitre@baylibre.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mtd/chips/cfi_cmdset_0001.c | 3 +++ 1 file changed, 3 insertions(+)