From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <smatch-owner@vger.kernel.org>
Received: from mx0b-00069f02.pphosted.com ([205.220.177.32]:32932 "EHLO
        mx0b-00069f02.pphosted.com" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S234206AbhHCPI6 (ORCPT
        <rfc822;smatch@vger.kernel.org>); Tue, 3 Aug 2021 11:08:58 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc
 : subject : message-id : references : content-type : in-reply-to :
 mime-version; s=corp-2021-07-09;
 bh=GECx6QOML3zV/qq6UiFOn75IczzhhFqVhDNMpX5Hmlc=;
 b=UzKaTPU2NvQHkeHPGe+0VDx3zIoRC6dGkHaYnb3LO7P2LcpswfOQo1OjfOvNQidAFMh1
 23rLQCjy86ERV9vVAn0rc/5sw/DYpv7QIqiyGWFf5ctE0DvEW1gUH8Za5YzEYnTVqnwF
 BuWsGuXgS861O1sb9xFaSnuuPOrUQLRzuTsOS/AsGKy5OuFK9CTmw2+sc5/+VFkw1hMm
 hB1UtBqJsMXu4LYUuuR71NysqAUAxx6EEJ8Q8YOa9+Pj0DuT4wxYCmUqOhK/ajcCxEQu
 U/wgIl5k9Ts8w8zhSINM2a4nCZdmDxS2zWRBDbm7kB9a2M66jfx0UFLPuul3CKqxEDOk Wg== 
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc
 : subject : message-id : references : content-type : in-reply-to :
 mime-version; s=corp-2020-01-29;
 bh=GECx6QOML3zV/qq6UiFOn75IczzhhFqVhDNMpX5Hmlc=;
 b=bQpCMpEy6HEXQM8HxANTFOS7KqykypZRdBo1kB4muz2AwFn5GMwBhdJc/4ZPXGj1ZHaP
 0ZW+7mrXv66tccAqwSbH3t2XJZqa4W7etdjZc/iQJmwP7lnd2CrfyZRTP9Qjh027HTSm
 982C0jiTYYIFU1hWtFGMOIKgrTj6U8BUUN9ENQoOaAdvm5LNk1PL2asNz7AzXmjSAOt1
 ODdHIj6jYuEv6U+YRDfzZrLBZOGJimI/mBJfWcJqhf5CpFDxSKHvzUZVbYgZNTH7Tjle
 N+vgswRsvnzXnSjSSLnK9AB/+X3FwNluxkkSE/La9sD0WJBFu+DJRcCwqjSVMyCP4OTR 9g== 
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=oracle.onmicrosoft.com; s=selector2-oracle-onmicrosoft-com;
 h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;
 bh=GECx6QOML3zV/qq6UiFOn75IczzhhFqVhDNMpX5Hmlc=;
 b=Sv1kO/jrSYSaxjyCd+cikhZX5cderbwpvBhxwdK1BYi4xUqT8dPFKw6ztxiIlznLr3tsw+uBhtP6pZEIo7rQT+lmBDi0MtkopTtz25rMvk0Qygg4xG12lnwO9+HElxlb3LF8slPsUN+QOouvHPRXHrh39fwv05gQBO1oMprdUWQ=
Date: Tue, 3 Aug 2021 18:08:26 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH] check_netdev_priv: warn about using netdev priv data
 after free_netdev
Message-ID: <20210803150826.GC1931@kadam>
References: <20210802210022.5226-1-paskripkin@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210802210022.5226-1-paskripkin@gmail.com>
MIME-Version: 1.0
List-ID: <smatch.vger.kernel.org>
To: Pavel Skripkin <paskripkin@gmail.com>
Cc: smatch@vger.kernel.org

Thanks Pavel!

It looks really nice.  I've applied it.  I'll test it tonight and push
tomorrow.

I don't see any major issues with the check at all, but I have a few
comments below.

On Tue, Aug 03, 2021 at 12:00:22AM +0300, Pavel Skripkin wrote:
> +static void match_free_netdev(const char *fn, struct expression *expr, void *_arg_no)
> +{
> +	struct expression *arg;
> +	const char *name;
> +
> +	arg = get_argument_from_call_expr(expr->args, PTR_INT(_arg_no));
> +	if (!arg)
> +		return;
> +
> +	name = expr_to_var(arg);
> +	if (!name)
> +		return;
> +
> +	set_state(my_id, name, NULL, &freed);
> +}

There is a new param_key API which would make this function shorter.

static void free_netdev(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	set_state(my_id, name, NULL, &freed);
}

Then in the register function you'd add a hooks like this:

	add_function_param_key_hook("free_netdev", &free_netdev, 0, "$", NULL);
	add_function_param_key_hook("free_candev", &free_netdev, 0, "$", NULL);

Of course, you've already written your own code which works so it's
fine.  But that param_key API is really the most awesome thing.

> +
> +static void match_symbol(struct expression *expr)
> +{
> +	const char *parent_netdev, *name;
> +	struct smatch_state *state;
> +

This hook is run for every variable so it's tempting to add a shortcut
here:

	if (!has_states(my_id))
		return;

> +	name = expr_to_var(expr);
> +	if (!name)
> +		return;
> +
> +	parent_netdev = get_parent_netdev_name(expr);
> +	if (!parent_netdev)
> +		return;
> +	
> +	state = get_state(my_id, parent_netdev, NULL);
> +	if (state == &freed)
> +		sm_error("Using %s after free_{netdev,candev}(%s);\n", name, parent_netdev);
> +}
> +
> +void check_uaf_netdev_priv(int id)
> +{
> +	if (option_project != PROJ_KERNEL)
> +		return;
> +
> +	my_id = id;
> +
> +	add_function_hook("free_netdev", &match_free_netdev, NULL);

NULL works but INT_PTR(0) is nicer.  ;)

> +	add_function_hook("free_candev", &match_free_netdev, NULL);
> +	add_modification_hook(my_id, &ok_to_use);
> +	add_hook(&match_symbol, SYM_HOOK);
> +}

Anyway, I think I will probably add the has_states() check but the
rest isn't important.

regards,
dan carpenter