[PATCH] clk: xilinx: fix NULL pointer dereference in xvcu_clk_hw_unregister_leaf()

[PATCH] clk: xilinx: fix NULL pointer dereference in xvcu_clk_hw_unregister_leaf()

[Haotian Zhang]

xvcu_clk_hw_unregister_leaf() gets the mux with
clk_hw_get_parent(divider), which returns NULL when the divider has no
parent, and passes it to clk_hw_unregister_mux() without checking it.
The following check tests the wrong variable: divider has already been
verified to be non-NULL at this point, so "if (!divider) return;" is
dead code and a NULL mux is dereferenced, crashing in
clk_hw_unregister_mux().

Only unregister the mux when it is non-NULL, and always release the
divider.

Fixes: 9c789deea206 ("soc: xilinx: vcu: implement clock provider for output clocks")
Assisted-by: DeepSeek-V4.1-Flash
Signed-off-by: Haotian Zhang <vulab@iscas.ac.cn>
---
 drivers/clk/xilinx/xlnx_vcu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
index f14bda375e35..b004c16692d3 100644
--- a/drivers/clk/xilinx/xlnx_vcu.c
+++ b/drivers/clk/xilinx/xlnx_vcu.c
@@ -516,9 +516,8 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
 		return;
 
 	mux = clk_hw_get_parent(divider);
-	clk_hw_unregister_mux(mux);
-	if (!divider)
-		return;
+	if (mux)
+		clk_hw_unregister_mux(mux);
 
 	clk_hw_unregister_divider(divider);
 }
-- 
2.25.1

Re: [PATCH] clk: xilinx: fix NULL pointer dereference in xvcu_clk_hw_unregister_leaf()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: xilinx: Use-After-Free due to incorrect child/parent unregister order
- [High] clk: xilinx: UAF race during removal due to mixed devm/manual cleanup
--

commit 7f74ba5d4aed358cc163ed1eb7e56a50f1f385b0
Author: Haotian Zhang <vulab@iscas.ac.cn>

clk: xilinx: fix NULL pointer dereference in xvcu_clk_hw_unregister_leaf()

This commit addresses a NULL pointer dereference when a divider has no parent
by checking that the mux is non-NULL before calling clk_hw_unregister_mux().
It also removes dead code that erroneously checked the divider instead of the
mux.

> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> index ccd9d7c0b1809..58a9762ef20a9 100644
> --- a/drivers/clk/xilinx/xlnx_vcu.c
> +++ b/drivers/clk/xilinx/xlnx_vcu.c
> @@ -516,9 +516,8 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
>  		return;
>  
>  	mux = clk_hw_get_parent(divider);
> -	clk_hw_unregister_mux(mux);
> -	if (!divider)
> -		return;
> +	if (mux)
> +		clk_hw_unregister_mux(mux);
>  
>  	clk_hw_unregister_divider(divider);

[Severity: High]
Does this unregister the parent mux before the child divider, leading to a
use-after-free?

Calling clk_hw_unregister_mux() first reparents the divider to the orphan
list, but leaves divider->core->parents[0].hw pointing to the mux struct before
the memory is freed by the mux unregistration.

If a concurrent thread registers a new clock, the clock framework invokes
clk_core_reparent_orphans_nolock() to try resolving orphans. It iterates the
orphan list, finds the divider, and calls clk_core_fill_parent_index(), which
reads the dangling entry->hw pointer and dereferences it:

