Re: [PATCH v4] 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:
- [High] Use-after-free and double-free of LED pattern data due to missing ownership transfer
- [Medium] Subsystem guidelines violation: mixing goto and scope-based cleanup
--

commit 902ea70efa572e9fe23362e3de394a74dc4bce83
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

    leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode
    
    Currently, when the LED is configured as a RGB LED or a multi-color
    LED device, the same pattern is programmed for all LED channels
    regardless of the sub-led intensities when triggered by HW pattern.
    It results that the LED device is always working in a white-balanced
    mode regardless of the intensity settings.
    
    To fix this, scale the pattern data according to the sub-led intensity
    and program the HW pattern separately for each LPG channel.

> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index d7d6518de30f2..e407742ec3b31 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c

[ ... ]

> @@ -995,15 +1005,16 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>  	if (len % 2)
>  		return -EINVAL;
>  
> -	pattern = kzalloc_objs(*pattern, len / 2);
> +	struct led_pattern *pattern __free(kfree) = kzalloc_objs(*pattern, len / 2);
> +
>  	if (!pattern)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < len; i += 2) {

[ ... ]

>  	if (lpg->lut_base || lpg->lut_sdam) {
> -		lo_pause = pattern[0].delta_t;
> -		hi_pause = pattern[actual_len - 1].delta_t;
> +		prep->lo_pause = pattern[0].delta_t;
> +		prep->hi_pause = pattern[actual_len - 1].delta_t;
>  	} else {
>  		if (delta_t != pattern[0].delta_t || delta_t != pattern[actual_len - 1].delta_t)
> -			goto out_free_pattern;
> +			return -EINVAL;
> +		prep->lo_pause = 0;
> +		prep->hi_pause = 0;
>  	}
>  
> +	prep->data = pattern;
> +	prep->len = actual_len;
> +	prep->delta_t = delta_t;
> +	prep->ping_pong = ping_pong;
>  
> -	mutex_lock(&lpg->lock);
> +	return 0;
> +}

[Severity: High]
Does this code inadvertently free the pattern buffer? Since pattern was
allocated with the __free(kfree) attribute, it will be automatically freed
upon returning from lpg_prepare_pattern(). The caller receives a dangling
pointer in prep->data. Should this use no_free_ptr(pattern) to transfer
ownership to the lpg_pattern structure?

> +
> +static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
> +			   u32 len, int repeat)
> +{
> +	struct lpg_channel *chan;
> +	struct lpg *lpg = led->lpg;
> +	struct lpg_pattern pattern;
> +	unsigned int lo_idx;
> +	unsigned int hi_idx;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = lpg_prepare_pattern(lpg, led_pattern, len, repeat, &pattern);
> +	if (ret < 0)
> +		return ret;
> +
> +	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: Medium]
Is it okay to mix guard(mutex) and goto-based error handling in the same
function? The subsystem guidelines for cleanup recommend converting all
resources in a function to scope-based cleanup or converting none of them,
as mixing the two approaches creates confusing ownership semantics.

[ ... ]

> -out_unlock:
> -	mutex_unlock(&lpg->lock);
> -out_free_pattern:
> -	kfree(pattern);
> -
> +free_pattern:
> +	kfree(pattern.data);

[Severity: High]
Will this result in a double-free? If pattern.data was already freed when
lpg_prepare_pattern() returned due to the __free() attribute, calling
kfree() here on the same pointer could lead to memory corruption.

>  	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)
>  {
>  	struct led_classdev_mc *mc = lcdev_to_mccdev(cdev);
>  	struct lpg_led *led = container_of(mc, struct lpg_led, mcdev);
> +	struct lpg *lpg = led->lpg;
> +	struct lpg_channel *chan;
> +	struct lpg_pattern pattern;
>  	unsigned int triled_mask = 0;
> -	int ret, i;
> +	unsigned int lo_idx;
> +	unsigned int hi_idx;
> +	unsigned int scale;
> +	unsigned int i, j;
> +	int ret;
> +
> +	ret = lpg_prepare_pattern(lpg, led_pattern, len, repeat, &pattern);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* 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) {
> +		kfree(pattern.data);
> +		return -ENOMEM;
> +	}
>  
>  	for (i = 0; i < led->num_channels; i++)
>  		triled_mask |= led->channels[i]->triled_mask;
> -	triled_set(led->lpg, triled_mask, 0);
> +	triled_set(lpg, triled_mask, 0);
>  
> -	ret = lpg_pattern_set(led, pattern, len, repeat);
> +	led_mc_calc_color_components(mc, LED_FULL);
> +
> +	/*
> +	 * Each channel gets its own LUT block scaled by subled_info[i].brightness
> +	 * so the pattern respects the configured colour balance.
> +	 */
> +	guard(mutex)(&lpg->lock);

[Severity: Medium]
This function also mixes __free(kfree), guard(mutex) and goto-based
cleanup labels. Does this violate the subsystem cleanup guidelines?

> +
> +	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: High]
Could this result in a use-after-free? Because lpg_prepare_pattern() uses
scope-based cleanup on the pattern buffer without transferring ownership,
the memory backing pattern.data is already freed by the time we attempt to
read its contents here.

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