Free memory region, if gb_lights_channel_config is not successful.
Signed-off-by: Arvind Yadav arvind.yadav.cs@gmail.com --- drivers/staging/greybus/light.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index 3f4148c..b00d47c 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -984,7 +984,7 @@ static int gb_lights_channel_config(struct gb_light *light,
ret = channel_attr_groups_set(channel, cdev); if (ret < 0) - return ret; + goto err;
gb_lights_led_operations_set(channel, cdev);
@@ -994,15 +994,18 @@ static int gb_lights_channel_config(struct gb_light *light, * configurations. */ if (!is_channel_flash(channel)) - return ret; + goto err;
light->has_flash = true;
ret = gb_lights_channel_flash_config(channel); if (ret < 0) - return ret; + goto err;
return ret; +err: + kfree(cdev->name); + return ret; }
static int gb_lights_light_config(struct gb_lights *glights, u8 id)
On Thu, Sep 21, 2017 at 05:05:27PM +0530, Arvind Yadav wrote:
Free memory region, if gb_lights_channel_config is not successful.
The question I have is do we free this on module unload? I don't see that we do. I feel like we should do a free after calling __gb_lights_led_unregister(). But that's awkward because we call __gb_lights_led_unregister() when this function fails so it would end up being a double free.
Signed-off-by: Arvind Yadav arvind.yadav.cs@gmail.com
drivers/staging/greybus/light.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index 3f4148c..b00d47c 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -984,7 +984,7 @@ static int gb_lights_channel_config(struct gb_light *light, ret = channel_attr_groups_set(channel, cdev); if (ret < 0)
return ret;
goto err;gb_lights_led_operations_set(channel, cdev); @@ -994,15 +994,18 @@ static int gb_lights_channel_config(struct gb_light *light, * configurations. */ if (!is_channel_flash(channel))
return ret;
goto err;
"ret" is zero here. This is actually a success return. It would be cleaner to just write "return 0;". Anyway, this patch introduces a use after free so that doesn't work.
Also it's better to choose a label name which says what the label does so in this case it would be "goto err_free_name" or "goto err_cdev_name" or whatever, but something to indicate that it's to do with freeing the cdev->name. Just "err" is too ambiguous.
light->has_flash = true; ret = gb_lights_channel_flash_config(channel); if (ret < 0)
return ret;
goto err;return ret;
^^^^^^^^^^ Here as well, change this from "return ret;" to "return 0;".
regards, dan carpenter
Hi, On Thu, Sep 21, 2017 at 03:59:18PM +0300, Dan Carpenter wrote:
On Thu, Sep 21, 2017 at 05:05:27PM +0530, Arvind Yadav wrote:
Free memory region, if gb_lights_channel_config is not successful.
Arvind, thanks for patch and good catch. But please look at the subject of other patches applied to this file and try to stick with the labels, staging: greybus: light:
The question I have is do we free this on module unload? I don't see that we do. I feel like we should do a free after calling __gb_lights_led_unregister(). But that's awkward because we call __gb_lights_led_unregister() when this function fails so it would end up being a double free.
Yes Dan, You are correct, this should be free in __gb_lights_led_unregister(), and not here.
Signed-off-by: Arvind Yadav arvind.yadav.cs@gmail.com
drivers/staging/greybus/light.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index 3f4148c..b00d47c 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -984,7 +984,7 @@ static int gb_lights_channel_config(struct gb_light *light, ret = channel_attr_groups_set(channel, cdev); if (ret < 0)
return ret;
goto err;gb_lights_led_operations_set(channel, cdev); @@ -994,15 +994,18 @@ static int gb_lights_channel_config(struct gb_light *light, * configurations. */ if (!is_channel_flash(channel))
return ret;
goto err;"ret" is zero here. This is actually a success return. It would be cleaner to just write "return 0;". Anyway, this patch introduces a use after free so that doesn't work.
Also it's better to choose a label name which says what the label does so in this case it would be "goto err_free_name" or "goto err_cdev_name" or whatever, but something to indicate that it's to do with freeing the cdev->name. Just "err" is too ambiguous.
light->has_flash = true; ret = gb_lights_channel_flash_config(channel); if (ret < 0)
return ret;
goto err;return ret;
^^^^^^^^^^Here as well, change this from "return ret;" to "return 0;".
It should be return 0; from the start, you are right, but that would be a complete different change than the actual fix that now goes far away from this code.
Thank both, --- Cheers, Rui
-
Arvind Yadav -
Dan Carpenter -
Rui Miguel Silva