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

From: Jesse Brandeburg <jesse.brandeburg@intel.com>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: <netdev@vger.kernel.org>
Subject: Re: [PATCH ethtool v2 07/13] ethtool: avoid null pointer dereference
Date: Fri, 9 Dec 2022 09:36:54 -0800	[thread overview]
Message-ID: <c62825b9-e2b2-9293-e36e-c34d83c0d7e6@intel.com> (raw)
In-Reply-To: <20221208062312.2emtsvurflldumsr@lion.mk-sys.cz>

On 12/7/2022 10:23 PM, Michal Kubecek wrote:
> On Wed, Dec 07, 2022 at 05:11:16PM -0800, Jesse Brandeburg wrote:
>> '$ 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
> 
> Again: this has nothing to do with netlink, this is command line parser,
> nlctx->argp is a member of argv[] array. And as execve() (which is the
> only syscall in the exec* family, the rest are wrappers) does not pass
> argc, only argv[], argc is actually determined by kernel so for it to be
> actually null, you would need a serious bug in kernel first.

Thank you for explaining! I can drop this patch, but it's disappointing 
that one fairly cheap conditional will prevent us from being able to 
cleanly run scan-build. If you have any other suggestions please let me 
know (and see below)

I spent some time today trying to get the command line to pass a NULL 
value but I couldn't do it, and elsewhere in the code the checks for 
argc prevent the NULL value or no value from getting into the ethtool 
code parsing the commands, so in this case it's not really a false 
positive, but taken care of by other code that isn't observable to the 
scan-build virtual machine. The good news is I don't see a real issue here.

> Even if we want to be safe against buggy kernel passing garbage as
> command line arguments, I still believe we should do that earlier, in
> the general code, not deep in a specific helper function. Also, you only
> check for null but that does not catch an invalid pointer in argv[]
> which, unlike a null pointer, could do an actual harm. And I don't see
> how that could be checked, I'm afraid we have to trust kernel.

OK, let's trust the kernel, but can we still fix this issue in order to 
be able to add scan-build to a list of tools to run cleanly in automation?

some TL;DR details in case there is someone else that has a suggestion!

Here is the callchain, for reference:
This is from the command
# ethtool -s eth0 wol pumbag

#0  nl_parse_char_bitset
#1  in nl_parser at netlink/parser.c:1099
#2  in nl_sset at netlink/settings.c:1247
#3  in netlink_run_handler at netlink/netlink.c:493
#4  in main at ethtool.c:6425

and in the #0 frame above, *nlctx->argp = "pumbag"
in the callchain above, scan-build doesn't like us de-referencing argp 
because it doesn't have proof it's not null.

Further I tried putting the check in every element of the stack frame 
above and they all fail the scan-build check still, probably because the 
pointer is advanced to the "pumbag" argument later in the code.

Anyway, I'm still working on the v3 of the series.