From: ultracoolguy@tutanota.com
To: Marek Behun <kabel@blackhole.sk>
Cc: Pavel <pavel@ucw.cz>, Dmurphy <dmurphy@ti.com>,
Linux Leds <linux-leds@vger.kernel.org>,
Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] leds: lm3697: Fix out-of-bound access
Date: Mon, 5 Oct 2020 15:57:30 +0200 (CEST) [thread overview]
Message-ID: <MIt2NiS--3-2@tutanota.com> (raw)
In-Reply-To: <20201005141334.36d9441a@blackhole.sk>
[-- 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
next prev parent reply other threads:[~2020-10-05 13:57 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-03 13:02 [PATCH] leds: lm3697: Fix out-of-bound access ultracoolguy
2020-10-03 13:56 ` Pavel Machek
2020-10-03 14:43 ` ultracoolguy
2020-10-05 12:13 ` Marek Behun
2020-10-05 13:50 ` Pavel Machek
2020-10-05 13:57 ` ultracoolguy [this message]
2020-10-05 14:33 ` Dan Murphy
2020-10-05 14:37 ` Dan Murphy
2020-10-05 14:38 ` ultracoolguy
2020-10-05 14:41 ` Dan Murphy
2020-10-05 15:35 ` ultracoolguy
2020-10-05 16:05 ` Pavel Machek
2020-10-05 16:48 ` Alexander Dahl
2020-10-05 17:14 ` ultracoolguy
2020-10-05 17:32 ` Pavel Machek
2020-10-05 18:29 ` ultracoolguy
2020-10-05 18:31 ` ultracoolguy
2020-10-05 18:39 ` Pavel Machek
2020-10-05 18:48 ` ultracoolguy
2020-10-06 7:33 ` Marek Behun
2020-10-06 11:59 ` ultracoolguy
2020-10-06 12:21 ` Dan Murphy
2020-10-06 14:41 ` Marek Behun
2020-10-06 14:57 ` Dan Murphy
2020-10-06 15:14 ` Marek Behun
2020-10-06 17:26 ` Pavel Machek
2020-10-05 15:59 ` Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MIt2NiS--3-2@tutanota.com \
--to=ultracoolguy@tutanota.com \
--cc=dmurphy@ti.com \
--cc=kabel@blackhole.sk \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.