Re: [PATCH] leds: lm3697: Fix out-of-bound access

[ultracoolguy]

 I agree with you. 

Attached patch with changes.



Oct 5, 2020, 12:13 by kabel@xxxxxxxxxxxx:

> On Sat, 3 Oct 2020 15:02:51 +0200 (CEST)
> ultracoolguy@xxxxxxxxxxxx wrote:
>
>> From 0dfd5ab647ccbc585c543d702b44d20f0e3fe436 Mon Sep 17 00:00:00 2001
>> From: Ultracoolguy <ultracoolguy@xxxxxxxxxxxx>
>> Date: Fri, 2 Oct 2020 18:27:00 -0400
>> Subject: [PATCH] leds:lm3697:Fix out-of-bound access
>>
>> If both led banks aren't used in device tree,
>> an out-of-bounds condition in lm3697_init occurs
>> because of the for loop assuming that all the banks are used.
>> Fix it by adding a variable that contains the number of used banks.
>>
>> Signed-off-by: Ultracoolguy <ultracoolguy@xxxxxxxxxxxx>
>> ---
>>  drivers/leds/leds-lm3697.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
>> index 024983088d59..a4ec2b6077e6 100644
>> --- a/drivers/leds/leds-lm3697.c
>> +++ b/drivers/leds/leds-lm3697.c
>> @@ -56,7 +56,7 @@ struct lm3697_led {
>>  struct ti_lmu_bank lmu_data;
>>  int control_bank;
>>  int enabled;
>> -	int num_leds;
>> +	int num_led_strings;
>>
>
> OK, I looked at the datasheet for this controlled. The controlled can
> control 3 LED strings, each having several LEDs connected in series.
> But only 2 different brightnesses can be set (control bank), so for each
> string there is a register setting which control bank should control it.
>
> The Control Bank is set via the `reg` DT property (reg=0 means
> Control Bank A, reg=1 means Control Bank B). The `led-sources`
> property defines which strings should be controlled by each bank.
>
> So I think this variable name should stay num_leds (as in number of leds
> in this control bank).
> The structure though should be renamed:
>  struct lm3697_led  ->  struct lm3697_bank.
>
>> };
>>
>>  /**
>> @@ -78,6 +78,7 @@ struct lm3697 {
>>  struct mutex lock;
>>
>>  int bank_cfg;
>> +	int num_leds;
>>
>
> This should be named num_banks.
>
>>
>> struct lm3697_led leds[];
>>
>
> This variable should be named banks, i.e.:
>  struct lm3697_bank banks[];
>
>> };
>> @@ -180,7 +181,7 @@ static int lm3697_init(struct lm3697 *priv)
>>  if (ret)
>>  dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
>>
>> -	for (i = 0; i < LM3697_MAX_CONTROL_BANKS; i++) {
>> +	for (i = 0; i < priv->num_leds; i++) {
>>
>
>  if (!count)
> to
>  if (!count || count > LM3697_MAX_CONTROL_BANKS)
>
> (the error message should also be changed, or maybe dropped, and the
> error code changed from -ENODEV to -EINVAL, if we use || operator).
>
>> led = &priv->leds[i];
>>  ret = ti_lmu_common_set_ramp(&led->lmu_data);
>>  if (ret)
>> @@ -244,22 +245,22 @@ static int lm3697_probe_dt(struct lm3697 *priv)
>>  led->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB +
>>  led->control_bank * 2;
>>
>> -		led->num_leds = fwnode_property_count_u32(child, "led-sources");
>> -		if (led->num_leds > LM3697_MAX_LED_STRINGS) {
>> +		led->num_led_strings = fwnode_property_count_u32(child, "led-sources");
>> +		if (led->num_led_strings > LM3697_MAX_LED_STRINGS) {
>>  dev_err(&priv->client->dev, "Too many LED strings defined\n");
>>  continue;
>>  }
>>
>>  ret = fwnode_property_read_u32_array(child, "led-sources",
>>  led->hvled_strings,
>> -						    led->num_leds);
>> +						    led->num_led_strings);
>>  if (ret) {
>>  dev_err(&priv->client->dev, "led-sources property missing\n");
>>  fwnode_handle_put(child);
>>  goto child_out;
>>  }
>>
>> -		for (j = 0; j < led->num_leds; j++)
>> +		for (j = 0; j < led->num_led_strings; j++)
>>  priv->bank_cfg |=
>>  (led->control_bank << led->hvled_strings[j]);
>>
>> @@ -317,6 +318,8 @@ static int lm3697_probe(struct i2c_client *client,
>>  if (!led)
>>  return -ENOMEM;
>>
>> +	led->num_leds = count;
>> +
>>  mutex_init(&led->lock);
>>  i2c_set_clientdata(client, led);
>>
>> --
>> 2.28.0
>>
>
> Marek
>