Re: [PATCH] mm: hugetlb: checking for IS_ERR() instead of NULL

[Nigel Christian <nigel.l.christian@gmail.com>]

On Tue, Jun 01, 2021 at 11:50:06PM +0300, Dan Carpenter wrote:
> On Tue, Jun 01, 2021 at 10:51:23PM +0300, Dan Carpenter wrote:
> > The other thing which might be interesting is if you pass a NULL
> > to IS_ERR() and then dereference the NULL then print a warning about
> > that.  This has a lot of overlaps with some of my existing checks, but
> > it's still a new idea so it belongs in a separate check.  It's fine and
> > good even if one bug triggers a lot of different warnings.  I'll write
> > that, hang on, brb.
> 
> 100% untested.  :)  I'll test it tonight.

Ha, you make it look easy. Let me know if I can help with testing
Should I just add below to my smatch and recompile,
or is there an experimental branch to build from?

> 
> There are a bunch of ways to write a check like this.  This test is
> based on copy and paste, guess work, and instinct.  I normally just
> start writing the simplest check I can and test that, then I refine it
> based on whatever the common false postives are.
> 
> In this code, do I need to have a modification hook?  Probably not, but
> it was in the original code I copy and pasted and it seemed harmless.
> Slightly ugly perhaps?
> 
> I knew from experience that I want to check if it's an explicit NULL
> pointer passed to IS_ERR().  There are a few ways to write that.  I
> could have looked at the values or I could have looked at the ->possible
> values.  I probably should have looked at the values instead...
> 
> The __in_fake_assign assignment is copy and paste.  I shoud probably
> delete that but it's harmless and a potential speed up.  It was in the
> check_check_deref.c and I don't remember why.  Probably it's essential.
> 
> I'm not happy with the DEREF_HOOK api.  I've been planning to re-write
> that but I haven't yet.
> 
> 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"
> #include "smatch_extra.h"
> #include "smatch_slist.h"
> 
> static int my_id;
> 
> STATE(null);
> 
> static void is_ok(struct sm_state *sm, struct expression *mod_expr)
> {
> 	set_state(my_id, sm->name, sm->sym, &undefined);
> }
> 
> /*
>  * The expr_has_null_exact() function means that it was explicitly
>  * assigned NULL, not just that it is potentially NULL.
>  */
> static bool expr_has_null_exact(struct expression *expr)
> {
> 	struct sm_state *sm, *tmp;
> 	sval_t sval;
> 
> 	sm = get_sm_state_expr(SMATCH_EXTRA, expr);
> 	if (!sm)
> 		return false;
> 
> 	FOR_EACH_PTR(sm->possible, tmp) {
> 		if (!estate_get_single_value(tmp->state, &sval))
> 			continue;
> 		if (sval.value == 0)
> 			return true;
> 	} END_FOR_EACH_PTR(tmp);
> 
> 	return false;
> }
> 
> static void match_is_err(const char *fn, struct expression *expr, void *unused)
> {
> 	struct expression *arg;
> 
> 	arg = get_argument_from_call_expr(expr->args, 0);
> 	if (!expr_has_null_exact(arg))
> 		return;
> 	set_state_expr(my_id, arg, &null);
> }
> 
> static void check_dereference(struct expression *expr)
> {
> 	char *name;
> 
> 	if (__in_fake_assign)
> 		return;
> 
> 	if (get_state_expr(my_id, expr) != &null)
> 		return;
> 	if (implied_not_equal(expr, 0))
> 		return;
> 
> 	name = expr_to_str(expr);
> 	sm_error("potential NULL dereference '%s'", name);
> 	free_string(name);
> }
> 
> static void match_dereferences(struct expression *expr)
> {
> 	if (expr->type != EXPR_PREOP)
> 		return;
> 	check_dereference(expr->unop);
> }
> 
> void check_null_deref_after_IS_ERR(int id)
> {
> 	my_id = id;
> 
> 	if (option_project != PROJ_KERNEL)
> 		return;
> 
> 	add_function_hook("IS_ERR", &match_is_err, NULL);
> 	add_hook(&match_dereferences, DEREF_HOOK);
> 
> 	add_modification_hook(my_id, &is_ok);
> }
> 
>