Don't compare the parent clock name with a NULL name in the
clk_parent_map. This prevents a kernel crash when passing NULL
core->parents[i].name to strcmp().
An example which triggered this is a mux clock with four parents when
each of them is referenced in the clock driver using
clk_parent_data.fw_name and then calling clk_set_parent(clk, 3rd_parent)
on this mux.
In this case the first parent is also the HW default so
core->parents[i].hw is populated when the clock is registered. Calling
clk_set_parent(clk, 3rd_parent) will then go through all parents and
skip the first parent because it's hw pointer doesn't match. For the
second parent no hw pointer is cached yet and clk_core_get(core, 1)
returns a non-matching pointer (which is correct because we are comparing
the second with the third parent). Comparing the result of
clk_core_get(core, 2) with the requested parent gives a match. However
we don't reach this point because right after the clk_core_get(core, 1)
mismatch the old code tried to !strcmp(parent->name, NULL) (where the
second argument is actually core->parents[i].name, but that was never
populated by the clock driver).
Signed-off-by: Martin Blumenstingl <[email protected]>
---
I have seen the original crash when I was testing an MMC driver which
is not upstream yet on v5.3-rc4. I'm not sure whether this fix is
"correct" (it fixes the crash for me) or where to point the Fixes tag
to, it may be one of:
- fc0c209c147f ("clk: Allow parents to be specified without string names")
- 1a079560b145 ("clk: Cache core in clk_fetch_parent_index() without names")
This is meant to be applied on top of v5.3-rc4.
drivers/clk/clk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c0990703ce54..567a044a368b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1632,7 +1632,8 @@ static int clk_fetch_parent_index(struct clk_core *core,
break;
/* Fallback to comparing globally unique names */
- if (!strcmp(parent->name, core->parents[i].name))
+ if (core->parents[i].name &&
+ !strcmp(parent->name, core->parents[i].name))
break;
}
--
2.22.1
Quoting Martin Blumenstingl (2019-08-15 15:31:55)
> Don't compare the parent clock name with a NULL name in the
> clk_parent_map. This prevents a kernel crash when passing NULL
> core->parents[i].name to strcmp().
>
> An example which triggered this is a mux clock with four parents when
> each of them is referenced in the clock driver using
> clk_parent_data.fw_name and then calling clk_set_parent(clk, 3rd_parent)
> on this mux.
> In this case the first parent is also the HW default so
> core->parents[i].hw is populated when the clock is registered. Calling
> clk_set_parent(clk, 3rd_parent) will then go through all parents and
> skip the first parent because it's hw pointer doesn't match. For the
> second parent no hw pointer is cached yet and clk_core_get(core, 1)
> returns a non-matching pointer (which is correct because we are comparing
> the second with the third parent). Comparing the result of
> clk_core_get(core, 2) with the requested parent gives a match. However
> we don't reach this point because right after the clk_core_get(core, 1)
> mismatch the old code tried to !strcmp(parent->name, NULL) (where the
> second argument is actually core->parents[i].name, but that was never
> populated by the clock driver).
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
> I have seen the original crash when I was testing an MMC driver which
> is not upstream yet on v5.3-rc4. I'm not sure whether this fix is
> "correct" (it fixes the crash for me) or where to point the Fixes tag
> to, it may be one of:
> - fc0c209c147f ("clk: Allow parents to be specified without string names")
> - 1a079560b145 ("clk: Cache core in clk_fetch_parent_index() without names")
>
> This is meant to be applied on top of v5.3-rc4.
>
Ah ok. I thought that strcmp() would ignore NULL arguments, but
apparently not. I can apply this to clk-fixes.
Hi Stephen,
On Fri, Aug 16, 2019 at 1:29 AM Stephen Boyd <[email protected]> wrote:
>
> Quoting Martin Blumenstingl (2019-08-15 15:31:55)
> > Don't compare the parent clock name with a NULL name in the
> > clk_parent_map. This prevents a kernel crash when passing NULL
> > core->parents[i].name to strcmp().
> >
> > An example which triggered this is a mux clock with four parents when
> > each of them is referenced in the clock driver using
> > clk_parent_data.fw_name and then calling clk_set_parent(clk, 3rd_parent)
> > on this mux.
> > In this case the first parent is also the HW default so
> > core->parents[i].hw is populated when the clock is registered. Calling
> > clk_set_parent(clk, 3rd_parent) will then go through all parents and
> > skip the first parent because it's hw pointer doesn't match. For the
> > second parent no hw pointer is cached yet and clk_core_get(core, 1)
> > returns a non-matching pointer (which is correct because we are comparing
> > the second with the third parent). Comparing the result of
> > clk_core_get(core, 2) with the requested parent gives a match. However
> > we don't reach this point because right after the clk_core_get(core, 1)
> > mismatch the old code tried to !strcmp(parent->name, NULL) (where the
> > second argument is actually core->parents[i].name, but that was never
> > populated by the clock driver).
> >
> > Signed-off-by: Martin Blumenstingl <[email protected]>
> > ---
> > I have seen the original crash when I was testing an MMC driver which
> > is not upstream yet on v5.3-rc4. I'm not sure whether this fix is
> > "correct" (it fixes the crash for me) or where to point the Fixes tag
> > to, it may be one of:
> > - fc0c209c147f ("clk: Allow parents to be specified without string names")
> > - 1a079560b145 ("clk: Cache core in clk_fetch_parent_index() without names")
> >
> > This is meant to be applied on top of v5.3-rc4.
> >
>
> Ah ok. I thought that strcmp() would ignore NULL arguments, but
> apparently not. I can apply this to clk-fixes.
at least ARM [0] and the generic [1] implementations don't
I did not bisect this so do you have any suggestion for a Fixes tag? I
mentioned two candidates above, but I'm not sure which one to use
just let me know, then I'll resend with the fixes tag so you can take
it through clk-fixes
Martin
[0] https://elixir.bootlin.com/linux/v5.2/source/arch/arm/boot/compressed/string.c#L91
[1] https://elixir.bootlin.com/linux/v5.2/source/lib/string.c#L356
Quoting Martin Blumenstingl (2019-08-15 15:31:55)
> Don't compare the parent clock name with a NULL name in the
> clk_parent_map. This prevents a kernel crash when passing NULL
> core->parents[i].name to strcmp().
>
> An example which triggered this is a mux clock with four parents when
> each of them is referenced in the clock driver using
> clk_parent_data.fw_name and then calling clk_set_parent(clk, 3rd_parent)
> on this mux.
> In this case the first parent is also the HW default so
> core->parents[i].hw is populated when the clock is registered. Calling
> clk_set_parent(clk, 3rd_parent) will then go through all parents and
> skip the first parent because it's hw pointer doesn't match. For the
> second parent no hw pointer is cached yet and clk_core_get(core, 1)
> returns a non-matching pointer (which is correct because we are comparing
> the second with the third parent). Comparing the result of
> clk_core_get(core, 2) with the requested parent gives a match. However
> we don't reach this point because right after the clk_core_get(core, 1)
> mismatch the old code tried to !strcmp(parent->name, NULL) (where the
> second argument is actually core->parents[i].name, but that was never
> populated by the clock driver).
>
> Signed-off-by: Martin Blumenstingl <[email protected]>
> ---
Applied to clk-fixes
Quoting Martin Blumenstingl (2019-08-15 23:48:08)
> > > I have seen the original crash when I was testing an MMC driver which
> > > is not upstream yet on v5.3-rc4. I'm not sure whether this fix is
> > > "correct" (it fixes the crash for me) or where to point the Fixes tag
> > > to, it may be one of:
> > > - fc0c209c147f ("clk: Allow parents to be specified without string names")
> > > - 1a079560b145 ("clk: Cache core in clk_fetch_parent_index() without names")
> > >
> > > This is meant to be applied on top of v5.3-rc4.
> > >
> >
> > Ah ok. I thought that strcmp() would ignore NULL arguments, but
> > apparently not. I can apply this to clk-fixes.
> at least ARM [0] and the generic [1] implementations don't
>
> I did not bisect this so do you have any suggestion for a Fixes tag? I
> mentioned two candidates above, but I'm not sure which one to use
> just let me know, then I'll resend with the fixes tag so you can take
> it through clk-fixes
>
>
I added the fixes tag for the first one, where it was broken, i.e.
fc0c209c147f. Thanks.
Hi,
On 16/08/2019 19:31, Stephen Boyd wrote:
> Quoting Martin Blumenstingl (2019-08-15 23:48:08)
>>>> I have seen the original crash when I was testing an MMC driver which
>>>> is not upstream yet on v5.3-rc4. I'm not sure whether this fix is
>>>> "correct" (it fixes the crash for me) or where to point the Fixes tag
>>>> to, it may be one of:
>>>> - fc0c209c147f ("clk: Allow parents to be specified without string names")
>>>> - 1a079560b145 ("clk: Cache core in clk_fetch_parent_index() without names")
>>>>
>>>> This is meant to be applied on top of v5.3-rc4.
>>>>
>>>
>>> Ah ok. I thought that strcmp() would ignore NULL arguments, but
>>> apparently not. I can apply this to clk-fixes.
>> at least ARM [0] and the generic [1] implementations don't
>>
>> I did not bisect this so do you have any suggestion for a Fixes tag? I
>> mentioned two candidates above, but I'm not sure which one to use
>> just let me know, then I'll resend with the fixes tag so you can take
>> it through clk-fixes
>>
>>
>
> I added the fixes tag for the first one, where it was broken, i.e.
> fc0c209c147f. Thanks.
>
For the record, this also fixes the Amlogic Meson GXBB/GXL when AO-CEC driver
is enabled :
[ 7.790319] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 7.790324] Mem abort info:
[ 7.790326] ESR = 0x96000007
[ 7.790330] Exception class = DABT (current EL), IL = 32 bits
[ 7.790331] SET = 0, FnV = 0
[ 7.790333] EA = 0, S1PTW = 0
[ 7.790334] Data abort info:
[ 7.790336] ISV = 0, ISS = 0x00000007
[ 7.790337] CM = 0, WnR = 0
[ 7.790341] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000364b7000
[ 7.790343] [0000000000000000] pgd=00000000364f5003, pud=00000000364f6003, pmd=00000000364f7003, pte=0000000000000000
[ 7.790350] Internal error: Oops: 96000007 [#1] SMP
[ 7.790461] Modules linked in: ao_cec(+)
[ 7.793569] CPU: 1 PID: 398 Comm: systemd-udevd Not tainted 5.3.0-rc5 #1
[ 7.800199] Hardware name: Libre Computer Board AML-S905X-CC (DT)
[ 7.806233] pstate: 40000005 (nZcv daif -PAN -UAO)
[ 7.810985] pc : __pi_strcmp+0x1c/0x154
[ 7.814775] lr : clk_fetch_parent_index.part.43+0xe4/0x120
[ 7.820203] sp : ffff0000113d3770
[ 7.823481] x29: ffff0000113d3770 x28: ffff00001136f000
[ 7.828741] x27: 0000000000000100 x26: ffff000010130648
[ 7.834012] x25: ffff000008943100 x24: 0000000000008000
[ 7.839273] x23: ffff80007c15cc00 x22: ffff80007c15cd00
[ 7.844525] x21: ffff80007c15ca00 x20: 0000000000000000
[ 7.849791] x19: 0000000000000000 x18: 0000000000000001
[ 7.855048] x17: 0000000000000000 x16: 0000000000000000
[ 7.860316] x15: ffffffffffffffff x14: ffffffffffffffff
[ 7.865578] x13: 0000000000000028 x12: 0101010101010101
[ 7.870831] x11: 0000000000000004 x10: 0101010101010101
[ 7.876092] x9 : ffffffffffffffff x8 : 7f7f7f7f7f7f7f7f
[ 7.881354] x7 : 0000000000000000 x6 : 0d0d0206ebadf2e1
[ 7.886615] x5 : 61722d6b06020d0d x4 : 8080808000000000
[ 7.891876] x3 : 36c6f636b2d72610 x2 : 00006b32335f6f61
[ 7.897137] x1 : 0000000000000000 x0 : ffff000010b147f0
[ 7.902405] Call trace:
[ 7.904825] __pi_strcmp+0x1c/0x154
[ 7.908269] clk_calc_new_rates+0x208/0x260
[ 7.912419] clk_calc_new_rates+0x10c/0x260
[ 7.916556] clk_core_set_rate_nolock+0xe8/0x1e8
[ 7.921129] clk_set_rate+0x34/0xa0
[ 7.924583] meson_ao_cec_probe+0x19c/0x278 [ao_cec]
[ 7.929498] platform_drv_probe+0x50/0xa0
[ 7.933461] really_probe+0xec/0x3d0
[ 7.936989] driver_probe_device+0xdc/0x130
[ 7.941128] device_driver_attach+0x6c/0x78
[ 7.945267] __driver_attach+0x9c/0x168
[ 7.949065] bus_for_each_dev+0x70/0xc0
[ 7.952860] driver_attach+0x20/0x28
[ 7.956394] bus_add_driver+0x190/0x220
[ 7.960190] driver_register+0x60/0x110
[ 7.963987] __platform_driver_register+0x44/0x50
[ 7.968646] meson_ao_cec_driver_init+0x1c/0x1000 [ao_cec]
[ 7.974079] do_one_initcall+0x74/0x1b0
[ 7.977873] do_init_module+0x50/0x208
[ 7.981581] load_module+0x1dc4/0x2350
[ 7.985288] __se_sys_finit_module+0x9c/0xf8
[ 7.989515] __arm64_sys_finit_module+0x18/0x20
[ 7.994001] el0_svc_common.constprop.0+0x7c/0xe8
[ 7.998658] el0_svc_compat_handler+0x18/0x20
[ 8.002970] el0_svc_compat+0x8/0x10
[ 8.006510] Code: 540002e1 f2400807 54000141 f8408402 (f8408423)
[ 8.012543] ---[ end trace e915b8961764bcd0 ]---
Neil