[v1] leds: lp5523: fix out-of-bounds bug in lp5523_selftest()

[PATCH] leds: lp5523: fix out-of-bounds bug in lp5523_selftest()

Posted by Maarten Zanders 3 years, 5 months ago

When not all LED channels of the led chip are configured, the
sysfs selftest functionality gives erroneous results and tries to
test all channels of the chip.
There is a potential for LED overcurrent conditions since the
test current will be set to values from out-of-bound regions.

It is wrong to use pdata->led_config[i].led_current to skip absent
channels as led_config[] only contains the configured LED channels.

Instead of iterating over all the physical channels of the device,
loop over the available LED configurations and use led->chan_nr to
access the correct i2c registers. Keep the zero-check for the LED
current as existing users might depend on this to disable a channel.

Reported-by: Arne Staessen <a.staessen@televic.com>
Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
---
 drivers/leds/leds-lp5523.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 369d40b0b65b..e08e3de1428d 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -581,8 +581,8 @@ static ssize_t lp5523_selftest(struct device *dev,
 	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
 	struct lp55xx_chip *chip = led->chip;
 	struct lp55xx_platform_data *pdata = chip->pdata;
-	int i, ret, pos = 0;
-	u8 status, adc, vdd;
+	int ret, pos = 0;
+	u8 status, adc, vdd, i;
 
 	mutex_lock(&chip->lock);
 
@@ -612,20 +612,21 @@ static ssize_t lp5523_selftest(struct device *dev,
 
 	vdd--;	/* There may be some fluctuation in measurement */
 
-	for (i = 0; i < LP5523_MAX_LEDS; i++) {
-		/* Skip non-existing channels */
+	for (i = 0; i < pdata->num_channels; i++) {
+		/* Skip disabled channels */
 		if (pdata->led_config[i].led_current == 0)
 			continue;
 
 		/* Set default current */
-		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + i,
+		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + led->chan_nr,
 			pdata->led_config[i].led_current);
 
-		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + i, 0xff);
+		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr,
+			     0xff);
 		/* let current stabilize 2 - 4ms before measurements start */
 		usleep_range(2000, 4000);
 		lp55xx_write(chip, LP5523_REG_LED_TEST_CTRL,
-			     LP5523_EN_LEDTEST | i);
+			     LP5523_EN_LEDTEST | led->chan_nr);
 		/* ADC conversion time is 2.7 ms typically */
 		usleep_range(3000, 6000);
 		ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
@@ -633,20 +634,22 @@ static ssize_t lp5523_selftest(struct device *dev,
 			goto fail;
 
 		if (!(status & LP5523_LEDTEST_DONE))
-			usleep_range(3000, 6000);/* Was not ready. Wait. */
+			usleep_range(3000, 6000); /* Was not ready. Wait. */
 
 		ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &adc);
 		if (ret < 0)
 			goto fail;
 
 		if (adc >= vdd || adc < LP5523_ADC_SHORTCIRC_LIM)
-			pos += sprintf(buf + pos, "LED %d FAIL\n", i);
+			pos += sprintf(buf + pos, "LED %d FAIL\n",
+				       led->chan_nr);
 
-		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + i, 0x00);
+		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr,
+			     0x00);
 
 		/* Restore current */
-		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + i,
-			led->led_current);
+		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + led->chan_nr,
+			     led->led_current);
 		led++;
 	}
 	if (pos == 0)
-- 
2.37.3

Re: [PATCH] leds: lp5523: fix out-of-bounds bug in lp5523_selftest()

Posted by Pavel Machek 3 years, 5 months ago

Hi!

> When not all LED channels of the led chip are configured, the
> sysfs selftest functionality gives erroneous results and tries to
> test all channels of the chip.
> There is a potential for LED overcurrent conditions since the
> test current will be set to values from out-of-bound regions.
> 
> It is wrong to use pdata->led_config[i].led_current to skip absent
> channels as led_config[] only contains the configured LED channels.
> 
> Instead of iterating over all the physical channels of the device,
> loop over the available LED configurations and use led->chan_nr to
> access the correct i2c registers. Keep the zero-check for the LED
> current as existing users might depend on this to disable a channel.

