From patchwork Thu Dec 8 01:11:16 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jesse Brandeburg X-Patchwork-Id: 13067798 X-Patchwork-Delegate: mkubecek+ethtool@suse.cz Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C156C4708E for ; Thu, 8 Dec 2022 01:11:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229912AbiLHBLs (ORCPT ); Wed, 7 Dec 2022 20:11:48 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38856 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229909AbiLHBLi (ORCPT ); Wed, 7 Dec 2022 20:11:38 -0500 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A98C28B394 for ; Wed, 7 Dec 2022 17:11:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1670461896; x=1701997896; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=g4QGfIesznF6QI8eeukcbQ72FnrpCVMFQYdJvcqQpUw=; b=a01xxeJAnqNTydUyRK/EDSQvYxOr4riKpRjyM5adWJyjb2vnKoCR+Xzt 3DWzGOM4YY45HVq+0ca2tHTamHYc2YiKT4bnpHOuqHyAqxi7PoypRe4wY ldAzQgFcRMk/LUeEbMdXRns8+gnxysF1xDqF4vqr3UeRLU9GhTfZY+XHK zwqT+DAcYswxOtGzIMUhCzn0qXVvB5XYgacgC0I+4UxSYbAKsiYIOafw5 qDQWyBjLggTxa1jcsfpeOeIvIHMfDIcl83ucNeNHqsCz3XBrZ+P62JSPS MuzJokE7So6PpczWRDx3SIyakNJj22zdbqwRfK/U9PknNQ40zkpRGkv// g==; X-IronPort-AV: E=McAfee;i="6500,9779,10554"; a="304672877" X-IronPort-AV: E=Sophos;i="5.96,226,1665471600"; d="scan'208";a="304672877" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2022 17:11:33 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10554"; a="640445370" X-IronPort-AV: E=Sophos;i="5.96,226,1665471600"; d="scan'208";a="640445370" Received: from jbrandeb-coyote30.jf.intel.com ([10.166.29.19]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2022 17:11:30 -0800 From: Jesse Brandeburg To: mkubecek@suse.cz Cc: netdev@vger.kernel.org, Jesse Brandeburg Subject: [PATCH ethtool v2 07/13] ethtool: avoid null pointer dereference Date: Wed, 7 Dec 2022 17:11:16 -0800 Message-Id: <20221208011122.2343363-8-jesse.brandeburg@intel.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20221208011122.2343363-1-jesse.brandeburg@intel.com> References: <20221208011122.2343363-1-jesse.brandeburg@intel.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org X-Patchwork-Delegate: mkubecek+ethtool@suse.cz '$ scan-build make' reports: Description: Array access (from variable 'arg') results in a null pointer dereference File: /git/ethtool/netlink/parser.c Line: 782 Description: Dereference of null pointer (loaded from variable 'p') File: /git/ethtool/netlink/parser.c Line: 794 Both of these bugs are prevented by checking the input in nl_parse_char_bitset(), which is called from nl_sset() via the kernel callback, specifically for the parsing of the wake-on-lan options (-s wol). None of the other functions in this file seem to have the issue of deferencing data without checking for validity first. This could "technically" allow nlctxt->argp to be NULL, and scan-build is limited in it's ability to parse for bugs only at file scope in this case. This particular bug should be unlikely to happen because the kernel builds/parses the netlink structure before handing it to the application. However in the interests of being able to run this scan-build tool regularly, I'm still sending the initial version of this patch as I tried several other ways to fix the bug with an earlier check for NULL in nl_sset, but it won't prevent the scan-build error due to the file scope problem. CC: Michal Kubecek Fixes: 81a30f416ec7 ("netlink: add bitset command line parser handlers") Signed-off-by: Jesse Brandeburg --- v2: updated commit message with more nuance after feedback from ethtool maintainer. I'd be open to fixing this a different way but this seemed the most straight-forward with the smallest amount of code changed. v1: original version --- netlink/parser.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netlink/parser.c b/netlink/parser.c index 70f451008eb4..c573a9598a9f 100644 --- a/netlink/parser.c +++ b/netlink/parser.c @@ -874,7 +874,7 @@ int nl_parse_bitset(struct nl_context *nlctx, uint16_t type, const void *data, * optionally followed by '/' and another numeric value (mask, unless no_mask * is set), or a string consisting of characters corresponding to bit indices. * The @data parameter points to struct char_bitset_parser_data. Generates - * biset nested attribute. Fails if type is zero or if @dest is not null. + * bitset nested attribute. Fails if type is zero or if @dest is not null. */ int nl_parse_char_bitset(struct nl_context *nlctx, uint16_t type, const void *data, struct nl_msg_buff *msgbuff, @@ -882,7 +882,7 @@ int nl_parse_char_bitset(struct nl_context *nlctx, uint16_t type, { const struct char_bitset_parser_data *parser_data = data; - if (!type || dest) { + if (!type || dest || !*nlctx->argp) { fprintf(stderr, "ethtool (%s): internal error parsing '%s'\n", nlctx->cmd, nlctx->param); return -EFAULT;