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++) {
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
