From 60f28307ac7d8ddbfe07e29481fd8926eecc85a7 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 8 Aug 2018 15:57:47 +0300 Subject: [PATCH] user_data: improve how returned data is handled There were three issues here. Returns weren't handled properly if you had a return like "return simple_strtol();". Second is that nla_data() wasn't marked as returning user data. Third if you had a dereference like *(u8 *)nla_data() that wasn't handled correctly. The simple_strtol() function was only being hooked in when it was used in an assignment. This might be a bigger issue that I could handle better by faking an assignment when we hit a return... Anyway, for now I've just made a list of function which return user data. So now there are three ways that simple_strtol() is hooked in. If you have an assignment that's saved. If it's called at all then we mark the function as setting user data. And if we call __smatch_user_rl(simple_strtol()); then it just looks up the function from the list in the returns_use_data[] array. There is some overlap and duplication here, but I think it doesn't affect the output. Then when we hit a return, if we return a function call we can look that information up separately and return the range list. The next problem is that nla_data() wasn't marked as returning user data. The reason for this is because nlmsg_data() does pointer math and smatch can't handle it. In the olden days, we maybe would have marked *nlh as user data and we could have made that work, but I removed that code because it kind of duplicated looking up the struct members and it wasn't clear which information should trump the other. Anyway, nla_data() is normally verified early, on but Smatch is crap at tracking arrays so we lose that information. Before Spectre, it wasn't useful to know that nla_data() was user information because we trusted it but now I want to mark it as user data but trusted so that we don't get hundreds of array overflow warnings. So I manually went into smatch_capped.c and marked all the functions with "nla_get_" in the name as capped. The final change is that in var_user_rl() if we have a "*foo" dereference we look up "foo" to see if it's marked as user data, and if so then "*foo" is also user data. Reported-by: Sabrina Dubroca Reported-by: Josh Poimboeuf Signed-off-by: Dan Carpenter --- check_user_data2.c | 75 ++++++++++++++++++++++++++++++++++++++---------------- smatch_capped.c | 12 +++++++++ 2 files changed, 65 insertions(+), 22 deletions(-) diff --git a/check_user_data2.c b/check_user_data2.c index 34000811..0e31e35a 100644 --- a/check_user_data2.c +++ b/check_user_data2.c @@ -43,6 +43,12 @@ static const char * kstr_funcs[] = { "kstrtos32_from_user", }; +static const char *returns_user_data[] = { + "simple_strtol", "simple_strtoll", "simple_strtoul", "simple_strtoull", + "kvm_register_read", "nlmsg_data", "nla_data", "memdup_user", + "kmap_atomic", "skb_network_header", +}; + static void set_points_to_user_data(struct expression *expr); static struct stree *start_states; @@ -341,9 +347,27 @@ static int is_skb_data(struct expression *expr) return 1; } -static int points_to_user_data(struct expression *expr) +static int get_rl_from_function(struct expression *expr, struct range_list **rl) +{ + int i; + + if (expr->type != EXPR_CALL || expr->fn->type != EXPR_SYMBOL || + !expr->fn->symbol_name || !expr->fn->symbol_name->name) + return 0; + + for (i = 0; i < ARRAY_SIZE(returns_user_data); i++) { + if (strcmp(expr->fn->symbol_name->name, returns_user_data[i]) == 0) { + *rl = alloc_whole_rl(get_type(expr)); + return 1; + } + } + return 0; +} + +int points_to_user_data(struct expression *expr) { struct smatch_state *state; + struct range_list *rl; char buf[256]; struct symbol *sym; char *name; @@ -585,23 +609,13 @@ static void match_condition(struct expression *expr) static void match_user_assign_function(const char *fn, struct expression *expr, void *unused) { - func_gets_user_data = true; - tag_as_user_data(expr->left); set_points_to_user_data(expr->left); } static void match_returns_user_rl(const char *fn, struct expression *expr, void *unused) { - struct smatch_state *estate; - struct range_list *rl; - func_gets_user_data = true; - - rl = alloc_whole_rl(get_type(expr->right)); - rl = cast_rl(get_type(expr->left), rl); - estate = alloc_estate_rl(rl); - set_state_expr(my_id, expr->left, estate); } static int get_user_macro_rl(struct expression *expr, struct range_list **rl) @@ -776,6 +790,9 @@ static struct range_list *var_user_rl(struct expression *expr) return NULL; } + if (get_rl_from_function(expr, &rl)) + goto found; + if (get_user_macro_rl(expr, &rl)) goto found; @@ -802,6 +819,12 @@ static struct range_list *var_user_rl(struct expression *expr) } } + if (expr->type == EXPR_PREOP && expr->op == '*' && + is_user_rl(expr->unop)) { + rl = var_to_absolute_rl(expr); + goto found; + } + return NULL; found: user_data_flag = 1; @@ -1059,10 +1082,12 @@ static void param_set_to_user_data(int return_id, char *return_ranges, struct ex { struct sm_state *sm; struct smatch_state *start_state; + struct range_list *rl; int param; char *return_str; const char *param_name; struct symbol *ret_sym; + bool return_found = false; expr = strip_expr(expr); return_str = expr_to_str(expr); @@ -1106,21 +1131,31 @@ static void param_set_to_user_data(int return_id, char *return_ranges, struct ex goto free_string; } - if (!ret_sym) - goto free_string; FOR_EACH_MY_SM(my_id, __get_cur_stree(), sm) { + if (!ret_sym) + break; if (ret_sym != sm->sym) continue; param_name = state_name_to_param_name(sm->name, return_str); if (!param_name) continue; + if (strcmp(param_name, "$") == 0) + return_found = true; sql_insert_return_states(return_id, return_ranges, func_gets_user_data ? USER_DATA3_SET : USER_DATA3, -1, param_name, show_rl(estate_rl(sm->state))); } END_FOR_EACH_SM(sm); + + if (!return_found && get_user_rl(expr, &rl)) { + sql_insert_return_states(return_id, return_ranges, + func_gets_user_data ? USER_DATA3_SET : USER_DATA3, + -1, "$", show_rl(rl)); + goto free_string; + } + free_string: free_string(return_str); } @@ -1169,19 +1204,15 @@ void check_user_data2(int id) add_function_hook("memcpy_fromiovec", &match_user_copy, INT_PTR(0)); for (i = 0; i < ARRAY_SIZE(kstr_funcs); i++) add_function_hook(kstr_funcs[i], &match_user_copy, INT_PTR(2)); + add_function_hook("usb_control_msg", &match_user_copy, INT_PTR(6)); - add_function_assign_hook("simple_strtol", &match_returns_user_rl, NULL); - add_function_assign_hook("simple_strtoll", &match_returns_user_rl, NULL); - add_function_assign_hook("simple_strtoul", &match_returns_user_rl, NULL); - add_function_assign_hook("simple_strtoull", &match_returns_user_rl, NULL); - add_function_assign_hook("kvm_register_read", &match_returns_user_rl, NULL); + for (i = 0; i < ARRAY_SIZE(returns_user_data); i++) { + add_function_assign_hook(returns_user_data[i], &match_user_assign_function, NULL); + add_function_hook(returns_user_data[i], &match_returns_user_rl, NULL); + } add_function_hook("sscanf", &match_sscanf, NULL); - add_function_assign_hook("memdup_user", &match_user_assign_function, NULL); - add_function_assign_hook("kmap_atomic", &match_user_assign_function, NULL); - add_function_assign_hook("skb_network_header", &match_user_assign_function, NULL); - add_hook(&match_syscall_definition, AFTER_DEF_HOOK); add_hook(&match_assign, ASSIGNMENT_HOOK); diff --git a/smatch_capped.c b/smatch_capped.c index a793a930..a42303d4 100644 --- a/smatch_capped.c +++ b/smatch_capped.c @@ -221,6 +221,7 @@ static void print_return_implies_capped(int return_id, char *return_ranges, stru char *return_str; int param; sval_t sval; + bool return_found = false; expr = strip_expr(expr); return_str = expr_to_str(expr); @@ -259,10 +260,21 @@ static void print_return_implies_capped(int return_id, char *return_ranges, stru param_name = state_name_to_param_name(sm->name, return_str); if (!param_name) continue; + if (strcmp(param_name, "$") == 0) + return_found = true; sql_insert_return_states(return_id, return_ranges, CAPPED_DATA, -1, param_name, "1"); } END_FOR_EACH_SM(sm); + if (return_found) + goto free_string; + + if (option_project == PROJ_KERNEL && get_function() && + strstr(get_function(), "nla_get_")) + sql_insert_return_states(return_id, return_ranges, CAPPED_DATA, + -1, "$", "1"); + +free_string: free_string(return_str); } -- 2.11.4.GIT