Using x1e80100-camcc on a recent kernel I discovered the following NULL
pointer dereference.
[ 1.347567] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 1.347569] Mem abort info:
[ 1.347569] ESR = 0x0000000096000004
[ 1.347570] EC = 0x25: DABT (current EL), IL = 32 bits
[ 1.347572] SET = 0, FnV = 0
[ 1.347572] EA = 0, S1PTW = 0
[ 1.347573] FSC = 0x04: level 0 translation fault
[ 1.347574] Data abort info:
[ 1.347575] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 1.347576] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 1.347576] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 1.347577] [0000000000000000] user address but active_mm is swapper
[ 1.347579] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 1.347580] Modules linked in:
[ 1.347583] CPU: 1 PID: 80 Comm: kworker/u49:1 Not tainted 6.8.0-rc6-next-20240228-00163-gbe6ae77b72b2 #26
[ 1.347586] Hardware name: Qualcomm CRD, BIOS 6.0.230809.BOOT.MXF.2.4-00174-HAMOA-1 08/ 9/2023
[ 1.347587] Workqueue: events_unbound deferred_probe_work_func
[ 1.347595] pstate: 01400005 (nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[ 1.347597] pc : clk_core_get+0xe0/0x110
[ 1.347601] lr : clk_core_get+0x108/0x110
[ 1.347603] sp : ffff800080353940
[ 1.347604] x29: ffff8000803539a0 x28: 0000000000000000 x27: ffffb0aa57c4e2e0
[ 1.347607] x26: ffffb0aa57c4e240 x25: ffff4cbd0511e4c8 x24: 0000000000000000
[ 1.347609] x23: ffffb0aa583c3440 x22: 0000000000000000 x21: ffff4cc07e1d2ab8
[ 1.347612] x20: 0000000000000000 x19: ffff4cbd00e28ac0 x18: 0000000000000001
[ 1.347614] x17: 0000000000000018 x16: 0000000000000034 x15: 0000000000000002
[ 1.347616] x14: ffffb0aa58fc6498 x13: ffffb0aa58293000 x12: 696669746f6e5f6b
[ 1.347619] x11: 0000000ad6d076a3 x10: ffffb0aa58c600fb x9 : 0000000000000008
[ 1.347621] x8 : 0101010101010101 x7 : 00000000736c6c65 x6 : 0080f0e8e16e646c
[ 1.347624] x5 : ffff800080353958 x4 : 0000000000000000 x3 : ffff4cbd00d09100
[ 1.347626] x2 : 0000000000000000 x1 : ffff4cbd00d09100 x0 : 0000000000000000
[ 1.347628] Call trace:
[ 1.347630] clk_core_get+0xe0/0x110
[ 1.347631] clk_core_get_parent_by_index+0xc8/0xe0
[ 1.347634] __clk_register+0x1f0/0x864
[ 1.347636] devm_clk_hw_register+0x5c/0xd4
[ 1.347639] devm_clk_register_regmap+0x44/0x84
[ 1.347642] qcom_cc_really_probe+0x1b4/0x25c
[ 1.347644] cam_cc_x1e80100_probe+0x14c/0x1c8
[ 1.347646] platform_probe+0x68/0xc8
[ 1.347649] really_probe+0x148/0x2b0
[ 1.347651] __driver_probe_device+0x78/0x12c
[ 1.347654] driver_probe_device+0x40/0x118
[ 1.347656] __device_attach_driver+0xb8/0x134
[ 1.347658] bus_for_each_drv+0x88/0xe8
[ 1.347661] __device_attach+0xa0/0x190
[ 1.347664] device_initial_probe+0x14/0x20
[ 1.347666] bus_probe_device+0xac/0xb0
[ 1.347668] deferred_probe_work_func+0x88/0xc0
[ 1.347670] process_one_work+0x148/0x29c
[ 1.347675] worker_thread+0x2fc/0x40c
[ 1.347678] kthread+0x110/0x114
[ 1.347681] ret_from_fork+0x10/0x20
[ 1.347684] Code: aa1303e0 97fff96f b140041f 54fffd08 (f9400000)
[ 1.347686] ---[ end trace 0000000000000000 ]---
The first patch fixes the NULL dereference by checking hw before returning
hw->core.
The second patch addresses the cause of the NULL pointer, which is the DT
implied indexing is not fully captured in camcc-x1e80100.c.
Obviously the above NULL deref wouldn't occur with the second patch applied
however reading the description of clk_core_get() it appears to want to
return NULL but currently cannot do so, so check for hw and return a NULL
if hw is NULL instead of dereferencing hw.
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
Bryan O'Donoghue (2):
clk: Fix clk_core_get NULL dereference
clk: qcom: camcc-x1e80100: Fix missing DT_IFACE enum in x1e80100 camcc
drivers/clk/clk.c | 3 +++
drivers/clk/qcom/camcc-x1e80100.c | 1 +
2 files changed, 4 insertions(+)
---
base-commit: 1870cdc0e8dee32e3c221704a2977898ba4c10e8
change-id: 20240301-linux-next-24-03-01-simple-clock-fixes-dc7542e23d90
Best regards,
--
Bryan O'Donoghue <[email protected]>
It is possible for clk_core_get to dereference a NULL in the following
sequence:
clk_core_get()
of_clk_get_hw_from_clkspec()
__of_clk_get_hw_from_provider()
__clk_get_hw()
__clk_get_hw() can return NULL which is dereferenced by clk_core_get() at
hw->core.
Prior to commit dde4eff47c82 ("clk: Look for parents with clkdev based
clk_lookups") the check IS_ERR_OR_NULL() was performed which would have
caught the NULL.
Reading the description of this function it talks about returning NULL but
that cannot be so at the moment.
Update the function to check for hw before dereferencing it and return NULL
if hw is NULL.
Fixes: dde4eff47c82 ("clk: Look for parents with clkdev based clk_lookups")
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/clk/clk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a3bc7fb90d0f..25371c91a58f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -418,6 +418,9 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
if (IS_ERR(hw))
return ERR_CAST(hw);
+ if (!hw)
+ return NULL;
+
return hw->core;
}
--
2.43.0
The desired DT pattern for clock indexing is the following:
clocks = <&gcc GCC_CAMERA_AHB_CLK>,
<&bi_tcxo_div2>,
<&bi_tcxo_ao_div2>,
<&sleep_clk>;
In order to facilitate that indexing structure we need to have DT_IFACE
enum defined.
Fixes: 76126a5129b5 ("clk: qcom: Add camcc clock driver for x1e80100")
Signed-off-by: Bryan O'Donoghue <[email protected]>
---
drivers/clk/qcom/camcc-x1e80100.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
index f7f3d92c263d..46bb225906bf 100644
--- a/drivers/clk/qcom/camcc-x1e80100.c
+++ b/drivers/clk/qcom/camcc-x1e80100.c
@@ -21,6 +21,7 @@
#include "reset.h"
enum {
+ DT_IFACE,
DT_BI_TCXO,
DT_BI_TCXO_AO,
DT_SLEEP_CLK,
--
2.43.0
On 2.03.2024 01:52, Bryan O'Donoghue wrote:
> The desired DT pattern for clock indexing is the following:
>
> clocks = <&gcc GCC_CAMERA_AHB_CLK>,
> <&bi_tcxo_div2>,
> <&bi_tcxo_ao_div2>,
> <&sleep_clk>;
>
> In order to facilitate that indexing structure we need to have DT_IFACE
> enum defined.
>
> Fixes: 76126a5129b5 ("clk: qcom: Add camcc clock driver for x1e80100")
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
On Sat, 02 Mar 2024 00:52:13 +0000, Bryan O'Donoghue wrote:
> Using x1e80100-camcc on a recent kernel I discovered the following NULL
> pointer dereference.
>
> [ 1.347567] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [ 1.347569] Mem abort info:
> [ 1.347569] ESR = 0x0000000096000004
> [ 1.347570] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 1.347572] SET = 0, FnV = 0
> [ 1.347572] EA = 0, S1PTW = 0
> [ 1.347573] FSC = 0x04: level 0 translation fault
> [ 1.347574] Data abort info:
> [ 1.347575] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [ 1.347576] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [ 1.347576] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [ 1.347577] [0000000000000000] user address but active_mm is swapper
> [ 1.347579] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [ 1.347580] Modules linked in:
> [ 1.347583] CPU: 1 PID: 80 Comm: kworker/u49:1 Not tainted 6.8.0-rc6-next-20240228-00163-gbe6ae77b72b2 #26
> [ 1.347586] Hardware name: Qualcomm CRD, BIOS 6.0.230809.BOOT.MXF.2.4-00174-HAMOA-1 08/ 9/2023
> [ 1.347587] Workqueue: events_unbound deferred_probe_work_func
> [ 1.347595] pstate: 01400005 (nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [ 1.347597] pc : clk_core_get+0xe0/0x110
> [ 1.347601] lr : clk_core_get+0x108/0x110
> [ 1.347603] sp : ffff800080353940
> [ 1.347604] x29: ffff8000803539a0 x28: 0000000000000000 x27: ffffb0aa57c4e2e0
> [ 1.347607] x26: ffffb0aa57c4e240 x25: ffff4cbd0511e4c8 x24: 0000000000000000
> [ 1.347609] x23: ffffb0aa583c3440 x22: 0000000000000000 x21: ffff4cc07e1d2ab8
> [ 1.347612] x20: 0000000000000000 x19: ffff4cbd00e28ac0 x18: 0000000000000001
> [ 1.347614] x17: 0000000000000018 x16: 0000000000000034 x15: 0000000000000002
> [ 1.347616] x14: ffffb0aa58fc6498 x13: ffffb0aa58293000 x12: 696669746f6e5f6b
> [ 1.347619] x11: 0000000ad6d076a3 x10: ffffb0aa58c600fb x9 : 0000000000000008
> [ 1.347621] x8 : 0101010101010101 x7 : 00000000736c6c65 x6 : 0080f0e8e16e646c
> [ 1.347624] x5 : ffff800080353958 x4 : 0000000000000000 x3 : ffff4cbd00d09100
> [ 1.347626] x2 : 0000000000000000 x1 : ffff4cbd00d09100 x0 : 0000000000000000
> [ 1.347628] Call trace:
> [ 1.347630] clk_core_get+0xe0/0x110
> [ 1.347631] clk_core_get_parent_by_index+0xc8/0xe0
> [ 1.347634] __clk_register+0x1f0/0x864
> [ 1.347636] devm_clk_hw_register+0x5c/0xd4
> [ 1.347639] devm_clk_register_regmap+0x44/0x84
> [ 1.347642] qcom_cc_really_probe+0x1b4/0x25c
> [ 1.347644] cam_cc_x1e80100_probe+0x14c/0x1c8
> [ 1.347646] platform_probe+0x68/0xc8
> [ 1.347649] really_probe+0x148/0x2b0
> [ 1.347651] __driver_probe_device+0x78/0x12c
> [ 1.347654] driver_probe_device+0x40/0x118
> [ 1.347656] __device_attach_driver+0xb8/0x134
> [ 1.347658] bus_for_each_drv+0x88/0xe8
> [ 1.347661] __device_attach+0xa0/0x190
> [ 1.347664] device_initial_probe+0x14/0x20
> [ 1.347666] bus_probe_device+0xac/0xb0
> [ 1.347668] deferred_probe_work_func+0x88/0xc0
> [ 1.347670] process_one_work+0x148/0x29c
> [ 1.347675] worker_thread+0x2fc/0x40c
> [ 1.347678] kthread+0x110/0x114
> [ 1.347681] ret_from_fork+0x10/0x20
> [ 1.347684] Code: aa1303e0 97fff96f b140041f 54fffd08 (f9400000)
> [ 1.347686] ---[ end trace 0000000000000000 ]---
>
> [...]
Applied, thanks!
[2/2] clk: qcom: camcc-x1e80100: Fix missing DT_IFACE enum in x1e80100 camcc
commit: 9dd7b0d351f0c6af9b69d969919a2a8b04bbfd6e
Best regards,
--
Bjorn Andersson <[email protected]>
Quoting Bryan O'Donoghue (2024-03-01 16:52:14)
> It is possible for clk_core_get to dereference a NULL in the following
> sequence:
>
> clk_core_get()
> of_clk_get_hw_from_clkspec()
> __of_clk_get_hw_from_provider()
> __clk_get_hw()
>
> __clk_get_hw() can return NULL which is dereferenced by clk_core_get() at
> hw->core.
>
> Prior to commit dde4eff47c82 ("clk: Look for parents with clkdev based
> clk_lookups") the check IS_ERR_OR_NULL() was performed which would have
> caught the NULL.
>
> Reading the description of this function it talks about returning NULL but
> that cannot be so at the moment.
>
> Update the function to check for hw before dereferencing it and return NULL
> if hw is NULL.
>
> Fixes: dde4eff47c82 ("clk: Look for parents with clkdev based clk_lookups")
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
I dug through a bunch of old patches and this looks right. I'm not sure
why a provider would ever return NULL though. A NULL pointer means that
the parent is never going to appear so we shouldn't treat this clk as
orphaned in case the clk needs to be clk_get()able and change parents.
This was all part of my plan to introduce a clk_parent_hw() clk op that
returns a pointer and then implement probe defer through clk_get() when
a clk is orphaned. A NULL clk also means a clk is optional and not
there.
Anyway, I've applied this to clk-next. I hope to send out the
clk_parent_hw clk op series soon.