From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34150 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231194AbhHBVAk (ORCPT ); Mon, 2 Aug 2021 17:00:40 -0400 Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 821BEC06175F for ; Mon, 2 Aug 2021 14:00:27 -0700 (PDT) Received: by mail-lf1-x12f.google.com with SMTP id c16so6589203lfc.2 for ; Mon, 02 Aug 2021 14:00:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=QVhjbxiPwZ6eG8ji++fKYOW/Ni3PMhg5PwFPOELEprY=; b=SQXWsz5WqOvombnAXiJ1Ld57sm24JcYpF80Zz19eJwCg5n/PppkzCjIhw1v28IJ927 gCzkma/h6FgOyPF8qMYXevNR6mdeSRYgMElm3WTYZlT8qF5HRYvbXHXiuKdq4fSR61PT /tqP3BmIRdwtjzL6h2azWnM9avSpCzSUlb5eR/opodNj6AAEEyYvwBM+kaMMUe/B8iNQ nxxvgHb92XYq7tXM1YfcHyoPg5dBZ7kVRnXBgmbN7Uv5rSeSoykYU14RU5sHzNNtyYuO o7O+RLpilKo2A63WvBdhw+fmDbslXpsFUahplDYNZtKMzEl7p0FfdpP4D0oFH2uGV8JU gmJg== From: Pavel Skripkin Subject: [PATCH] check_netdev_priv: warn about using netdev priv data after free_netdev Date: Tue, 3 Aug 2021 00:00:22 +0300 Message-Id: <20210802210022.5226-1-paskripkin@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-ID: To: smatch@vger.kernel.org Cc: Pavel Skripkin After manual code review I found a lot of bugs, when code uses netdev_priv() pointer after free_{netdev,candev} call. Example: struct some_dev *dev = netdev_priv(net_dev); free_netdev(net_dev); do_clean_up(dev); Obviously, that above code snippet will trigger UAF bug, since dev pointer goes away with net_dev. Since there isn't any check for this type of bug in other static analysis tools, let's add it to smatch. Side note: this code requires further work to be able to find complex situtations like struct some_dev *dev = get_dev_from_smth(smth); free_netdev(dev->netdev); do_clean_up(dev); In this case dev is dev->netdev private data and it's a bit difficult to to figure out if ->netdev is actual dev _parent_ or not. Signed-off-by: Pavel Skripkin --- check_list.h | 1 + check_netdev_priv.c | 117 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 check_netdev_priv.c diff --git a/check_list.h b/check_list.h index fd205269..972b2795 100644 --- a/check_list.h +++ b/check_list.h @@ -203,6 +203,7 @@ CK(check_list_add) CK(check_list_add_late) CK(check_sscanf_return) CK(check_kvmalloc_NOFS) +CK(check_uaf_netdev_priv) /* wine specific stuff */ CK(check_wine_filehandles) diff --git a/check_netdev_priv.c b/check_netdev_priv.c new file mode 100644 index 00000000..87d373b5 --- /dev/null +++ b/check_netdev_priv.c @@ -0,0 +1,117 @@ +/* SPDX-License-Identifier: MIT + * + * Copyright (C) 2021 Pavel Skripkin + */ + +/* TODO: + * + * Try to find a way how to handle situations like: + * + * struct some_dev *dev = get_dev_from_smth(smth); + * + * free_netdev(dev->netdev); + * do_clean_up(dev); + * + * + * In this case dev is dev->netdev private data (exmpl: ems_usb_disconnect()) + */ + +#include "smatch.h" +#include "smatch_extra.h" +#include "smatch_function_hashtable.h" + +static int my_id; +STATE(freed); +STATE(ok); + +static void ok_to_use(struct sm_state *sm, struct expression *mod_expr) +{ + if (sm->state != &ok) + set_state(my_id, sm->name, sm->sym, &ok); +} + +static inline char *get_function_name(struct expression *expr) +{ + if (!expr || expr->type != EXPR_CALL) + return NULL; + if (expr->fn->type != EXPR_SYMBOL || !expr->fn->symbol_name) + return NULL; + return expr->fn->symbol_name->name; +} + +static inline int is_netdev_priv(struct expression *call) +{ + char *name; + + if (!call || call->type != EXPR_CALL) + return 0; + + name = get_function_name(call); + if (!name) + return 0; + + return !strcmp("netdev_priv", name); +} + +static const char *get_parent_netdev_name(struct expression *expr) +{ + struct expression *call, *arg_expr = NULL; + struct symbol *sym; + + call = get_assigned_expr(strip_expr(expr)); + if (is_netdev_priv(call)) { + arg_expr = get_argument_from_call_expr(call->args, 0); + arg_expr = strip_expr(arg_expr); + } else { + return NULL; + } + + return expr_to_var_sym(arg_expr, &sym); +} + +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); +} + +static void match_symbol(struct expression *expr) +{ + const char *parent_netdev, *name; + struct smatch_state *state; + + 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); + add_function_hook("free_candev", &match_free_netdev, NULL); + add_modification_hook(my_id, &ok_to_use); + add_hook(&match_symbol, SYM_HOOK); +} -- 2.32.0