drivers/clk/clk.c:clk_core_fill_parent_index() {
    ...
	if (entry->hw) {
		parent = entry->hw->core;
    ...
}

Should the teardown strictly follow leaf-to-root order (gate -> divider -> mux)
to prevent orphaned children from dereferencing freed parent pointers during
concurrent reparenting?


[Severity: High]
Does this manual cleanup race with the devres-managed OF provider removal?

Because xvcu_register_clock_provider() registers the OF clock provider using
devm_of_clk_add_hw_provider(), the OF provider isn't removed from the global
clock provider list until the devres cleanup phase, which executes after
xvcu_remove() returns.

By manually freeing the hardware structures here during xvcu_remove(), the
active OF provider is left with dangling pointers in its internal hws array.
If a concurrent consumer invokes clk_get() during this window,
of_clk_hw_onecell_get() could yield a freed clk_hw pointer, resulting in a
use-after-free in the clock framework.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009061805.3172589-1-vulab@iscas.ac.cn?part=1

Re: [PATCH] clk: xilinx: fix NULL pointer dereference in xvcu_clk_hw_unregister_leaf()

[Jerome Brunet]

On ven. 09 oct. 2026 at 06:33, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] clk: xilinx: Use-After-Free due to incorrect child/parent unregister order
> - [High] clk: xilinx: UAF race during removal due to mixed devm/manual cleanup
> --
>
> commit 7f74ba5d4aed358cc163ed1eb7e56a50f1f385b0
> Author: Haotian Zhang <vulab@iscas.ac.cn>
>
> clk: xilinx: fix NULL pointer dereference in xvcu_clk_hw_unregister_leaf()
>
> This commit addresses a NULL pointer dereference when a divider has no parent
> by checking that the mux is non-NULL before calling clk_hw_unregister_mux().
> It also removes dead code that erroneously checked the divider instead of the
> mux.
>
>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
>> index ccd9d7c0b1809..58a9762ef20a9 100644
>> --- a/drivers/clk/xilinx/xlnx_vcu.c
>> +++ b/drivers/clk/xilinx/xlnx_vcu.c
>> @@ -516,9 +516,8 @@ static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
>>  		return;
>>  
>>  	mux = clk_hw_get_parent(divider);
>> -	clk_hw_unregister_mux(mux);

should deregister the divider here

>> -	if (!divider)
>> -		return;
>> +	if (mux)
>> +		clk_hw_unregister_mux(mux);
>>  
>>  	clk_hw_unregister_divider(divider);

this last line should be removed

>
> [Severity: High]
> Does this unregister the parent mux before the child divider, leading to a
> use-after-free?

Yes, the function clearly has a copy paste error than you should address properly.

>
> Calling clk_hw_unregister_mux() first reparents the divider to the orphan
> list, but leaves divider->core->parents[0].hw pointing to the mux struct before
> the memory is freed by the mux unregistration.
>
> If a concurrent thread registers a new clock, the clock framework invokes
> clk_core_reparent_orphans_nolock() to try resolving orphans. It iterates the
> orphan list, finds the divider, and calls clk_core_fill_parent_index(), which
> reads the dangling entry->hw pointer and dereferences it:
>
> drivers/clk/clk.c:clk_core_fill_parent_index() {
>     ...
> 	if (entry->hw) {
> 		parent = entry->hw->core;
>     ...
> }
>
> Should the teardown strictly follow leaf-to-root order (gate -> divider -> mux)
> to prevent orphaned children from dereferencing freed parent pointers during
> concurrent reparenting?
>
>
> [Severity: High]
> Does this manual cleanup race with the devres-managed OF provider removal?
>
> Because xvcu_register_clock_provider() registers the OF clock provider using
> devm_of_clk_add_hw_provider(), the OF provider isn't removed from the global
> clock provider list until the devres cleanup phase, which executes after
> xvcu_remove() returns.
>
> By manually freeing the hardware structures here during xvcu_remove(), the
> active OF provider is left with dangling pointers in its internal hws array.
> If a concurrent consumer invokes clk_get() during this window,
> of_clk_hw_onecell_get() could yield a freed clk_hw pointer, resulting in a
> use-after-free in the clock framework.
>
>>  }
>
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261009061805.3172589-1-vulab@iscas.ac.cn?part=1

-- 
Jerome

Re: [PATCH] clk: xilinx: fix NULL pointer dereference in xvcu_clk_hw_unregister_leaf()

[Krzysztof Kozlowski]

On 09/10/2026 08:18, Haotian Zhang wrote:
> xvcu_clk_hw_unregister_leaf() gets the mux with
> clk_hw_get_parent(divider), which returns NULL when the divider has no
> parent, and passes it to clk_hw_unregister_mux() without checking it.
> The following check tests the wrong variable: divider has already been
> verified to be non-NULL at this point, so "if (!divider) return;" is
> dead code and a NULL mux is dereferenced, crashing in
> clk_hw_unregister_mux().
> 
> Only unregister the mux when it is non-NULL, and always release the
> divider.

So you fired up your OpenClaw or whatever LLM tool and don't bother to
slow down, right?

Best regards,
Krzysztof