Thanks, applied, I'll push eventually.

I wonder, should we do these kind of actions on attribute _read_? I'd
preffer some kind of write action to trigger this.

Best regards,
								Pavel

> Reported-by: Arne Staessen <a.staessen@televic.com>
> Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
> ---
>  drivers/leds/leds-lp5523.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 369d40b0b65b..e08e3de1428d 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -581,8 +581,8 @@ static ssize_t lp5523_selftest(struct device *dev,
>  	struct lp55xx_led *led = i2c_get_clientdata(to_i2c_client(dev));
>  	struct lp55xx_chip *chip = led->chip;
>  	struct lp55xx_platform_data *pdata = chip->pdata;
> -	int i, ret, pos = 0;
> -	u8 status, adc, vdd;
> +	int ret, pos = 0;
> +	u8 status, adc, vdd, i;
>  
>  	mutex_lock(&chip->lock);
>  
> @@ -612,20 +612,21 @@ static ssize_t lp5523_selftest(struct device *dev,
>  
>  	vdd--;	/* There may be some fluctuation in measurement */
>  
> -	for (i = 0; i < LP5523_MAX_LEDS; i++) {
> -		/* Skip non-existing channels */
> +	for (i = 0; i < pdata->num_channels; i++) {
> +		/* Skip disabled channels */
>  		if (pdata->led_config[i].led_current == 0)
>  			continue;
>  
>  		/* Set default current */
> -		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + i,
> +		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + led->chan_nr,
>  			pdata->led_config[i].led_current);
>  
> -		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + i, 0xff);
> +		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr,
> +			     0xff);
>  		/* let current stabilize 2 - 4ms before measurements start */
>  		usleep_range(2000, 4000);
>  		lp55xx_write(chip, LP5523_REG_LED_TEST_CTRL,
> -			     LP5523_EN_LEDTEST | i);
> +			     LP5523_EN_LEDTEST | led->chan_nr);
>  		/* ADC conversion time is 2.7 ms typically */
>  		usleep_range(3000, 6000);
>  		ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
> @@ -633,20 +634,22 @@ static ssize_t lp5523_selftest(struct device *dev,
>  			goto fail;
>  
>  		if (!(status & LP5523_LEDTEST_DONE))
> -			usleep_range(3000, 6000);/* Was not ready. Wait. */
> +			usleep_range(3000, 6000); /* Was not ready. Wait. */
>  
>  		ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &adc);
>  		if (ret < 0)
>  			goto fail;
>  
>  		if (adc >= vdd || adc < LP5523_ADC_SHORTCIRC_LIM)
> -			pos += sprintf(buf + pos, "LED %d FAIL\n", i);
> +			pos += sprintf(buf + pos, "LED %d FAIL\n",
> +				       led->chan_nr);
>  
> -		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + i, 0x00);
> +		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr,
> +			     0x00);
>  
>  		/* Restore current */
> -		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + i,
> -			led->led_current);
> +		lp55xx_write(chip, LP5523_REG_LED_CURRENT_BASE + led->chan_nr,
> +			     led->led_current);
>  		led++;
>  	}
>  	if (pos == 0)
> -- 
> 2.37.3

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

Re: [PATCH] leds: lp5523: fix out-of-bounds bug in lp5523_selftest()

Posted by Maarten Zanders 3 years, 5 months ago

Hi Pavel,

On 10/28/22 17:34, Pavel Machek wrote:
> I wonder, should we do these kind of actions on attribute _read_? I'd
> preffer some kind of write action to trigger this.
>
Yes I agree that doing these kind of things on a file read feels somehow 
strange. Strictly speaking, I guess you would implement an IOCTL to 
initiate the action and then perform a read to get the result. But that 
would needlessly complicate things on both ends? I'll keep it in mind 
and when I encounter similar interfaces in other drivers get back to 
this if needed.