[PATCH 1/2] ALSA: control - fix a leak in snd_ctl_led_init()
This unwind loop needs to free snd_ctl_leds[0] as well.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- sound/core/control_led.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index d4fb8b873f34..202b475d0bf3 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -712,7 +712,7 @@ static struct snd_ctl_layer_ops snd_ctl_led_lops = { static int __init snd_ctl_led_init(void) { struct snd_ctl_led *led; - unsigned int group; + int group;
device_initialize(&snd_ctl_led_dev); snd_ctl_led_dev.class = sound_class; @@ -730,7 +730,7 @@ static int __init snd_ctl_led_init(void) dev_set_name(&led->dev, led->name); if (device_add(&led->dev)) { put_device(&led->dev); - for (; group > 0; group--) { + for (; group >= 0; group--) { led = &snd_ctl_leds[group]; device_del(&led->dev); }
If count is 16 then this will put the NUL terminator one element beyond the end of the array.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- sound/core/control_led.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index 202b475d0bf3..ab5a455723c8 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -391,7 +391,7 @@ static ssize_t store_mode(struct device *dev, struct device_attribute *attr, { struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev); char _buf[16]; - size_t l = min(count, sizeof(_buf) - 1) + 1; + size_t l = min(count, sizeof(_buf) - 1); enum snd_ctl_led_mode mode;
memcpy(_buf, buf, l);
Dne 02. 04. 21 v 13:42 Dan Carpenter napsal(a):
If count is 16 then this will put the NUL terminator one element beyond the end of the array.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
No idea why I added + 1... Thanks for your correction.
Reviewed-by: Jaroslav Kysela perex@perex.cz
sound/core/control_led.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index 202b475d0bf3..ab5a455723c8 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -391,7 +391,7 @@ static ssize_t store_mode(struct device *dev, struct device_attribute *attr, { struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev); char _buf[16];
- size_t l = min(count, sizeof(_buf) - 1) + 1;
size_t l = min(count, sizeof(_buf) - 1); enum snd_ctl_led_mode mode;
memcpy(_buf, buf, l);
On Fri, 02 Apr 2021 19:52:43 +0200, Jaroslav Kysela wrote:
Dne 02. 04. 21 v 13:42 Dan Carpenter napsal(a):
If count is 16 then this will put the NUL terminator one element beyond the end of the array.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
No idea why I added + 1... Thanks for your correction.
Reviewed-by: Jaroslav Kysela perex@perex.cz
Applied now. Thanks.
Takashi
Dne 02. 04. 21 v 13:42 Dan Carpenter napsal(a):
This unwind loop needs to free snd_ctl_leds[0] as well.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
sound/core/control_led.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index d4fb8b873f34..202b475d0bf3 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -712,7 +712,7 @@ static struct snd_ctl_layer_ops snd_ctl_led_lops = { static int __init snd_ctl_led_init(void) { struct snd_ctl_led *led;
- unsigned int group;
int group;
device_initialize(&snd_ctl_led_dev); snd_ctl_led_dev.class = sound_class;
@@ -730,7 +730,7 @@ static int __init snd_ctl_led_init(void) dev_set_name(&led->dev, led->name); if (device_add(&led->dev)) { put_device(&led->dev);
for (; group > 0; group--) {
for (; group >= 0; group--) { led = &snd_ctl_leds[group];
It's not correct. This assignent should be 'led = &snd_ctl_leds[group - 1];' without other changes, because the put_device() is enough when device_add() fails.
Could you resend the correction?
Jaroslav
device_del(&led->dev); }
"group - 1" was intended here instead of "group". The current error handling will double free the first item in the array and leak the last item.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com --- v2: The first patch wasn't right. It fixed the leak but left the double free.
sound/core/control_led.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index d756a52e58db..93b201063c7d 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -734,7 +734,7 @@ static int __init snd_ctl_led_init(void) if (device_add(&led->dev)) { put_device(&led->dev); for (; group > 0; group--) { - led = &snd_ctl_leds[group]; + led = &snd_ctl_leds[group - 1]; device_del(&led->dev); } device_del(&snd_ctl_led_dev);
Dne 09. 04. 21 v 14:34 Dan Carpenter napsal(a):
"group - 1" was intended here instead of "group". The current error handling will double free the first item in the array and leak the last item.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
Reviewed-by: Jaroslav Kysela perex@perex.cz
v2: The first patch wasn't right. It fixed the leak but left the double free.
sound/core/control_led.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index d756a52e58db..93b201063c7d 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -734,7 +734,7 @@ static int __init snd_ctl_led_init(void) if (device_add(&led->dev)) { put_device(&led->dev); for (; group > 0; group--) {
led = &snd_ctl_leds[group];
led = &snd_ctl_leds[group - 1]; device_del(&led->dev); } device_del(&snd_ctl_led_dev);
On Fri, 09 Apr 2021 14:34:41 +0200, Dan Carpenter wrote:
"group - 1" was intended here instead of "group". The current error handling will double free the first item in the array and leak the last item.
Fixes: cb17fe0045aa ("ALSA: control - add sysfs support to the LED trigger module") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com
v2: The first patch wasn't right. It fixed the leak but left the double free.
Applied now. Thanks.
Takashi
participants (3)
-
Dan Carpenter -
Jaroslav Kysela -
Takashi Iwai