Re: [PATCH v3] leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Failing to inhibit cleanup of the __free() 'pattern' variable in lpg_prepare_pattern() causes it to be freed on success, leading to Use-After-Free and Double-Free in callers.
- [Low] Mixing goto-based error handling with scope-based cleanup (__free and guard) violates subsystem guidelines and causes a compilation error.
--

commit 6d61dc847ec65d99f5fb5a8faf11368f527f4b5c
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode

This patch updates the LED pattern programming to scale pattern data
according to sub-LED intensities. This ensures proper white balancing
when using hardware patterns.

> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
[ ... ]
> @@ -959,23 +977,15 @@ static int lpg_blink_mc_set(struct led_classdev *cdev,
> +static int lpg_prepare_pattern(struct lpg *lpg, struct led_pattern *led_pattern,
> +				u32 len, int repeat, struct lpg_pattern *prep)
>  {
[ ... ]
> -	pattern = kzalloc_objs(*pattern, len / 2);
> +	struct led_pattern *pattern __free(kfree) = kzalloc_objs(*pattern, len / 2);
> +
>  	if (!pattern)
>  		return -ENOMEM;
[ ... ]
> +	prep->data = pattern;
> +	prep->len = actual_len;
> +	prep->delta_t = delta_t;
> +	prep->ping_pong = ping_pong;
>  
> -	mutex_lock(&lpg->lock);
> +	return 0;
> +}

[Severity: Critical]
Since pattern is declared with __free(kfree), does returning 0 without
inhibiting cleanup cause pattern to be freed immediately upon function
exit? Could this be resolved by using no_free_ptr(pattern) when assigning
it to prep->data?

> +static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
> +			   u32 len, int repeat)
> +{
[ ... ]
> +	guard(mutex)(&lpg->lock);
>  
>  	if (lpg->lut_base)
> -		ret = lpg_lut_store(lpg, pattern, actual_len, &lo_idx, &hi_idx);
> +		ret = lpg_lut_store(lpg, pattern.data, pattern.len, &lo_idx, &hi_idx);
>  	else
> -		ret = lpg_lut_store_sdam(lpg, pattern, actual_len, &lo_idx, &hi_idx);
> +		ret = lpg_lut_store_sdam(lpg, pattern.data, pattern.len, &lo_idx, &hi_idx);
>  
>  	if (ret < 0)
> -		goto out_unlock;
> +		goto free_pattern;

[Severity: Low]
Does mixing goto-based cleanup labels with scope-based cleanup like
guard(mutex) violate the kernel's cleanup guidelines? The guidelines
recommend either converting all resources to scope-based cleanup or not
using it at all in the same function.

[ ... ]
> -out_unlock:
> -	mutex_unlock(&lpg->lock);
> -out_free_pattern:
> -	kfree(pattern);
> -
> +free_pattern:
> +	kfree(pattern.data);
>  	return ret;
>  }

[ ... ]
>  static int lpg_pattern_mc_set(struct led_classdev *cdev,
> -			      struct led_pattern *pattern, u32 len,
> +			      struct led_pattern *led_pattern, u32 len,
>  			      int repeat)
>  {
[ ... ]
> +	/* Allocate buffer for the per-channel scaled pattern copy */
> +	struct led_pattern *scaled __free(kfree) =
> +			   kmalloc_array(pattern.len, sizeof(*scaled), GFP_KERNEL);
> +	if (!scaled) {
> +		ret = -ENOMEM;
> +		goto free_pattern;
> +	}

[Severity: Low]
Does jumping to free_pattern on this error path skip the declaration of
guard(mutex)(&lpg->lock) further down in the function? In C, jumping
forward over a variable initialized with a cleanup attribute results in a
compilation error.

[ ... ]
> +	guard(mutex)(&lpg->lock);
> +
> +	for (i = 0; i < led->num_channels; i++) {
> +		chan = led->channels[i];
> +		scale = mc->subled_info[i].brightness;
> +
> +		for (j = 0; j < pattern.len; j++) {
> +			scaled[j].brightness = DIV_ROUND_CLOSEST(
> +				(u32)pattern.data[j].brightness * scale, LED_FULL);

[Severity: Critical]
If pattern.data was already freed at the end of lpg_prepare_pattern(),
can this loop dereference freed memory?

> +			scaled[j].delta_t = pattern.data[j].delta_t;
> +		}
[ ... ]
> +free_pattern:
> +	kfree(pattern.data);

[Severity: Critical]
Would this unconditional kfree() trigger a double-free, since pattern.data
was already freed upon exiting lpg_prepare_pattern()?

>  	if (ret < 0)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-lpg-rgb-color-balance-fix-v3-1-17796a06d799@oss.qualcomm.com?part=1