Hi, Dan! On 8/3/21 6:08 PM, Dan Carpenter wrote:
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);
I guess, I missed that API, sorry :( Next time I will use it instead.
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.
Thank you for applying and your guidelines, I appreciate it :) With regards, Pavel Skripkin