| Message ID | 20190815223155.21384-1-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [RFC,v1] clk: Fix potential NULL dereference in clk_fetch_parent_index() | expand |
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 <martin.blumenstingl@googlemail.com> > --- > 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 <sboyd@kernel.org> 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 <martin.blumenstingl@googlemail.com> > > --- > > 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 <martin.blumenstingl@googlemail.com> > --- 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
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; }
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 <martin.blumenstingl@googlemail.com> --- 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(-)