From 9d67605695689906f01ec1163c2932549b062219 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 17 Jan 2018 22:57:26 +0300 Subject: [PATCH] check_free_strict: New stricter cross function use after free check The normal check_frees_param.c records that a function frees a param if every path through the function frees the param (or the param is NULL). But there are other functions which maybe only free the param on error or in certain paths through the function. This is much more ambitious to check for those cases, but it's what the check does. The new check is kernel only because there are a lot of places where we only free the last user. The overwhelming most common way for the kernel to determine the last user is by doing an atomic_inc() when we have a new user and an atomic_dec() when the user is done. This check looks for that specifically so it would probably have a lot of false positives in non-kernel code. Signed-off-by: Dan Carpenter --- check_free.c | 12 ++- check_free.c => check_free_strict.c | 46 ++++++---- check_frees_param.c | 12 +-- check_frees_param_strict.c | 162 ++++++++++++++++++++++++++++++++++++ check_list.h | 4 +- smatch_extra.h | 1 + 6 files changed, 212 insertions(+), 25 deletions(-) copy check_free.c => check_free_strict.c (83%) create mode 100644 check_frees_param_strict.c diff --git a/check_free.c b/check_free.c index 0cb78d38..be84c15b 100644 --- a/check_free.c +++ b/check_free.c @@ -23,6 +23,7 @@ #include #include "smatch.h" #include "smatch_slist.h" +#include "smatch_extra.h" static int my_id; @@ -236,6 +237,9 @@ int parent_is_free_var_sym(const char *name, struct symbol *sym) char *end; struct smatch_state *state; + if (option_project == PROJ_KERNEL) + return parent_is_free_var_sym_strict(name, sym); + strncpy(buf, name, sizeof(buf) - 1); buf[sizeof(buf) - 1] = '\0'; @@ -273,12 +277,12 @@ void check_free(int id) my_id = id; if (option_project == PROJ_KERNEL) { - add_function_hook("kfree", &match_free, INT_PTR(0)); - add_function_hook("kmem_cache_free", &match_free, INT_PTR(1)); - } else { - add_function_hook("free", &match_free, INT_PTR(0)); + /* The kernel use check_free_strict.c */ + return; } + add_function_hook("free", &match_free, INT_PTR(0)); + if (option_spammy) add_hook(&match_symbol, SYM_HOOK); add_hook(&match_dereferences, DEREF_HOOK); diff --git a/check_free.c b/check_free_strict.c similarity index 83% copy from check_free.c copy to check_free_strict.c index 0cb78d38..7c033076 100644 --- a/check_free.c +++ b/check_free_strict.c @@ -215,21 +215,39 @@ static void match_free(const char *fn, struct expression *expr, void *param) set_state_expr(my_id, arg, &freed); } -static void set_param_freed(struct expression *call, struct expression *arg, char *key, char *unused) +static void set_param_freed(struct expression *expr, int param, char *key, char *value) { - struct symbol *sym; + struct expression *arg; char *name; + struct symbol *sym; + struct sm_state *sm; + while (expr->type == EXPR_ASSIGNMENT) + expr = strip_expr(expr->right); + if (expr->type != EXPR_CALL) + return; + + arg = get_argument_from_call_expr(expr->args, param); + if (!arg) + return; name = get_variable_from_key(arg, key, &sym); if (!name || !sym) goto free; + if (!is_impossible_path()) { + sm = get_sm_state(my_id, name, sym); + if (sm && slist_has_state(sm->possible, &freed)) { + sm_msg("warn: '%s' double freed", name); + set_state(my_id, name, sym, &ok); /* fixme: doesn't silence anything. I know */ + } + } + set_state(my_id, name, sym, &freed); free: free_string(name); } -int parent_is_free_var_sym(const char *name, struct symbol *sym) +int parent_is_free_var_sym_strict(const char *name, struct symbol *sym) { char buf[256]; char *start; @@ -252,7 +270,7 @@ int parent_is_free_var_sym(const char *name, struct symbol *sym) return 0; } -int parent_is_free(struct expression *expr) +int parent_is_free_strict(struct expression *expr) { struct symbol *sym; char *var; @@ -262,22 +280,21 @@ int parent_is_free(struct expression *expr) var = expr_to_var_sym(expr, &sym); if (!var || !sym) goto free; - ret = parent_is_free_var_sym(var, sym); + ret = parent_is_free_var_sym_strict(var, sym); free: free_string(var); return ret; } -void check_free(int id) +void check_free_strict(int id) { my_id = id; - if (option_project == PROJ_KERNEL) { - add_function_hook("kfree", &match_free, INT_PTR(0)); - add_function_hook("kmem_cache_free", &match_free, INT_PTR(1)); - } else { - add_function_hook("free", &match_free, INT_PTR(0)); - } + if (option_project != PROJ_KERNEL) + return; + + add_function_hook("kfree", &match_free, INT_PTR(0)); + add_function_hook("kmem_cache_free", &match_free, INT_PTR(1)); if (option_spammy) add_hook(&match_symbol, SYM_HOOK); @@ -285,7 +302,8 @@ void check_free(int id) add_hook(&match_call, FUNCTION_CALL_HOOK); add_hook(&match_return, RETURN_HOOK); - add_modification_hook(my_id, &ok_to_use); - select_call_implies_hook(PARAM_FREED, &set_param_freed); + add_modification_hook_late(my_id, &ok_to_use); add_pre_merge_hook(my_id, &pre_merge_hook); + + select_return_states_hook(PARAM_FREED, &set_param_freed); } diff --git a/check_frees_param.c b/check_frees_param.c index 91a2353f..83ecdfb0 100644 --- a/check_frees_param.c +++ b/check_frees_param.c @@ -106,15 +106,15 @@ void check_frees_param(int id) { my_id = id; - add_hook(&match_function_def, FUNC_DEF_HOOK); - if (option_project == PROJ_KERNEL) { - add_function_hook("kfree", &match_free, INT_PTR(0)); - add_function_hook("kmem_cache_free", &match_free, INT_PTR(1)); - } else { - add_function_hook("free", &match_free, INT_PTR(0)); + /* The kernel uses check_frees_param_strict.c */ + return; } + add_hook(&match_function_def, FUNC_DEF_HOOK); + + add_function_hook("free", &match_free, INT_PTR(0)); + select_call_implies_hook(PARAM_FREED, &set_param_freed); add_modification_hook(my_id, &set_ignore); diff --git a/check_frees_param_strict.c b/check_frees_param_strict.c new file mode 100644 index 00000000..debad846 --- /dev/null +++ b/check_frees_param_strict.c @@ -0,0 +1,162 @@ +/* + * Copyright (C) 2014 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 + */ + +/* + * This file is sort of like check_dereferences_param.c. In theory the one + * difference should be that the param is NULL it should still be counted as a + * free. But for now I don't handle that case. + */ + +#include "smatch.h" +#include "smatch_extra.h" +#include "smatch_slist.h" + +static int my_id; + +STATE(freed); +STATE(ignore); + +static struct smatch_state *unmatched_state(struct sm_state *sm) +{ + if (sm->state != &freed) + return &undefined; + if (parent_is_null_var_sym(sm->name, sm->sym)) + return &freed; + return &undefined; +} + +static void set_ignore(struct sm_state *sm, struct expression *mod_expr) +{ + set_state(my_id, sm->name, sm->sym, &ignore); +} + +static int counter_was_inced(struct expression *expr) +{ + char *name; + struct symbol *sym; + char buf[256]; + int ret = 0; + + name = expr_to_var_sym(expr, &sym); + if (!name || !sym) + goto free; + + snprintf(buf, sizeof(buf), "%s->users.counter", name); + ret = was_inced(buf, sym); +free: + free_string(name); + return ret; +} + +static void match_free(const char *fn, struct expression *expr, void *param) +{ + struct expression *arg, *tmp; + int cnt = 0; + + arg = get_argument_from_call_expr(expr->args, PTR_INT(param)); + if (!arg) + return; + while ((tmp = get_assigned_expr(arg))) { + arg = strip_expr(tmp); + if (cnt++ > 5) + break; + } + + if (get_param_num(arg) < 0) + return; + if (param_was_set(arg)) + return; + if (strcmp(fn, "kfree_skb") == 0 && counter_was_inced(arg)) + return; + + set_state_expr(my_id, arg, &freed); +} + +static void set_param_freed(struct expression *expr, int param, char *key, char *value) +{ + struct expression *arg; + char *name; + struct symbol *sym; + + while (expr->type == EXPR_ASSIGNMENT) + expr = strip_expr(expr->right); + if (expr->type != EXPR_CALL) + return; + + arg = get_argument_from_call_expr(expr->args, param); + if (!arg) + return; + name = get_variable_from_key(arg, key, &sym); + if (!name || !sym) + goto free; + if (get_param_num_from_sym(sym) < 0) + goto free; + + if (param_was_set_var_sym(name, sym)) + goto free; + + set_state(my_id, name, sym, &freed); +free: + free_string(name); +} + +static void param_freed_info(int return_id, char *return_ranges, struct expression *expr) +{ + struct sm_state *sm; + int param; + const char *param_name; + + if (on_atomic_dec_path()) + return; + + FOR_EACH_MY_SM(my_id, __get_cur_stree(), sm) { + if (sm->state != &freed) + continue; + + param = get_param_num_from_sym(sm->sym); + if (param < 0) + continue; + + param_name = get_param_name(sm); + if (!param_name) + continue; + + sql_insert_return_states(return_id, return_ranges, PARAM_FREED, + param, param_name, ""); + } END_FOR_EACH_SM(sm); +} + +void check_frees_param_strict(int id) +{ + my_id = id; + + if (option_project != PROJ_KERNEL) + return; + + add_function_hook("kfree", &match_free, INT_PTR(0)); + add_function_hook("kmem_cache_free", &match_free, INT_PTR(1)); + add_function_hook("kfree_skb", &match_free, INT_PTR(0)); + add_function_hook("kfree_skbmem", &match_free, INT_PTR(0)); + add_function_hook("dma_pool_free", &match_free, INT_PTR(1)); + add_function_hook("spi_unregister_controller", &match_free, INT_PTR(0)); + + select_return_states_hook(PARAM_FREED, &set_param_freed); + add_modification_hook(my_id, &set_ignore); + add_split_return_callback(¶m_freed_info); + + add_unmatched_state_hook(my_id, &unmatched_state); +} diff --git a/check_list.h b/check_list.h index 52abfbc0..5e0d4a08 100644 --- a/check_list.h +++ b/check_list.h @@ -74,7 +74,6 @@ CK(check_bogus_loop) CK(check_deref) CK(check_check_deref) CK(check_dereferences_param) -CK(check_frees_param) CK(check_index_overflow) CK(check_testing_index_after_use) CK(check_memcpy_overflow) @@ -102,7 +101,10 @@ CK(check_resource_size) CK(check_release_resource) CK(check_proc_create) CK(check_freeing_null) +CK(check_frees_param) CK(check_free) +CK(check_frees_param_strict) +CK(check_free_strict) CK(check_no_effect) CK(check_kunmap) CK(check_snprintf) diff --git a/smatch_extra.h b/smatch_extra.h index cbac7b9b..de010dc7 100644 --- a/smatch_extra.h +++ b/smatch_extra.h @@ -166,6 +166,7 @@ int implied_not_equal(struct expression *expr, long long val); int implied_not_equal_name_sym(char *name, struct symbol *sym, long long val); int parent_is_null_var_sym(const char *name, struct symbol *sym); int parent_is_null(struct expression *expr); +int parent_is_free_var_sym_strict(const char *name, struct symbol *sym); int parent_is_free_var_sym(const char *name, struct symbol *sym); int parent_is_free(struct expression *expr); -- 2.11.4.GIT