netdev - [PATCH ethtool v2 07/13] ethtool: avoid null pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed,  7 Dec 2022 17:11:16 -0800
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     mkubecek@...e.cz
Cc:     netdev@...r.kernel.org,
        Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: [PATCH ethtool v2 07/13] ethtool: avoid null pointer dereference

'$ 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 <mkubecek@...e.cz>
Fixes: 81a30f416ec7 ("netlink: add bitset command line parser handlers")
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
---
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;
-- 
2.31.1