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

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

[ultracoolguy]

[-- Attachment #1: Type: text/plain, Size: 3495 bytes --]

Signed-off-by: Ultracoolguy <ultracoolguy@tutanota.com>
Hi, all. This is a patch fixing an out-of-bounds error due to lm3697_init expecting the device tree to use both control banks.  This fixes it by adding a new variable that will hold the number of used banks.

Panic caused by this bug:

<1>[    3.059763] Unable to handle kernel NULL pointer dereference at virtual address 00000000000001e5
<1>[    3.059779] Mem abort info:
<1>[    3.059788]   ESR = 0x96000004
<1>[    3.059799]   EC = 0x25: DABT (current EL), IL = 32 bits
<1>[    3.059807]   SET = 0, FnV = 0
<1>[    3.059816]   EA = 0, S1PTW = 0
<1>[    3.059824] Data abort info:
<1>[    3.059833]   ISV = 0, ISS = 0x00000004
<1>[    3.059841]   CM = 0, WnR = 0
<1>[    3.059850] [00000000000001e5] user address but active_mm is swapper
<0>[    3.059864] Internal error: Oops: 96000004 [#1] PREEMPT SMP
<7>[    3.059874] Modules linked in:
<7>[    3.059893] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc7-postmarketos-qcom-msm8953 #71
<7>[    3.059903] Hardware name: Motorola G7 Power (ocean) (DT)
<7>[    3.059916] pstate: a0000005 (NzCv daif -PAN -UAO BTYPE=--)
<7>[    3.059937] pc : regmap_write+0x1c/0x78
<7>[    3.059952] lr : ti_lmu_common_set_ramp+0x60/0x70
<7>[    3.059961] sp : ffff800010043ad0
<7>[    3.059970] x29: ffff800010043ad0 x28: ffff0000b98510d0 
 <7>[    3.059982] x27: ffff0000b9851288 x26: ffff800010906cc0 
<7>[    3.059995] x25: ffff8000108e5d85 x24: 0000000000000001 
<7>[    3.060008] x23: 0000000000000000 x22: ffff0000b9851080 
<7>[    3.060020] x21: 0000000000000001 x20: 000000000000000f 
<7>[    3.060032] x19: 0000000000000001 x18: 0000000000000000 
<7>[    3.060045] x17: 00000000f2f3902a x16: 00000000ee02cbfb 
<7>[    3.060058] x15: 000000000000000a x14: 0000000000000307 
<7>[    3.060070] x13: ffffffffffffffff x12: ffffffffffffffff 
<7>[    3.060083] x11: 0000000000000000 x10: 0000000000000950 
<7>[    3.060095] x9 : ffff8000105864e4 x8 : ffff0000b2f089b0 
<7>[    3.060108] x7 : 0000000000000004 x6 : 000000000000033c 
<7>[    3.060120] x5 : ffff0000b98c51d0 x4 : 0000000000000000 
<7>[    3.060132] x3 : ffff0000b2f08000 x2 : 00000000000000ff 
<7>[    3.060145] x1 : 0000000000000000 x0 : 0000000000000001
<7>[    3.060158] Call trace:
<7>[    3.060172]  regmap_write+0x1c/0x78
<7>[    3.060183]  ti_lmu_common_set_ramp+0x60/0x70
<7>[    3.060195]  lm3697_probe+0x4d4/0x534
<7>[    3.060211]  i2c_device_probe+0x22c/0x294
<7>[    3.060225]  really_probe+0x1e0/0x468
<7>[    3.060237]  driver_probe_device+0xfc/0x140
<7>[    3.060250]  device_driver_attach+0x48/0x70
<7>[    3.060262]  __driver_attach+0x134/0x14c
<7>[    3.060275]  bus_for_each_dev+0x64/0xbc
<7>[    3.060287]  driver_attach+0x28/0x30
<7>[    3.060299]  bus_add_driver+0x110/0x1fc
<7>[    3.060312]  driver_register+0xa8/0xec
<7>[    3.060325]  i2c_register_driver+0x94/0xac
<7>[    3.060341]  lm3697_driver_init+0x20/0x28
<7>[    3.060356]  do_one_initcall+0xc4/0x228
<7>[    3.060368]  kernel_init_freeable+0x1e4/0x24c
<7>[    3.060384]  kernel_init+0x18/0x110
<7>[    3.060397]  ret_from_fork+0x10/0x18
<0>[    3.060415] Code: 910003fd a90153f3 aa0003f3 f90013f5 (b941e400) 
<4>[    3.060439] ---[ end trace fcc24bd799273148 ]---


[-- Attachment #2: 0001-leds-lm3697-Fix-out-of-bound-access.patch --]
[-- Type: text/x-patch, Size: 2515 bytes --]

From 0dfd5ab647ccbc585c543d702b44d20f0e3fe436 Mon Sep 17 00:00:00 2001
From: Ultracoolguy <ultracoolguy@tutanota.com>
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@tutanota.com>
---
 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;
 };

 /**
@@ -78,6 +78,7 @@ struct lm3697 {
 	struct mutex lock;

 	int bank_cfg;
+	int num_leds;

 	struct lm3697_led leds[];
 };
@@ -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++) {
 		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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

Hi!

> Signed-off-by: Ultracoolguy <ultracoolguy@tutanota.com>

I'll need your real name to apply a patch.

> Hi, all. This is a patch fixing an out-of-bounds error due to lm3697_init expecting the device tree to use both control banks.  This fixes it by adding a new variable that will hold the number of used banks.
> 
> Panic caused by this bug:

> <7>[    3.059893] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc7-postmarketos-qcom-msm8953 #71

Ok, so I assume this is only problem with certain device trees, and
not a problem with dts' in mainline?

This is not trivial patch, no need to cc trivial tree. OTOH Ccing
Marek who did a lot of cleanups in -next might be useful. Doing that
now.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[ultracoolguy]

>I'll need your real name to apply a patch.

My real name is Gabriel David.

>Ok, so I assume this is only problem with certain device trees, and
not a problem with dts' in mainline?

Yes. 
Here's the current node I'm using that causes this bug:

&i2c_5 {
        status = "ok";

        ti_lm3697@36 {
                        compatible = "ti,lm3697";
                        #address-cells = <1>;
                        #size-cells = <0>;
                        reg = <0x36>;

                        led@1 {
                                reg = <1>;
                                led-sources = <0 1 2>;
                                ti,brightness-resolution = <2047>;
                                label = "white:backlight";
                        };
        };
};

>This is not trivial patch, no need to cc trivial tree. OTOH Ccing
Marek who did a lot of cleanups in -next might be useful. Doing that
now.

Sorry for that. Gonna CC Marek from now on.

Btw thanks for the quick response!

Oct 3, 2020, 13:56 by pavel@ucw.cz:

> Hi!
>
>> Signed-off-by: Ultracoolguy <ultracoolguy@tutanota.com>
>>
>
> I'll need your real name to apply a patch.
>
>> Hi, all. This is a patch fixing an out-of-bounds error due to lm3697_init expecting the device tree to use both control banks.  This fixes it by adding a new variable that will hold the number of used banks.
>>
>> Panic caused by this bug:
>>
>> <7>[    3.059893] CPU: 3 PID: 1 Comm: swapper/0 Tainted: G        W         5.9.0-rc7-postmarketos-qcom-msm8953 #71
>>
>
> Ok, so I assume this is only problem with certain device trees, and
> not a problem with dts' in mainline?
>
> This is not trivial patch, no need to cc trivial tree. OTOH Ccing
> Marek who did a lot of cleanups in -next might be useful. Doing that
> now.
>
> Best regards,
>
>  Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

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

[Marek Behun]

On Sat, 3 Oct 2020 15:02:51 +0200 (CEST)
ultracoolguy@tutanota.com wrote:

> From 0dfd5ab647ccbc585c543d702b44d20f0e3fe436 Mon Sep 17 00:00:00 2001
> From: Ultracoolguy <ultracoolguy@tutanota.com>
> 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@tutanota.com>
> ---
>  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++) {

Ultracoolguy is correct that this for cycle should not iterate
LM3697_MAX_CONTROL_BANKS. Instead, the count check in lm3697_probe should be changed from

  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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

Hi!

> >  	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++) {
> 
> Ultracoolguy is correct that this for cycle should not iterate
> LM3697_MAX_CONTROL_BANKS. Instead, the count check in lm3697_probe should be changed from
> 
>   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).

I guess Dan (or someone else?) can submit simple one-liner I could
apply into -for-next (and maybe stable), and then we can sort the
naming etc in the driver? Gettings banks vs. LEDs right would be nice.

Thanks and best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[ultracoolguy]

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]

I agree with you. 

Attached patch with changes.



Oct 5, 2020, 12:13 by kabel@blackhole.sk:

> On Sat, 3 Oct 2020 15:02:51 +0200 (CEST)
> ultracoolguy@tutanota.com wrote:
>
>> From 0dfd5ab647ccbc585c543d702b44d20f0e3fe436 Mon Sep 17 00:00:00 2001
>> From: Ultracoolguy <ultracoolguy@tutanota.com>
>> 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@tutanota.com>
>> ---
>>  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
>


[-- Attachment #2: 0001-leds-lm3697-Fix-out-of-bound-access.patch --]
[-- Type: text/x-patch, Size: 7598 bytes --]

From 8382605d2eb7ff96625713d1f6400406c262e4c2 Mon Sep 17 00:00:00 2001
From: Ultracoolguy <ultracoolguy@tutanota.com>
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.
---
 drivers/leds/leds-lm3697.c | 97 ++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 47 deletions(-)

diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index 024983088d59..9cb83f568172 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -48,7 +48,7 @@
  * @control_bank: Control bank the LED is associated to. 0 is control bank A
  *		   1 is control bank B
  */
-struct lm3697_led {
+struct lm3697_bank {
 	u32 hvled_strings[LM3697_MAX_LED_STRINGS];
 	char label[LED_MAX_NAME_SIZE];
 	struct led_classdev led_dev;
@@ -78,8 +78,9 @@ struct lm3697 {
 	struct mutex lock;

 	int bank_cfg;
+	int num_banks;

-	struct lm3697_led leds[];
+	struct lm3697_bank banks[];
 };

 static const struct reg_default lm3697_reg_defs[] = {
@@ -112,52 +113,52 @@ static const struct regmap_config lm3697_regmap_config = {
 static int lm3697_brightness_set(struct led_classdev *led_cdev,
 				enum led_brightness brt_val)
 {
-	struct lm3697_led *led = container_of(led_cdev, struct lm3697_led,
+	struct lm3697_bank *bank = container_of(led_cdev, struct lm3697_bank,
 					      led_dev);
-	int ctrl_en_val = (1 << led->control_bank);
+	int ctrl_en_val = (1 << bank->control_bank);
 	int ret;

-	mutex_lock(&led->priv->lock);
+	mutex_lock(&bank->priv->lock);

 	if (brt_val == LED_OFF) {
 		ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE,
 					 ctrl_en_val, ~ctrl_en_val);
 		if (ret) {
-			dev_err(&led->priv->client->dev, "Cannot write ctrl register\n");
+			dev_err(&bank->priv->client->dev, "Cannot write ctrl register\n");
 			goto brightness_out;
 		}

-		led->enabled = LED_OFF;
+		bank->enabled = LED_OFF;
 	} else {
-		ret = ti_lmu_common_set_brightness(&led->lmu_data, brt_val);
+		ret = ti_lmu_common_set_brightness(&bank->lmu_data, brt_val);
 		if (ret) {
-			dev_err(&led->priv->client->dev,
+			dev_err(&bank->priv->client->dev,
 				"Cannot write brightness\n");
 			goto brightness_out;
 		}

-		if (!led->enabled) {
-			ret = regmap_update_bits(led->priv->regmap,
+		if (!bank->enabled) {
+			ret = regmap_update_bits(bank->priv->regmap,
 						 LM3697_CTRL_ENABLE,
 						 ctrl_en_val, ctrl_en_val);
 			if (ret) {
-				dev_err(&led->priv->client->dev,
+				dev_err(&bank->priv->client->dev,
 					"Cannot enable the device\n");
 				goto brightness_out;
 			}

-			led->enabled = brt_val;
+			bank->enabled = brt_val;
 		}
 	}

 brightness_out:
-	mutex_unlock(&led->priv->lock);
+	mutex_unlock(&bank->priv->lock);
 	return ret;
 }

 static int lm3697_init(struct lm3697 *priv)
 {
-	struct lm3697_led *led;
+	struct lm3697_bank *bank;
 	int i, ret;

 	if (priv->enable_gpio) {
@@ -180,9 +181,9 @@ 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++) {
-		led = &priv->leds[i];
-		ret = ti_lmu_common_set_ramp(&led->lmu_data);
+	for (i = 0; i < priv->num_banks; i++) {
+		bank = &priv->banks[i];
+		ret = ti_lmu_common_set_ramp(&bank->lmu_data);
 		if (ret)
 			dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
 	}
@@ -193,7 +194,7 @@ static int lm3697_init(struct lm3697 *priv)
 static int lm3697_probe_dt(struct lm3697 *priv)
 {
 	struct fwnode_handle *child = NULL;
-	struct lm3697_led *led;
+	struct lm3697_bank *bank;
 	const char *name;
 	int control_bank;
 	size_t i = 0;
@@ -228,63 +229,63 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 			goto child_out;
 		}

-		led = &priv->leds[i];
+		bank = &priv->banks[i];

 		ret = ti_lmu_common_get_brt_res(&priv->client->dev,
-						child, &led->lmu_data);
+						child, &bank->lmu_data);
 		if (ret)
			dev_warn(&priv->client->dev, "brightness resolution property missing\n");

-		led->control_bank = control_bank;
-		led->lmu_data.regmap = priv->regmap;
-		led->lmu_data.runtime_ramp_reg = LM3697_CTRL_A_RAMP +
+		bank->control_bank = control_bank;
+		bank->lmu_data.regmap = priv->regmap;
+		bank->lmu_data.runtime_ramp_reg = LM3697_CTRL_A_RAMP +
 						 control_bank;
-		led->lmu_data.msb_brightness_reg = LM3697_CTRL_A_BRT_MSB +
-						   led->control_bank * 2;
-		led->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB +
-						   led->control_bank * 2;
+		bank->lmu_data.msb_brightness_reg = LM3697_CTRL_A_BRT_MSB +
+						   bank->control_bank * 2;
+		bank->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB +
+						   bank->control_bank * 2;

-		led->num_leds = fwnode_property_count_u32(child, "led-sources");
-		if (led->num_leds > LM3697_MAX_LED_STRINGS) {
+		bank->num_leds = fwnode_property_count_u32(child, "led-sources");
+		if (bank->num_leds > 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);
+						    bank->hvled_strings,
+						    bank->num_leds);
 		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 < bank->num_leds; j++)
 			priv->bank_cfg |=
-				(led->control_bank << led->hvled_strings[j]);
+				(bank->control_bank << bank->hvled_strings[j]);

 		ret = ti_lmu_common_get_ramp_params(&priv->client->dev,
-						    child, &led->lmu_data);
+						    child, &bank->lmu_data);
 		if (ret)
 			dev_warn(&priv->client->dev, "runtime-ramp properties missing\n");

 		fwnode_property_read_string(child, "linux,default-trigger",
-					    &led->led_dev.default_trigger);
+					    &bank->led_dev.default_trigger);

 		ret = fwnode_property_read_string(child, "label", &name);
 		if (ret)
-			snprintf(led->label, sizeof(led->label),
+			snprintf(bank->label, sizeof(bank->label),
 				"%s::", priv->client->name);
 		else
-			snprintf(led->label, sizeof(led->label),
+			snprintf(bank->label, sizeof(bank->label),
 				 "%s:%s", priv->client->name, name);

-		led->priv = priv;
-		led->led_dev.name = led->label;
-		led->led_dev.max_brightness = led->lmu_data.max_brightness;
-		led->led_dev.brightness_set_blocking = lm3697_brightness_set;
+		bank->priv = priv;
+		bank->led_dev.name = bank->label;
+		bank->led_dev.max_brightness = bank->lmu_data.max_brightness;
+		bank->led_dev.brightness_set_blocking = lm3697_brightness_set;

-		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
+		ret = devm_led_classdev_register(priv->dev, &bank->led_dev);
 		if (ret) {
 			dev_err(&priv->client->dev, "led register err: %d\n",
 				ret);
@@ -307,16 +308,18 @@ static int lm3697_probe(struct i2c_client *client,
 	int ret;

 	count = device_get_child_node_count(&client->dev);
-	if (!count) {
-		dev_err(&client->dev, "LEDs are not defined in device tree!");
-		return -ENODEV;
+	if (!count || count > LM3697_MAX_CONTROL_BANKS) {
+		return -EINVAL;
 	}

-	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
+	led = devm_kzalloc(&client->dev, struct_size(led, banks, count),
 			   GFP_KERNEL);
 	if (!led)
 		return -ENOMEM;

+	led->num_banks = count;
+
 	mutex_init(&led->lock);
 	i2c_set_clientdata(client, led);

--
2.28.0

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

[Dan Murphy]

Marek

On 10/5/20 8:57 AM, ultracoolguy@tutanota.com wrote:
> I agree with you.
>
> Attached patch with changes.

Nack to the patch.

The subject says it does one thing but you also unnecessarily changed 
the name of the structure.

Renaming the structure does not fix the underlying issue

Dan

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

[Dan Murphy]

All

On 10/5/20 9:33 AM, Dan Murphy wrote:
> Marek
>
Sorry not Marek but Gabriel I misread the "To" field

Dan

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

[ultracoolguy]

I understand. So I should leave it like it was and do the rename in another patch? 

Oct 5, 2020, 14:33 by dmurphy@ti.com:

> Marek
>
> On 10/5/20 8:57 AM, ultracoolguy@tutanota.com wrote:
>
>> I agree with you.
>>
>> Attached patch with changes.
>>
>
> Nack to the patch.
>
> The subject says it does one thing but you also unnecessarily changed the name of the structure.
>
> Renaming the structure does not fix the underlying issue
>
> Dan
>

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

[Dan Murphy]

Gabriel

On 10/5/20 9:38 AM, ultracoolguy@tutanota.com wrote:
> I understand. So I should leave it like it was and do the rename in another patch?

You should do the fix in one patch and leave the structure name alone.

The structure naming if fine and has no benefit and actually will make 
it more difficult for others to backport future fixes.

Unless Pavel finds benefit in accepting the structure rename.

Dan

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

[ultracoolguy]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

Well, the major benefit I see is that it makes the driver slightly more readable. However I'm fine with whatever you guys decide.

I'll attach the patch with the struct renaming removed just in case.



Oct 5, 2020, 14:41 by dmurphy@ti.com:

> Gabriel
>
> On 10/5/20 9:38 AM, ultracoolguy@tutanota.com wrote:
>
>> I understand. So I should leave it like it was and do the rename in another patch?
>>
>
> You should do the fix in one patch and leave the structure name alone.
>
> The structure naming if fine and has no benefit and actually will make it more difficult for others to backport future fixes.
>
> Unless Pavel finds benefit in accepting the structure rename.
>
> Dan
>


[-- Attachment #2: 0001-leds-lm3697-Fix-out-of-bound-access.patch --]
[-- Type: text/x-patch, Size: 2179 bytes --]

From ee004d26bb2f91491141aa06f5518cc411711ff0 Mon Sep 17 00:00:00 2001
From: Ultracoolguy <ultracoolguy@tutanota.com>
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.
---
 drivers/leds/leds-lm3697.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index 024983088d59..bd53450050b2 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -78,8 +78,9 @@ struct lm3697 {
 	struct mutex lock;
 
 	int bank_cfg;
+	int num_banks;
 
-	struct lm3697_led leds[];
+	struct lm3697_led banks[];
 };
 
 static const struct reg_default lm3697_reg_defs[] = {
@@ -180,8 +181,8 @@ 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++) {
-		led = &priv->leds[i];
+	for (i = 0; i < priv->num_banks; i++) {
+		led = &priv->banks[i];
 		ret = ti_lmu_common_set_ramp(&led->lmu_data);
 		if (ret)
 			dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
@@ -228,7 +229,7 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 			goto child_out;
 		}
 
-		led = &priv->leds[i];
+		led = &priv->banks[i];
 
 		ret = ti_lmu_common_get_brt_res(&priv->client->dev,
 						child, &led->lmu_data);
@@ -307,16 +308,17 @@ static int lm3697_probe(struct i2c_client *client,
 	int ret;
 
 	count = device_get_child_node_count(&client->dev);
-	if (!count) {
-		dev_err(&client->dev, "LEDs are not defined in device tree!");
-		return -ENODEV;
+	if (!count || count > LM3697_MAX_CONTROL_BANKS) {
+		return -EINVAL;
 	}
 
-	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
+	led = devm_kzalloc(&client->dev, struct_size(led, banks, count),
 			   GFP_KERNEL);
 	if (!led)
 		return -ENOMEM;
 
+	led->num_banks = count;
+
 	mutex_init(&led->lock);
 	i2c_set_clientdata(client, led);
 
-- 
2.28.0

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

Hi!

> On 10/5/20 8:57 AM, ultracoolguy@tutanota.com wrote:
> > I agree with you.
> > 
> > Attached patch with changes.
> 
> Nack to the patch.

No need to do that, without matching From: and Signed-off, I can't
really apply it...

> The subject says it does one thing but you also unnecessarily changed the
> name of the structure.
> 
> Renaming the structure does not fix the underlying issue

While I can't really apply it, it is fairly good bugreport. Can I get
minimal patch to fix the problem, that can be cc-ed to stable, and
that I can just apply with git am?

And yes, I believe renaming the structures to be non-confusing is a
very good thing, but lets make it separate patch, so that stable
backporting is easier.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2552 bytes --]

Hi!

> Well, the major benefit I see is that it makes the driver slightly more readable. However I'm fine with whatever you guys decide.
> 
> I'll attach the patch with the struct renaming removed just in case.

Thanks for the patches. Content is pretty good, but I'd really need
From + Signed-off-by: with your real name to be able to apply it. (I'd
avoid renaming leds->banks variable in this patch, too, so we have
minimum stable patch).

Dan is maintaining this code, I suspect he'll come up with minimum
fix + followup cleanups shortly.

Best regards,
								Pavel


> diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
> index 024983088d59..bd53450050b2 100644
> --- a/drivers/leds/leds-lm3697.c
> +++ b/drivers/leds/leds-lm3697.c
> @@ -78,8 +78,9 @@ struct lm3697 {
>  	struct mutex lock;
>  
>  	int bank_cfg;
> +	int num_banks;
>  
> -	struct lm3697_led leds[];
> +	struct lm3697_led banks[];
>  };
>  
>  static const struct reg_default lm3697_reg_defs[] = {
> @@ -180,8 +181,8 @@ 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++) {
> -		led = &priv->leds[i];
> +	for (i = 0; i < priv->num_banks; i++) {
> +		led = &priv->banks[i];
>  		ret = ti_lmu_common_set_ramp(&led->lmu_data);
>  		if (ret)
>  			dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
> @@ -228,7 +229,7 @@ static int lm3697_probe_dt(struct lm3697 *priv)
>  			goto child_out;
>  		}
>  
> -		led = &priv->leds[i];
> +		led = &priv->banks[i];
>  
>  		ret = ti_lmu_common_get_brt_res(&priv->client->dev,
>  						child, &led->lmu_data);
> @@ -307,16 +308,17 @@ static int lm3697_probe(struct i2c_client *client,
>  	int ret;
>  
>  	count = device_get_child_node_count(&client->dev);
> -	if (!count) {
> -		dev_err(&client->dev, "LEDs are not defined in device tree!");
> -		return -ENODEV;
> +	if (!count || count > LM3697_MAX_CONTROL_BANKS) {
> +		return -EINVAL;
>  	}
>  
> -	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
> +	led = devm_kzalloc(&client->dev, struct_size(led, banks, count),
>  			   GFP_KERNEL);
>  	if (!led)
>  		return -ENOMEM;
>  
> +	led->num_banks = count;
> +
>  	mutex_init(&led->lock);
>  	i2c_set_clientdata(client, led);
>  
> -- 
> 2.28.0
> 


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Alexander Dahl]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

Hei hei,

On Mon, Oct 05, 2020 at 05:35:38PM +0200, ultracoolguy@tutanota.com wrote:
> Well, the major benefit I see is that it makes the driver slightly
> more readable. However I'm fine with whatever you guys decide.
> 
> I'll attach the patch with the struct renaming removed just in case.

Note: your patch, especially the commit message, still needs a
Signed-off-by line.  Please read [1] (again?) and resend.

Greets
Alex

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> Oct 5, 2020, 14:41 by dmurphy@ti.com:
> 
> > Gabriel
> >
> > On 10/5/20 9:38 AM, ultracoolguy@tutanota.com wrote:
> >
> >> I understand. So I should leave it like it was and do the rename in another patch?
> >>
> >
> > You should do the fix in one patch and leave the structure name alone.
> >
> > The structure naming if fine and has no benefit and actually will make it more difficult for others to backport future fixes.
> >
> > Unless Pavel finds benefit in accepting the structure rename.
> >
> > Dan
> >
> 

> >From ee004d26bb2f91491141aa06f5518cc411711ff0 Mon Sep 17 00:00:00 2001
> From: Ultracoolguy <ultracoolguy@tutanota.com>
> 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.
> ---
>  drivers/leds/leds-lm3697.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
> index 024983088d59..bd53450050b2 100644
> --- a/drivers/leds/leds-lm3697.c
> +++ b/drivers/leds/leds-lm3697.c
> @@ -78,8 +78,9 @@ struct lm3697 {
>  	struct mutex lock;
>  
>  	int bank_cfg;
> +	int num_banks;
>  
> -	struct lm3697_led leds[];
> +	struct lm3697_led banks[];
>  };
>  
>  static const struct reg_default lm3697_reg_defs[] = {
> @@ -180,8 +181,8 @@ 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++) {
> -		led = &priv->leds[i];
> +	for (i = 0; i < priv->num_banks; i++) {
> +		led = &priv->banks[i];
>  		ret = ti_lmu_common_set_ramp(&led->lmu_data);
>  		if (ret)
>  			dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
> @@ -228,7 +229,7 @@ static int lm3697_probe_dt(struct lm3697 *priv)
>  			goto child_out;
>  		}
>  
> -		led = &priv->leds[i];
> +		led = &priv->banks[i];
>  
>  		ret = ti_lmu_common_get_brt_res(&priv->client->dev,
>  						child, &led->lmu_data);
> @@ -307,16 +308,17 @@ static int lm3697_probe(struct i2c_client *client,
>  	int ret;
>  
>  	count = device_get_child_node_count(&client->dev);
> -	if (!count) {
> -		dev_err(&client->dev, "LEDs are not defined in device tree!");
> -		return -ENODEV;
> +	if (!count || count > LM3697_MAX_CONTROL_BANKS) {
> +		return -EINVAL;
>  	}
>  
> -	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
> +	led = devm_kzalloc(&client->dev, struct_size(led, banks, count),
>  			   GFP_KERNEL);
>  	if (!led)
>  		return -ENOMEM;
>  
> +	led->num_banks = count;
> +
>  	mutex_init(&led->lock);
>  	i2c_set_clientdata(client, led);
>  
> -- 
> 2.28.0
> 


-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[ultracoolguy]

[-- Attachment #1: Type: text/plain, Size: 4111 bytes --]

Agh. I added the Signed-off-by in an earlier non-published version of the commit, but forgot to add it back. But that doesn't really excuses me.

I attached the (hopefully) final version of this patch.  Pavel, I'll send the struct rename separately after I submit this. 

Oct 5, 2020, 16:48 by post@lespocky.de:

> Hei hei,
>
> On Mon, Oct 05, 2020 at 05:35:38PM +0200, ultracoolguy@tutanota.com wrote:
>
>> Well, the major benefit I see is that it makes the driver slightly
>> more readable. However I'm fine with whatever you guys decide.
>>
>> I'll attach the patch with the struct renaming removed just in case.
>>
>
> Note: your patch, especially the commit message, still needs a
> Signed-off-by line.  Please read [1] (again?) and resend.
>
> Greets
> Alex
>
> [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
>> Oct 5, 2020, 14:41 by dmurphy@ti.com:
>>
>> > Gabriel
>> >
>> > On 10/5/20 9:38 AM, ultracoolguy@tutanota.com wrote:
>> >
>> >> I understand. So I should leave it like it was and do the rename in another patch?
>> >>
>> >
>> > You should do the fix in one patch and leave the structure name alone.
>> >
>> > The structure naming if fine and has no benefit and actually will make it more difficult for others to backport future fixes.
>> >
>> > Unless Pavel finds benefit in accepting the structure rename.
>> >
>> > Dan
>> >
>>
>> >From ee004d26bb2f91491141aa06f5518cc411711ff0 Mon Sep 17 00:00:00 2001
>> From: Ultracoolguy <ultracoolguy@tutanota.com>
>> 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.
>> ---
>> drivers/leds/leds-lm3697.c | 18 ++++++++++--------
>> 1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
>> index 024983088d59..bd53450050b2 100644
>> --- a/drivers/leds/leds-lm3697.c
>> +++ b/drivers/leds/leds-lm3697.c
>> @@ -78,8 +78,9 @@ struct lm3697 {
>> struct mutex lock;
>>
>> int bank_cfg;
>> +	int num_banks;
>>
>> -	struct lm3697_led leds[];
>> +	struct lm3697_led banks[];
>> };
>>
>> static const struct reg_default lm3697_reg_defs[] = {
>> @@ -180,8 +181,8 @@ 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++) {
>> -		led = &priv->leds[i];
>> +	for (i = 0; i < priv->num_banks; i++) {
>> +		led = &priv->banks[i];
>> ret = ti_lmu_common_set_ramp(&led->lmu_data);
>> if (ret)
>> dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
>> @@ -228,7 +229,7 @@ static int lm3697_probe_dt(struct lm3697 *priv)
>> goto child_out;
>> }
>>
>> -		led = &priv->leds[i];
>> +		led = &priv->banks[i];
>>
>> ret = ti_lmu_common_get_brt_res(&priv->client->dev,
>> child, &led->lmu_data);
>> @@ -307,16 +308,17 @@ static int lm3697_probe(struct i2c_client *client,
>> int ret;
>>
>> count = device_get_child_node_count(&client->dev);
>> -	if (!count) {
>> -		dev_err(&client->dev, "LEDs are not defined in device tree!");
>> -		return -ENODEV;
>> +	if (!count || count > LM3697_MAX_CONTROL_BANKS) {
>> +		return -EINVAL;
>> }
>>
>> -	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
>> +	led = devm_kzalloc(&client->dev, struct_size(led, banks, count),
>> GFP_KERNEL);
>> if (!led)
>> return -ENOMEM;
>>
>> +	led->num_banks = count;
>> +
>> mutex_init(&led->lock);
>> i2c_set_clientdata(client, led);
>>
>> -- 
>> 2.28.0
>>
>
>
> -- 
> /"\ ASCII RIBBON | »With the first link, the chain is forged. The first
> \ / CAMPAIGN     | speech censured, the first thought forbidden, the
> X  AGAINST      | first freedom denied, chains us all irrevocably.«
> / \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)
>


[-- Attachment #2: 0001-leds-lm3697-Fix-out-of-bound-access.patch --]
[-- Type: text/x-patch, Size: 2226 bytes --]

From 146c98f0a0227fc3e11ffe6e66f0f7cf8aaebc69 Mon Sep 17 00:00:00 2001
From: Gabriel David <ultracoolguy@tutanota.com>
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: Gabriel David <ultracoolguy@tutanota.com>
---
 drivers/leds/leds-lm3697.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index 024983088d59..a3c44b4c9072 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -78,8 +78,9 @@ struct lm3697 {
 	struct mutex lock;

 	int bank_cfg;
+	int num_banks;

-	struct lm3697_led leds[];
+	struct lm3697_led banks[];
 };

 static const struct reg_default lm3697_reg_defs[] = {
@@ -180,8 +181,8 @@ 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++) {
-		led = &priv->leds[i];
+	for (i = 0; i < priv->num_banks; i++) {
+		led = &priv->banks[i];
 		ret = ti_lmu_common_set_ramp(&led->lmu_data);
 		if (ret)
 			dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
@@ -228,7 +229,7 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 			goto child_out;
 		}

-		led = &priv->leds[i];
+		led = &priv->banks[i];

 		ret = ti_lmu_common_get_brt_res(&priv->client->dev,
 						child, &led->lmu_data);
@@ -307,16 +308,16 @@ static int lm3697_probe(struct i2c_client *client,
 	int ret;

 	count = device_get_child_node_count(&client->dev);
-	if (!count) {
-		dev_err(&client->dev, "LEDs are not defined in device tree!");
-		return -ENODEV;
-	}
+	if (!count || count > LM3697_MAX_CONTROL_BANKS)
+		return -EINVAL;

-	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
+	led = devm_kzalloc(&client->dev, struct_size(led, banks, count),
 			   GFP_KERNEL);
 	if (!led)
 		return -ENOMEM;

+	led->num_banks = count;
+
 	mutex_init(&led->lock);
 	i2c_set_clientdata(client, led);

--
2.28.0

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

Hi!

> Agh. I added the Signed-off-by in an earlier non-published version of the commit, but forgot to add it back. But that doesn't really excuses me.
> 
> I attached the (hopefully) final version of this patch.  Pavel, I'll send the struct rename separately after I submit this. 
>

Thanks, I applied it with ... some tweaks. I hope I did not break it,
and would not mind testing.

Best regards,
								Pavel
								

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[ultracoolguy]

This: 

led->num_banks = count; 

Has to be below devm_kzalloc. Else, it's NULL.
Oct 5, 2020, 17:32 by pavel@ucw.cz:

> Hi!
>
>> Agh. I added the Signed-off-by in an earlier non-published version of the commit, but forgot to add it back. But that doesn't really excuses me.
>>
>> I attached the (hopefully) final version of this patch.  Pavel, I'll send the struct rename separately after I submit this. 
>>
>
> Thanks, I applied it with ... some tweaks. I hope I did not break it,
> and would not mind testing.
>
> Best regards,
>  Pavel
>  
>
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

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

[ultracoolguy]

Otherwise everything works great :)

(And sorry for sending two emails)


Oct 5, 2020, 18:29 by ultracoolguy@tutanota.com:

> This: 
>
> led->num_banks = count; 
>
> Has to be below devm_kzalloc. Else, it's NULL.
> Oct 5, 2020, 17:32 by pavel@ucw.cz:
>
>> Hi!
>>
>>> Agh. I added the Signed-off-by in an earlier non-published version of the commit, but forgot to add it back. But that doesn't really excuses me.
>>>
>>> I attached the (hopefully) final version of this patch.  Pavel, I'll send the struct rename separately after I submit this. 
>>>
>>
>> Thanks, I applied it with ... some tweaks. I hope I did not break it,
>> and would not mind testing.
>>
>> Best regards,
>> Pavel
>>
>>
>> -- 
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>>
>
>

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On Mon 2020-10-05 20:31:16, ultracoolguy@tutanota.com wrote:
> Otherwise everything works great :)
> 
> (And sorry for sending two emails)

No problem, sorry for breaking your patch. I moved it near other
initializers.

Question: is it likely that someone will want to use your device with
-stable kernels? Should I mark this for -stable?

Thanks and best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[ultracoolguy]

>No problem, sorry for breaking your patch. I moved it near other
initializers.

No problem :)

>Question: is it likely that someone will want to use your device with
-stable kernels? Should I mark this for -stable?

To be honest, it's unlikely that someone other than me is interested in my device on -stable. However, if you feel like the patch is simple enough to not cause any problem, then feel free to do so. 

Oct 5, 2020, 18:39 by pavel@ucw.cz:

> On Mon 2020-10-05 20:31:16, ultracoolguy@tutanota.com wrote:
>
>> Otherwise everything works great :)
>>
>> (And sorry for sending two emails)
>>
>
> No problem, sorry for breaking your patch. I moved it near other
> initializers.
>
> Question: is it likely that someone will want to use your device with
> -stable kernels? Should I mark this for -stable?
>
> Thanks and best regards,
>
>  Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

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

[Marek Behun]

By the way I just realized that the DT binding in this driver seems
incorrect to me.

The controller logically supports 3 LED strings, each having
configurable control bank.

But the DT binding supports 2 DT nodes, one for each control bank
(identified by the `reg` property) and then `led-sources` says which
string should be controlled by given bank.

But taking in mind that DT should describe how devices are connected to
each other, I think the child nodes in the binding should instead
describe the 3 supported LED strings...

Marek

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

[ultracoolguy]

While I do agree with you that having the child nodes be led strings make more sense, would it be possible to have, for example, three strings controlled by the same label?

Oct 6, 2020, 07:33 by kabel@blackhole.sk:

> By the way I just realized that the DT binding in this driver seems
> incorrect to me.
>
> The controller logically supports 3 LED strings, each having
> configurable control bank.
>
> But the DT binding supports 2 DT nodes, one for each control bank
> (identified by the `reg` property) and then `led-sources` says which
> string should be controlled by given bank.
>
> But taking in mind that DT should describe how devices are connected to
> each other, I think the child nodes in the binding should instead
> describe the 3 supported LED strings...
>
> Marek
>

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

[Dan Murphy]

All

On 10/6/20 6:59 AM, ultracoolguy@tutanota.com wrote:
> While I do agree with you that having the child nodes be led strings make more sense, would it be possible to have, for example, three strings controlled by the same label?
>
> Oct 6, 2020, 07:33 by kabel@blackhole.sk:
>
>> By the way I just realized that the DT binding in this driver seems
>> incorrect to me.
>>
>> The controller logically supports 3 LED strings, each having
>> configurable control bank.

There are two control banks. You can connect the HVLED outputs to either 
control bank A or B there is no individual control of the LED strings.


>> But the DT binding supports 2 DT nodes, one for each control bank
>> (identified by the `reg` property) and then `led-sources` says which
>> string should be controlled by given bank.
>>
>> But taking in mind that DT should describe how devices are connected to
>> each other, I think the child nodes in the binding should instead
>> describe the 3 supported LED strings...

The outputs in this case are virtual outputs which are the banks (A and B).

Since the device is bank controlled the actual current sinks are not 
defined thus making the the banks the actual outputs.

This is why the 'reg' property defines the control bank either A or B 
and the led-sources indicates the strings associated with the control bank.

Dan

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

[Marek Behun]

Adding Rob to Cc, Rob, could we have your opinion on this? Mine is below.

On Tue, 6 Oct 2020 07:21:14 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> >> By the way I just realized that the DT binding in this driver seems
> >> incorrect to me.
> >>
> >> The controller logically supports 3 LED strings, each having
> >> configurable control bank.  
> 
> There are two control banks. You can connect the HVLED outputs to either 
> control bank A or B there is no individual control of the LED strings.
> 
> 
> >> But the DT binding supports 2 DT nodes, one for each control bank
> >> (identified by the `reg` property) and then `led-sources` says which
> >> string should be controlled by given bank.
> >>
> >> But taking in mind that DT should describe how devices are connected to
> >> each other, I think the child nodes in the binding should instead
> >> describe the 3 supported LED strings...  
> 
> The outputs in this case are virtual outputs which are the banks (A and B).
> 
> Since the device is bank controlled the actual current sinks are not 
> defined thus making the the banks the actual outputs.
> 
> This is why the 'reg' property defines the control bank either A or B 
> and the led-sources indicates the strings associated with the control bank.
> 
> Dan
> 

Dan, I looked at the datasheet, I understand this.

Nonetheless, device tree should describe how devices are connected to
each other. The chip has 3 pins for 3 LED strings.

If this controller should be able to support 3 LED strings via 3
outputs, the device tree binding nodes should, in my opinion, describe
each pin connected string. The nodes should maybe even be called
'led-string@N' where N is from [0, 1, 2].

The fact that the device is bank controlled and there are only two
banks (and it is configurable by which bank each LED string is
controlled) is more relevant to the driver, not as much to device tree
binding.

But yes, I do realize that if we had 3 child nodes, and the driver
created 3 LEDs, then changing brithrness on one of the 3 LEDs would
change brightness on at least another one, which we do not want.

Maybe this driver could parse these 3 `led-string` nodes, each having
defined bank via `led-sources`, and then register LED classdevs for
each bank that is mentioned. This way the device tree would be more
correct, IMO, and the driver would not have the problem mentioned in
the paragraph above.

Marek

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

[Dan Murphy]

Marek

On 10/6/20 9:41 AM, Marek Behun wrote:
> Adding Rob to Cc, Rob, could we have your opinion on this? Mine is below.
>
> Dan, I looked at the datasheet, I understand this.
>
> Nonetheless, device tree should describe how devices are connected to
> each other. The chip has 3 pins for 3 LED strings.
>
> If this controller should be able to support 3 LED strings via 3
> outputs, the device tree binding nodes should, in my opinion, describe
> each pin connected string. The nodes should maybe even be called
> 'led-string@N' where N is from [0, 1, 2].
>
> The fact that the device is bank controlled and there are only two
> banks (and it is configurable by which bank each LED string is
> controlled) is more relevant to the driver, not as much to device tree
> binding.
>
> But yes, I do realize that if we had 3 child nodes, and the driver
> created 3 LEDs, then changing brithrness on one of the 3 LEDs would
> change brightness on at least another one, which we do not want.
>
> Maybe this driver could parse these 3 `led-string` nodes, each having
> defined bank via `led-sources`, and then register LED classdevs for
> each bank that is mentioned. This way the device tree would be more
> correct, IMO, and the driver would not have the problem mentioned in
> the paragraph above.

Unfortunately we cannot and should not change the ABI now.

Using the led-sources as the bank indicator does not conform to the 
definition of the description of the led-sources in the yaml.

The preference was to use the led-sources to define the LED out to the bank.

Here is the conversation on how the driver got to where it is.

https://lore.kernel.org/patchwork/patch/972337/

Dan

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

[Marek Behun]

On Tue, 6 Oct 2020 09:57:08 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> Unfortunately we cannot and should not change the ABI now.
> 
> Using the led-sources as the bank indicator does not conform to the 
> definition of the description of the led-sources in the yaml.
> 
> The preference was to use the led-sources to define the LED out to the bank.
> 
> Here is the conversation on how the driver got to where it is.
> 
> https://lore.kernel.org/patchwork/patch/972337/
> 
> Dan
> 

Oh, if this was discussed already, then never mind. Sorry for taking
your time.

Marek

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

Hi!

> > >> By the way I just realized that the DT binding in this driver seems
> > >> incorrect to me.
> > >>
> > >> The controller logically supports 3 LED strings, each having
> > >> configurable control bank.  
> > 
> > There are two control banks. You can connect the HVLED outputs to either 
> > control bank A or B there is no individual control of the LED strings.
> > 
> > 
> > >> But the DT binding supports 2 DT nodes, one for each control bank
> > >> (identified by the `reg` property) and then `led-sources` says which
> > >> string should be controlled by given bank.
> > >>
> > >> But taking in mind that DT should describe how devices are connected to
> > >> each other, I think the child nodes in the binding should instead
> > >> describe the 3 supported LED strings...  
> > 
> > The outputs in this case are virtual outputs which are the banks (A and B).
> > 
> > Since the device is bank controlled the actual current sinks are not 
> > defined thus making the the banks the actual outputs.
> > 
> > This is why the 'reg' property defines the control bank either A or B 
> > and the led-sources indicates the strings associated with the control bank.

> Dan, I looked at the datasheet, I understand this.
> 
> Nonetheless, device tree should describe how devices are connected to
> each other. The chip has 3 pins for 3 LED strings.

Well, device tree is not a device schematics...

> If this controller should be able to support 3 LED strings via 3
> outputs, the device tree binding nodes should, in my opinion, describe
> each pin connected string. The nodes should maybe even be called
> 'led-string@N' where N is from [0, 1, 2].
> 
> The fact that the device is bank controlled and there are only two
> banks (and it is configurable by which bank each LED string is
> controlled) is more relevant to the driver, not as much to device tree
> binding.

Seems to me like two independend LEDs, and I'd describe it as
such. The fact that it goes over 3 wires is just a implementation
detail. Lets keep it simple...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]