Re: [PATCH RFC 07/16] clk: qcom: Add clock-owned GenPD DVFS aggregation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] clk: qcom: Hardware undervolted on actual_rate lookup failure
- [High] clk: qcom: TOCTOU race in qcom_clk_dvfs_cleanup() leads to UAF
- [High] clk: qcom: permanent DVFS domain bricking on failed performance state
- [Low] cleanup: mixed goto-based error handling and __free() in qcom_clk_dvfs_resource_acquire()
--

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

    clk: qcom: Add clock-owned GenPD DVFS aggregation
    
    Clocks on Qualcomm SoCs need a minimum power rail performance state to
    operate at a given frequency. We currently leave this to clock
    consumers, so a driver has to know which power domains supply both the
    clock and its parents. This ties knowledge of the clock tree to
    individual consumers and doesn't protect other users that prepare or
    change the same clock without making those power requests.

> diff --git a/drivers/clk/qcom/clk-dvfs.c b/drivers/clk/qcom/clk-dvfs.c
> new file mode 100644
> index 0000000000000..e6490c4dab949
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-dvfs.c
> @@ -0,0 +1,574 @@
[ ... ]
> +static unsigned int
> +qcom_clk_dvfs_aggregate(struct qcom_clk_dvfs_rail_voter *rail_voter)
> +{
> +	struct qcom_clk_dvfs_vote *vote;
> +	unsigned int level = 0;
> +
> +	list_for_each_entry(vote, &rail_voter->votes, node)
> +		level = max(level, vote->level);
> +
> +	return max(level, rail_voter->safety_floor_level);
> +}
[ ... ]
> +static int qcom_clk_dvfs_apply(struct qcom_clk_dvfs_rail_voter *rail_voter,
> +			       unsigned int level)
> +{
> +	int ret;
> +
> +	ret = qcom_clk_dvfs_activate(rail_voter);
> +	if (ret)
> +		return ret;
> +	ret = dev_pm_genpd_set_performance_state(rail_voter->dev, level);
> +	if (ret) {
> +		rail_voter->safety_floor_level =
> +			max(rail_voter->safety_floor_level,
> +			    max(rail_voter->last_successful_vote_level, level));
> +		return ret;

[Severity: High]
Does this code permanently limit the DVFS domain after a failed update?

If dev_pm_genpd_set_performance_state() fails in qcom_clk_dvfs_apply(),
the failed level is incorporated into safety_floor_level. 

qcom_clk_dvfs_aggregate() will then always return at least this level for any
future updates, forcing the domain to retry a known-bad state. This could
permanently break frequency scaling for the domain and leak a PM runtime
reference because the domain can never reach level 0 to suspend.

> +	}
> +
> +	rail_voter->last_successful_vote_level = level;
> +	rail_voter->safety_floor_level = 0;
> +
> +	return 0;
> +}
[ ... ]
> +int qcom_clk_dvfs_resource_acquire(struct clk_hw *hw,
> +				 const struct clk_resource_transition *transition,
> +				 void **cookie)
> +{
> +	struct qcom_clk_dvfs_clock *clock = to_clk_regmap(hw)->dvfs;
> +	struct qcom_clk_dvfs_transition *dvfs_transition __free(kfree) = NULL;
> +	struct qcom_clk_dvfs_vote *votes __free(kfree) = NULL;
> +	unsigned int level;
> +	size_t i;
> +	int ret;
> +
> +	if (!clock) {
> +		*cookie = NULL;
> +		return 0;
> +	}
> +
> +	dvfs_transition = kzalloc_obj(*dvfs_transition);
> +	if (!dvfs_transition)
> +		return -ENOMEM;
> +
> +	votes = kcalloc(clock->data->num_pd_indices, sizeof(*votes),
> +			 GFP_KERNEL);
> +	if (!votes)
> +		return -ENOMEM;
> +
> +	dvfs_transition->votes = votes;
> +	dvfs_transition->num_votes = clock->data->num_pd_indices;
> +	ret = qcom_clk_dvfs_find_level(clock->data, transition->new_rate, &level);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < dvfs_transition->num_votes; i++) {
> +		unsigned int pd_index = clock->data->pd_indices[i];
> +
> +		ret = qcom_clk_dvfs_add_vote(&clock->rail_voters[pd_index],
> +						  &dvfs_transition->votes[i], level);
> +		if (ret)
> +			goto unwind;
> +	}
> +
> +	dvfs_transition->votes = no_free_ptr(votes);
> +	*cookie = no_free_ptr(dvfs_transition);
> +
> +	return 0;
> +
> +unwind:
> +	while (i--) {

[Severity: Low]
This isn't a bug, but does this function mix goto error handling with __free()?

It appears qcom_clk_dvfs_resource_acquire() uses __free(kfree) for
dvfs_transition and votes while also using a "goto unwind;" path for error
cleanup. The cleanup subsystem guide recommends converting either all resources
to scope-based cleanup or none, rather than mixing both paradigms in the same
function.

> +		unsigned int pd_index = clock->data->pd_indices[i];
> +
> +		qcom_clk_dvfs_remove_vote(&clock->rail_voters[pd_index],
> +					    &dvfs_transition->votes[i]);
> +	}
> +
> +	return ret;
> +}
> +
> +int qcom_clk_dvfs_resource_release(struct clk_hw *hw,
> +				 const struct clk_resource_transition *transition,
> +				 void *cookie,
> +				 enum clk_resource_change_result result)
> +{
> +	struct qcom_clk_dvfs_transition *dvfs_transition = cookie;
> +	struct qcom_clk_dvfs_clock *clock = to_clk_regmap(hw)->dvfs;
> +	int first_error = 0;
> +	unsigned int level;
> +	int ret;
> +
> +	if (!clock)
> +		return 0;
> +	if (result == CLK_RESOURCE_COMPLETE) {
> +		ret = qcom_clk_dvfs_find_level(clock->data, transition->actual_rate,
> +					   &level);
> +		if (ret)
> +			first_error = ret;
> +	}
> +
> +	for (size_t i = 0; i < dvfs_transition->num_votes; i++) {
> +		unsigned int pd_index = clock->data->pd_indices[i];
> +
> +		if (!clock->votes[i].level)
> +			continue;
> +
> +		if (result == CLK_RESOURCE_COMPLETE) {
> +			ret = first_error;

[Severity: High]
Can this undervolt the hardware on an actual_rate lookup failure?

If qcom_clk_dvfs_find_level() fails to look up the level for the new rate
(setting first_error to a non-zero value), this loop sets ret = first_error.
Because ret is non-zero, it skips qcom_clk_dvfs_replace_vote(), leaving the
permanent vote at the old, lower level. 

Then, the temporary high transition vote is removed. Because the hardware
is already running at the higher actual_rate, the resulting lower aggregate
voltage vote could cause an immediate undervolt.

> +		} else if (result == CLK_RESOURCE_HW_CHANGED) {
> +			level = max(clock->votes[i].level,
> +				    dvfs_transition->votes[i].level);
> +			ret = 0;
> +		} else {
> +			continue;
> +		}
> +
> +		if (!ret) {
> +			ret = qcom_clk_dvfs_replace_vote(&clock->rail_voters[pd_index],
> +							 &clock->votes[i], level);
> +		}
> +		if (ret && !first_error)
> +			first_error = ret;
> +	}
[ ... ]
> +static void qcom_clk_dvfs_cleanup(void *data)
> +{
> +	struct qcom_clk_dvfs *dvfs = data;
> +
> +	for (size_t i = 0; i < dvfs->num_clocks; i++)
> +		dvfs->clocks[i].rclk->dvfs = NULL;

[Severity: High]
Can this race with consumer unprepare operations?

If a consumer holds a reference and unbinds, devres executes
qcom_clk_dvfs_cleanup(), which nullifies rclk->dvfs without taking the CCF
prepare_lock. 

A concurrent clk_unprepare() could read the non-NULL pointer, and if
qcom_clk_dvfs_cleanup() then proceeds to destroy rail_voter->lock and free
memory, qcom_clk_dvfs_unprepare() would cause a use-after-free when it tries
to lock the destroyed mutex in qcom_clk_dvfs_replace_vote().

> +
> +	for (size_t i = dvfs->num_rail_voters; i-- > 0;) {
> +		struct qcom_clk_dvfs_rail_voter *rail_voter = &dvfs->rail_voters[i];
> +
> +		if (rail_voter->active) {
> +			dev_pm_genpd_set_performance_state(rail_voter->dev, 0);
> +			WRITE_ONCE(rail_voter->must_contribute, false);
> +			pm_runtime_put_sync_suspend(rail_voter->dev);
> +		}
> +		dev_pm_genpd_remove_notifier(rail_voter->dev);
> +		device_link_del(rail_voter->link);
> +		dev_pm_domain_detach(rail_voter->dev, true);
> +		mutex_destroy(&rail_voter->lock);
> +	}
> +}

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