* [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr()
@ 2018-03-03 22:29 Linus Walleij
2018-03-27 17:51 ` Linus Walleij
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Linus Walleij @ 2018-03-03 22:29 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Richard Weinberger, Cyrille Pitchen, linux-mtd
Cc: 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);
--
2.14.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr()
2018-03-03 22:29 [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr() Linus Walleij
@ 2018-03-27 17:51 ` Linus Walleij
2018-03-27 19:02 ` Richard Weinberger
2018-03-29 17:24 ` Boris Brezillon
2 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2018-03-27 17:51 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Richard Weinberger, Cyrille Pitchen, linux-mtd
Cc: Linus Walleij
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr()
2018-03-03 22:29 [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr() Linus Walleij
2018-03-27 17:51 ` Linus Walleij
@ 2018-03-27 19:02 ` Richard Weinberger
2018-03-27 20:31 ` Linus Walleij
2018-03-28 7:03 ` Boris Brezillon
2018-03-29 17:24 ` Boris Brezillon
2 siblings, 2 replies; 6+ messages in thread
From: Richard Weinberger @ 2018-03-27 19:02 UTC (permalink / raw)
To: Linus Walleij
Cc: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Cyrille Pitchen, linux-mtd
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr()
2018-03-27 19:02 ` Richard Weinberger
@ 2018-03-27 20:31 ` Linus Walleij
2018-03-28 7:03 ` Boris Brezillon
1 sibling, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2018-03-27 20:31 UTC (permalink / raw)
To: Richard Weinberger
Cc: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Cyrille Pitchen, linux-mtd
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr()
2018-03-27 19:02 ` Richard Weinberger
2018-03-27 20:31 ` Linus Walleij
@ 2018-03-28 7:03 ` Boris Brezillon
1 sibling, 0 replies; 6+ messages in thread
From: Boris Brezillon @ 2018-03-28 7:03 UTC (permalink / raw)
To: Richard Weinberger
Cc: Linus Walleij, David Woodhouse, Brian Norris, Boris Brezillon,
Marek Vasut, Cyrille Pitchen, linux-mtd
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?
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr()
2018-03-03 22:29 [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr() Linus Walleij
2018-03-27 17:51 ` Linus Walleij
2018-03-27 19:02 ` Richard Weinberger
@ 2018-03-29 17:24 ` Boris Brezillon
2 siblings, 0 replies; 6+ messages in thread
From: Boris Brezillon @ 2018-03-29 17:24 UTC (permalink / raw)
To: Linus Walleij
Cc: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Richard Weinberger, Cyrille Pitchen, linux-mtd
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);
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-29 17:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-03 22:29 [PATCH] mtd: jedec_probe: Fix crash in jedec_read_mfr() Linus Walleij
2018-03-27 17:51 ` Linus Walleij
2018-03-27 19:02 ` Richard Weinberger
2018-03-27 20:31 ` Linus Walleij
2018-03-28 7:03 ` Boris Brezillon
2018-03-29 17:24 ` Boris Brezillon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.