On Fri, May 28, 2021 at 09:17:57PM +0800, Dongliang Mu wrote:
The snd_ctl_led_sysfs_add and snd_ctl_led_sysfs_remove should contain the refcount operations in pair. However, snd_ctl_led_sysfs_remove fails to decrease the refcount to zero, which causes device_release never to be invoked. This leads to memory leak to some resources, like struct device_private.
Fix this by calling put_device at the end of snd_ctl_led_sysfs_remove
Reported-by: syzbot+08a7d8b51ea048a74ffb@syzkaller.appspotmail.com Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer") Signed-off-by: Dongliang Mu mudongliangabcd@gmail.com
sound/core/control_led.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index 25f57c14f294..fff2688b5019 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -371,6 +371,10 @@ static void snd_ctl_led_disconnect(struct snd_card *card) snd_ctl_led_refresh(); }
+static void snd_ctl_led_release(struct device *dev) +{ +}
Just to clarify again some more, this call back has to free "led_card". This patch changes the memory leak into a use after free bug. (A use after free bug is worse than a memory leak).
There were some other leaks as discussed where a dummy free function is fine because they were dealing with static data structures (not allocated memory).
/*
- sysfs
*/ @@ -663,6 +667,7 @@ static void snd_ctl_led_sysfs_add(struct snd_card *card) led_card->number = card->number; led_card->led = led; device_initialize(&led_card->dev);
led_card->dev.release = snd_ctl_led_release;@@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card) sysfs_remove_link(&card->ctl_dev.kobj, link_name); sysfs_remove_link(&led_card->dev.kobj, "card"); device_del(&led_card->dev);
put_device(&led_card->dev);
Btw, I have created a Smatch warning for this type of code where we have:
put_device(&foo->dev); kfree(foo);
sound/core/control_led.c:709 snd_ctl_led_sysfs_remove() warn: freeing device managed memory: 'led_card'
So hopefully that will prevent future similar bugs. I'll test it out overnight and report back tomorrow how it works.
regards, dan carpenter
/* * Copyright (C) 2021 Oracle. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License * as published by the Free Software Foundation; either version 2 * of the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt */
#include "smatch.h"
static int my_id;
STATE(managed);
static void set_ignore(struct sm_state *sm, struct expression *mod_expr) { set_state(my_id, sm->name, sm->sym, &undefined); }
static void match_put_device(const char *fn, struct expression *expr, void *param) { struct expression *arg;
arg = get_argument_from_call_expr(expr->args, PTR_INT(param)); arg = strip_expr(arg); if (!arg || arg->type != EXPR_PREOP || arg->op != '&') return; arg = strip_expr(arg->unop); if (!arg || arg->type != EXPR_DEREF) return; arg = strip_expr(arg->deref); if (arg && arg->type == EXPR_PREOP && arg->op == '*') arg = arg->unop; set_state_expr(my_id, arg, &managed); }
static void match_free(const char *fn, struct expression *expr, void *param) { struct expression *arg; char *name;
arg = get_argument_from_call_expr(expr->args, PTR_INT(param)); if (get_state_expr(my_id, arg) != &managed) return; name = expr_to_str(arg); sm_warning("freeing device managed memory: '%s'", name); free_string(name); }
void check_put_device(int id) { my_id = id;
if (option_project != PROJ_KERNEL) return;
add_function_hook("put_device", &match_put_device, INT_PTR(0)); add_function_hook("device_unregister", &match_put_device, INT_PTR(0));
add_function_hook("kfree", &match_free, INT_PTR(0)); add_modification_hook(my_id, &set_ignore); }