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

[ultracoolguy@tutanota.com]

[-- 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