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

[Jerome Brunet <jbrunet@baylibre.com>]

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