Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: a135dfb5de1 ("ALSA: led control - add sysfs kcontrol LED marking layer")
> Signed-off-by: Dongliang Mu <mudongliangabcd@xxxxxxxxx>
> ---
>  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;
>  		if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
>  			goto cerr;
>  		led_card->dev.parent = &led->dev;
> @@ -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);
>  		kfree(led_card);
>  		led->cards[card->number] = NULL;
>  	}

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);
}