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

[sashiko-bot@kernel.org]

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