| Message ID | 20180303222903.27767-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: jedec_probe: Fix crash in jedec_read_mfr() | expand |
On Sat, Mar 3, 2018 at 11:29 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > It turns out that the loop where we read manufacturer > jedec_read_mfd() can under some circumstances get a > CFI_MFR_CONTINUATION repeatedly, making the loop go > over all banks and eventually hit the end of the > map and crash because of an access violation: > > Unable to handle kernel paging request at virtual address c4980000 > pgd = (ptrval) > [c4980000] *pgd=03808811, *pte=00000000, *ppte=00000000 > Internal error: Oops: 7 [#1] PREEMPT ARM > CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc1+ #150 > Hardware name: Gemini (Device Tree) > PC is at jedec_probe_chip+0x6ec/0xcd0 > LR is at 0x4 > pc : [<c03a2bf4>] lr : [<00000004>] psr: 60000013 > sp : c382dd18 ip : 0000ffff fp : 00000000 > r10: c0626388 r9 : 00020000 r8 : c0626340 > r7 : 00000000 r6 : 00000001 r5 : c3a71afc r4 : c382dd70 > r3 : 00000001 r2 : c4900000 r1 : 00000002 r0 : 00080000 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > Control: 0000397f Table: 00004000 DAC: 00000053 > Process swapper (pid: 1, stack limit = 0x(ptrval)) > > Fix this by breaking the loop with a return 0 if > the offset exceeds the map size. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Could someone maintaining MTD please pick up this bug fix? It is a very real problem to me. Yours, Linus Walleij
Am Samstag, 3. März 2018, 23:29:03 CEST schrieb Linus Walleij: > It turns out that the loop where we read manufacturer > jedec_read_mfd() can under some circumstances get a > CFI_MFR_CONTINUATION repeatedly, making the loop go > over all banks and eventually hit the end of the > map and crash because of an access violation: > > Unable to handle kernel paging request at virtual address c4980000 > pgd = (ptrval) > [c4980000] *pgd=03808811, *pte=00000000, *ppte=00000000 > Internal error: Oops: 7 [#1] PREEMPT ARM > CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc1+ #150 > Hardware name: Gemini (Device Tree) > PC is at jedec_probe_chip+0x6ec/0xcd0 > LR is at 0x4 > pc : [<c03a2bf4>] lr : [<00000004>] psr: 60000013 > sp : c382dd18 ip : 0000ffff fp : 00000000 > r10: c0626388 r9 : 00020000 r8 : c0626340 > r7 : 00000000 r6 : 00000001 r5 : c3a71afc r4 : c382dd70 > r3 : 00000001 r2 : c4900000 r1 : 00000002 r0 : 00080000 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > Control: 0000397f Table: 00004000 DAC: 00000053 > Process swapper (pid: 1, stack limit = 0x(ptrval)) > > Fix this by breaking the loop with a return 0 if > the offset exceeds the map size. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mtd/chips/jedec_probe.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/chips/jedec_probe.c > b/drivers/mtd/chips/jedec_probe.c index 7c0b27d132b1..b479bd81120b 100644 > --- a/drivers/mtd/chips/jedec_probe.c > +++ b/drivers/mtd/chips/jedec_probe.c > @@ -1889,6 +1889,8 @@ static inline u32 jedec_read_mfr(struct map_info *map, > uint32_t base, do { > uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi); > mask = (1 << (cfi->device_type * 8)) - 1; > + if (ofs >= map->size) > + return 0; > result = map_read(map, base + ofs); > bank++; > } while ((result.x[0] & mask) == CFI_MFR_CONTINUATION); The fix is legit but I'm not sure whether we should emit a warning in this case too since something is obviously wrong. Boris? Thanks, //richard
On Tue, Mar 27, 2018 at 9:02 PM, Richard Weinberger <richard@nod.at> wrote: > Am Samstag, 3. März 2018, 23:29:03 CEST schrieb Linus Walleij: >> It turns out that the loop where we read manufacturer >> jedec_read_mfd() can under some circumstances get a >> CFI_MFR_CONTINUATION repeatedly, making the loop go >> over all banks and eventually hit the end of the >> map and crash because of an access violation: >> >> uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi); >> mask = (1 << (cfi->device_type * 8)) - 1; >> + if (ofs >= map->size) >> + return 0; >> result = map_read(map, base + ofs); >> bank++; >> } while ((result.x[0] & mask) == CFI_MFR_CONTINUATION); > > The fix is legit but I'm not sure whether we should emit a warning in this > case too since something is obviously wrong. What is wrong is perhaps our way of probing JEDEC Flashes: the condition is hit when accessing a device on a 16bit bus in 8bit mode, which is what the probe code tries before it tries 16bit. However there are clear comments in the code (above jedec_match()) indicating that it is impossible to do a perfect probe routine so this could be expected to happen to some extent,I am merely amending an issue that may be unfixable. Yours, Linus Walleij
On Tue, 27 Mar 2018 21:02:08 +0200 Richard Weinberger <richard@nod.at> wrote: > Am Samstag, 3. März 2018, 23:29:03 CEST schrieb Linus Walleij: > > It turns out that the loop where we read manufacturer > > jedec_read_mfd() can under some circumstances get a > > CFI_MFR_CONTINUATION repeatedly, making the loop go > > over all banks and eventually hit the end of the > > map and crash because of an access violation: > > > > Unable to handle kernel paging request at virtual address c4980000 > > pgd = (ptrval) > > [c4980000] *pgd=03808811, *pte=00000000, *ppte=00000000 > > Internal error: Oops: 7 [#1] PREEMPT ARM > > CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc1+ #150 > > Hardware name: Gemini (Device Tree) > > PC is at jedec_probe_chip+0x6ec/0xcd0 > > LR is at 0x4 > > pc : [<c03a2bf4>] lr : [<00000004>] psr: 60000013 > > sp : c382dd18 ip : 0000ffff fp : 00000000 > > r10: c0626388 r9 : 00020000 r8 : c0626340 > > r7 : 00000000 r6 : 00000001 r5 : c3a71afc r4 : c382dd70 > > r3 : 00000001 r2 : c4900000 r1 : 00000002 r0 : 00080000 > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > > Control: 0000397f Table: 00004000 DAC: 00000053 > > Process swapper (pid: 1, stack limit = 0x(ptrval)) > > > > Fix this by breaking the loop with a return 0 if > > the offset exceeds the map size. > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > drivers/mtd/chips/jedec_probe.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/mtd/chips/jedec_probe.c > > b/drivers/mtd/chips/jedec_probe.c index 7c0b27d132b1..b479bd81120b 100644 > > --- a/drivers/mtd/chips/jedec_probe.c > > +++ b/drivers/mtd/chips/jedec_probe.c > > @@ -1889,6 +1889,8 @@ static inline u32 jedec_read_mfr(struct map_info *map, > > uint32_t base, do { > > uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi); > > mask = (1 << (cfi->device_type * 8)) - 1; > > + if (ofs >= map->size) > > + return 0; > > result = map_read(map, base + ofs); > > bank++; > > } while ((result.x[0] & mask) == CFI_MFR_CONTINUATION); > > The fix is legit but I'm not sure whether we should emit a warning in this > case too since something is obviously wrong. > Boris? Looks good to me: 0 does not seem to be a valid id, so it should not allow the caller to find a valid match (we could also return CFI_MFR_CONTINUATION but 0 is fine). Linus, do you want to add a Fixes and Cc-stable tag so that it can be backported to stable kernels?
On Sat, 3 Mar 2018 23:29:03 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > It turns out that the loop where we read manufacturer > jedec_read_mfd() can under some circumstances get a > CFI_MFR_CONTINUATION repeatedly, making the loop go > over all banks and eventually hit the end of the > map and crash because of an access violation: > > Unable to handle kernel paging request at virtual address c4980000 > pgd = (ptrval) > [c4980000] *pgd=03808811, *pte=00000000, *ppte=00000000 > Internal error: Oops: 7 [#1] PREEMPT ARM > CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc1+ #150 > Hardware name: Gemini (Device Tree) > PC is at jedec_probe_chip+0x6ec/0xcd0 > LR is at 0x4 > pc : [<c03a2bf4>] lr : [<00000004>] psr: 60000013 > sp : c382dd18 ip : 0000ffff fp : 00000000 > r10: c0626388 r9 : 00020000 r8 : c0626340 > r7 : 00000000 r6 : 00000001 r5 : c3a71afc r4 : c382dd70 > r3 : 00000001 r2 : c4900000 r1 : 00000002 r0 : 00080000 > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none > Control: 0000397f Table: 00004000 DAC: 00000053 > Process swapper (pid: 1, stack limit = 0x(ptrval)) > > Fix this by breaking the loop with a return 0 if > the offset exceeds the map size. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Applied. Also added: Fixes: 5c9c11e1c47c ("[MTD] [NOR] Add support for flash chips with ID in bank other than 0") Cc: <stable@vger.kernel.org> I'll send a fixes PR to Linus soon. Thanks, Boris > --- > drivers/mtd/chips/jedec_probe.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/chips/jedec_probe.c b/drivers/mtd/chips/jedec_probe.c > index 7c0b27d132b1..b479bd81120b 100644 > --- a/drivers/mtd/chips/jedec_probe.c > +++ b/drivers/mtd/chips/jedec_probe.c > @@ -1889,6 +1889,8 @@ static inline u32 jedec_read_mfr(struct map_info *map, uint32_t base, > do { > uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi); > mask = (1 << (cfi->device_type * 8)) - 1; > + if (ofs >= map->size) > + return 0; > result = map_read(map, base + ofs); > bank++; > } while ((result.x[0] & mask) == CFI_MFR_CONTINUATION);
diff --git a/drivers/mtd/chips/jedec_probe.c b/drivers/mtd/chips/jedec_probe.c index 7c0b27d132b1..b479bd81120b 100644 --- a/drivers/mtd/chips/jedec_probe.c +++ b/drivers/mtd/chips/jedec_probe.c @@ -1889,6 +1889,8 @@ static inline u32 jedec_read_mfr(struct map_info *map, uint32_t base, do { uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi); mask = (1 << (cfi->device_type * 8)) - 1; + if (ofs >= map->size) + return 0; result = map_read(map, base + ofs); bank++; } while ((result.x[0] & mask) == CFI_MFR_CONTINUATION);
It turns out that the loop where we read manufacturer jedec_read_mfd() can under some circumstances get a CFI_MFR_CONTINUATION repeatedly, making the loop go over all banks and eventually hit the end of the map and crash because of an access violation: Unable to handle kernel paging request at virtual address c4980000 pgd = (ptrval) [c4980000] *pgd=03808811, *pte=00000000, *ppte=00000000 Internal error: Oops: 7 [#1] PREEMPT ARM CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc1+ #150 Hardware name: Gemini (Device Tree) PC is at jedec_probe_chip+0x6ec/0xcd0 LR is at 0x4 pc : [<c03a2bf4>] lr : [<00000004>] psr: 60000013 sp : c382dd18 ip : 0000ffff fp : 00000000 r10: c0626388 r9 : 00020000 r8 : c0626340 r7 : 00000000 r6 : 00000001 r5 : c3a71afc r4 : c382dd70 r3 : 00000001 r2 : c4900000 r1 : 00000002 r0 : 00080000 Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 0000397f Table: 00004000 DAC: 00000053 Process swapper (pid: 1, stack limit = 0x(ptrval)) Fix this by breaking the loop with a return 0 if the offset exceeds the map size. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mtd/chips/jedec_probe.c | 2 ++ 1 file changed, 2 insertions(+)