linux-kernel - Re: [syzbot] KASAN: use-after-free Read in em28xx_close

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Jul 2021 17:14:55 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     syzbot <syzbot+005037419ebdf14e1d87@...kaller.appspotmail.com>,
        igormtorrente@...il.com, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] KASAN: use-after-free Read in em28xx_close_extension

On Tue, Jul 27, 2021 at 06:01:51PM +0800, Hillf Danton wrote:
> Along the probe path,
> 
> em28xx_usb_probe
>   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>   retval = em28xx_init_dev(dev, udev, intf, nr);
>     em28xx_init_extension(dev);
>       em28xx_ir_init(struct em28xx *dev)
>         kref_get(&dev->ref);
> 
>   kref_init(&dev->ref);

Good detective work.

I've created a Smatch check to try find these.  It uses the fact that
Smatch creates a bunch of fake assignments to set all the struct members
of "dev" to zero.  Then it uses the modification hook to find the
kref_init().  Those are sort of new uses for those hooks so that's quite
fun.

I'll test it out overnight and see how it works.

drivers/media/usb/em28xx/em28xx-cards.c:4086 em28xx_usb_probe() warn: kref has already been modifed (see line 3979)

regards,
dan carpenter

#include "smatch.h"
#include "smatch_slist.h"
#include "smatch_extra.h"

static int my_id;

STATE(fresh);

static int get_line(struct sm_state *sm)
{
	struct sm_state *tmp;
	int line = 0;

	FOR_EACH_PTR(sm->possible, tmp) {
		if (tmp->state == &undefined &&
		    tmp->line > line)
			line = tmp->line;
	} END_FOR_EACH_PTR(tmp);

	if (!line)
		return sm->line;
	return line;
}

static void match_modify(struct sm_state *sm, struct expression *mod_expr)
{
	if (sm->state != &fresh &&
	    mod_expr &&
	    mod_expr->type == EXPR_CALL &&
	    sym_name_is("kref_init", mod_expr->fn))
		sm_warning("kref has already been modifed (see line %d)", get_line(sm));

	set_state(my_id, sm->name, sm->sym, &undefined);
}

static bool is_alloc(struct expression *expr)
{
	static struct expression *ignore, *alloc_expr;
	struct expression *right;

	if (!expr || expr->type != EXPR_ASSIGNMENT || expr->op != '=')
		return false;
	if (expr == ignore)
		return false;
	if (expr == alloc_expr)
		return true;
	right = strip_expr(expr->right);
	if (right->type == EXPR_CALL &&
	    (sym_name_is("kzalloc", right->fn) ||
	     sym_name_is("kmalloc", right->fn))) {
		alloc_expr = expr;
		return true;
	}
	ignore = expr;
	return false;
}

static void match_assign(struct expression *expr)
{
	char *name;

	if (!is_alloc(get_faked_expression()))
		return;
	name = expr_to_str(expr->left);
	if (name && strstr(name, "refcount.refs.counter"))
		set_state_expr(my_id, expr->left, &fresh);
	free_string(name);
}

void check_kref_init_too_late(int id)
{
	my_id = id;

	add_hook(&match_assign, ASSIGNMENT_HOOK_AFTER);
	add_modification_hook(my_id, &match_modify);
}