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

Alexander Dahl
2020-10-05T16:48:29Z

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