Re: [PATCH RFC 08/16] clk: qcom: dvfs: Add debugfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: qcom: dvfs: UAF in debugfs due to missing file cleanup on unbind
- [Medium] cleanup: goto-based error handling mixed with __free() cleanup
--

commit 3f9e96971d4a45f08b00bd913e791d8aeb02fc13
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

clk: qcom: dvfs: Add debugfs

It's really useful to be able to analyze the current set of votes and
what holds them. Add debugfs to expose that information.

> diff --git a/drivers/clk/qcom/clk-dvfs.c b/drivers/clk/qcom/clk-dvfs.c
> index e6490c4dab949..4af78eb0c78dd 100644
> --- a/drivers/clk/qcom/clk-dvfs.c
> +++ b/drivers/clk/qcom/clk-dvfs.c

[ ... ]

> @@ -293,6 +333,8 @@ int qcom_clk_dvfs_resource_acquire(struct clk_hw *hw,
>  	for (i = 0; i < dvfs_transition->num_votes; i++) {
>  		unsigned int pd_index = clock->data->pd_indices[i];
>  
> +		dvfs_transition->votes[i].clock_name = clk_hw_get_name(hw);
> +		dvfs_transition->votes[i].transition = true;
>  		ret = qcom_clk_dvfs_add_vote(&clock->rail_voters[pd_index],
>  						  &dvfs_transition->votes[i], level);
>  		if (ret)

[Severity: Medium]
Is it safe to mix goto-based error handling with scope-based cleanups in this
function?

qcom_clk_dvfs_resource_acquire() declares `dvfs_transition` and `votes` with
`__free(kfree)`, but uses a `goto unwind;` label to perform a manual rollback
loop on error.

The kernel cleanup subsystem guidelines strictly prohibit mixing goto-based
error handling with scope-based `__free()` or `guard()` cleanups within the
same function. This pattern creates confusing ownership semantics and increases
the risk of double-free or resource leak bugs during future refactoring.

Should all resources in the function use either scope-based cleanup or manual
goto-based cleanup, rather than both?

[ ... ]

> @@ -565,7 +609,20 @@ int qcom_clk_dvfs_register(struct device *dev,
>  
>  	dev_set_drvdata(dev, dvfs);
>  
> -	return devm_add_action_or_reset(dev, qcom_clk_dvfs_cleanup, dvfs);
> +	ret = devm_add_action_or_reset(dev, qcom_clk_dvfs_cleanup, dvfs);
> +	if (ret)
> +		return ret;
> +
> +	scoped_guard(mutex, &qcom_clk_dvfs_debugfs_lock) {
> +		if (!qcom_clk_dvfs_debugfs_root)
> +			qcom_clk_dvfs_debugfs_root = debugfs_create_dir("qcom_clk_dvfs", NULL);
> +
> +		debugfs_create_devm_seqfile(dvfs->dev, dev_name(dvfs->dev),
> +					    qcom_clk_dvfs_debugfs_root,
> +					    qcom_clk_dvfs_debugfs_show);
> +	}
> +
> +	return 0;

[Severity: High]
Can this cause a Use-After-Free during device unbind?

The driver creates a debugfs file using debugfs_create_devm_seqfile() and
places it under the global static qcom_clk_dvfs_debugfs_root directory, which
is never removed.

debugfs_create_devm_seqfile() allocates a private entry structure via
devm_kzalloc(), but does not register a devm action to remove the debugfs file
itself. It relies on the removal of the parent directory.

Upon device unbind, devres frees the private entry structure, but the debugfs
file remains accessible because its parent directory is not removed and the
driver's cleanup path (qcom_clk_dvfs_cleanup) does not manually remove it.

A subsequent read of the leaked debugfs file by a user will dereference the
freed entry structure in debugfs_devm_entry_open(), causing a Use-After-Free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-topic-qc_clk_dvfs-v1-0-3041b788e9f7@oss.qualcomm.com?part=8