Re: [PATCH] scsi: ufs: Fix wild pointer dereference in ufshcd_clkscale_enable_store()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: ufs: ufshcd_clkscale_enable_store() mutates state before list_empty()
- [Medium] scsi: ufs: inconsistent empty clk_list_head handling in ufshcd_clkscale_enable_store()
--

commit 61a6e6195466b512ac30d8e2c7bbb0da09b2167e
Author: Wang Haoran <haoranwangsec@gmail.com>

scsi: ufs: Fix wild pointer dereference in ufshcd_clkscale_enable_store()

This commit adds a list_empty() check on hba->clk_list_head in
ufshcd_clkscale_enable_store() before calling list_first_entry(), returning
-EINVAL via the out_rel path when the clock list is empty.

>     scsi: ufs: Fix wild pointer dereference in ufshcd_clkscale_enable_store()
>     
>     ufshcd_clkscale_enable_store() calls
>     list_first_entry(&hba->clk_list_head, ...) and immediately dereferences
>     the result with clki->max_freq, without checking whether the clock list
>     is empty. list_first_entry() does not return NULL on an empty list: it
>     returns container_of() applied to the list head, i.e. a non-NULL wild
>     pointer pointing at &hba->clk_list_head itself, so the ->max_freq read
>     lands on adjacent fields of struct ufs_hba and yields a garbage value /
>     out-of-bounds access. The clock list can legitimately be empty on this
>     path: it starts out empty (INIT_LIST_HEAD() in ufshcd_alloc_host()) and
>     ufshcd_parse_clock_info() returns success leaving it empty when the
>     device tree has no clock-names property, while UFSHCD_CAP_CLK_SCALING is
>     a software capability bit set independently by vendor/platform code, so
>     a "cap set but no clocks" configuration is possible. In that state
>     ufshcd_devfreq_init() explicitly returns success for an empty list and
>     ufshcd_async_scan() still creates the clkscale_enable sysfs attribute
>     with is_enabled = true, so a simple "echo 0 > .../clkscale_enable"
>     reaches the unguarded list_first_entry() and dereferences the wild
>     pointer. Notably, all five sibling functions in this file that access
>     clk_list_head (ufshcd_print_clk_freqs(), ufshcd_setup_clocks(),
>     ufshcd_init_clocks(), ufshcd_devfreq_init(), ufshcd_devfreq_scale())
>     guard with list_empty() first - only this one was missed, which suggests
>     an oversight rather than a deliberate design. This potential issue was
>     found during a code audit.
>     
>     Fix this by adding a list_empty() guard before the list_first_entry()
>     call, returning -EINVAL via the existing out_rel error path when the
>     clock list is empty, exactly matching how the sibling functions handle
>     the same condition. Behaviour with a non-empty clock list is unchanged.
>     
>     Reported-by: Wang Haoran <haoranwangsec@gmail.com>
>     Reported-by: Li Ximo <lixicaicai@gmail.com>
>     Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>

[Severity: Medium]
The commit message states that all five sibling functions in
drivers/ufs/core/ufshcd.c that access clk_list_head (including
ufshcd_devfreq_scale()) guard with list_empty() first, and that returning
-EINVAL matches how the sibling functions handle an empty clock list. Is this
description accurate?

Looking at drivers/ufs/core/ufshcd.c, ufshcd_devfreq_scale() does not access
clk_list_head directly, while functions that do check list_empty() treat an
empty list as a no-op rather than returning -EINVAL. For example, in
ufshcd_devfreq_init():

	/* Skip devfreq if we don't have any clocks in the list */
	if (list_empty(clk_list))
		return 0;

Similarly, ufshcd_setup_clocks(), ufshcd_init_clocks(), ufshcd_set_clk_freq(),
and ufshcd_devfreq_target() return 0 when clk_list_head is empty, and
ufshcd_is_devfreq_scaling_required() returns false:

	if (list_empty(head))
		return false;

In ufshcd_clkscale_enable_store(), when ufshcd_is_devfreq_scaling_required()
returns false after ufshcd_suspend_clkscaling(hba), the function jumps to
out_rel with err = 0 rather than -EINVAL.

Could the commit message and empty-list handling in
ufshcd_clkscale_enable_store() be aligned with how these sibling functions
treat an empty clock list?

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cc75384c65021..a55226695ad13 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1874,6 +1874,11 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>  		goto out_rel;
>  	}
>  
> +	if (list_empty(&hba->clk_list_head)) {
> +		err = -EINVAL;
> +		goto out_rel;
> +	}
> +
>  	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
>  	freq = clki->max_freq;
>  

[Severity: Medium]
Does returning -EINVAL here leave hba->clk_scaling.is_enabled in a mutated
state when disabling clock scaling with an empty hba->clk_list_head?

In drivers/ufs/core/ufshcd.c, ufshcd_clkscale_enable_store() updates
hba->clk_scaling.is_enabled and handles the enable path before checking
list_empty(&hba->clk_list_head):

	value = !!value;
	if (value == hba->clk_scaling.is_enabled)
		goto out;

	ufshcd_rpm_get_sync(hba);
	ufshcd_hold(hba);

	hba->clk_scaling.is_enabled = value;

	if (value) {
		ufshcd_resume_clkscaling(hba);
		goto out_rel;
	}

	if (list_empty(&hba->clk_list_head)) {
		err = -EINVAL;
		goto out_rel;
	}

When hba->clk_list_head is empty and a user writes 0 to clkscale_enable:

1. hba->clk_scaling.is_enabled is set to 0 before the list_empty() check
   returns -EINVAL, so subsequent reads via ufshcd_clkscale_enable_show()
   report 0 even though the write failed.

2. A second write of 0 matches (value == hba->clk_scaling.is_enabled) and
   returns count (success) instead of -EINVAL.

3. A subsequent write of 1 sets hba->clk_scaling.is_enabled = 1, calls
   ufshcd_resume_clkscaling(hba), and jumps to out_rel before reaching the
   list_empty() check.

4. Because ufshcd_clkscale_enable_store() jumps to out_rel before calling
   ufshcd_suspend_clkscaling(hba) while leaving hba->clk_scaling.is_enabled
   as false, subsequent suspend transitions in ufshcd_clk_scaling_suspend()
   will also skip ufshcd_suspend_clkscaling(hba):

	if (suspend) {
		if (hba->clk_scaling.is_enabled)
			ufshcd_suspend_clkscaling(hba);
		ufshcd_clk_scaling_allow(hba, false);

Should the empty clock list case either be checked before mutating
hba->clk_scaling.is_enabled, or skip only the list_first_entry() and clock
scaling steps after ufshcd_suspend_clkscaling(hba) so that state transitions
remain consistent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021052042.3441449-1-lixicaicai@gmail.com?part=1