netdev - Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.

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: Mon, 8 May 2023 09:40:39 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Nicholas Vinson <nvinson234@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference
 issues.

On Sun, May 07, 2023 at 10:46:05PM -0400, Nicholas Vinson wrote:
> 
> On 5/7/23 18:57, Michal Kubecek wrote:
> > On Sun, Apr 30, 2023 at 06:50:52PM -0400, Nicholas Vinson wrote:
> > > Found via gcc -fanalyzer. Analyzer claims that it's possible certain
> > > functions may receive a NULL pointer when handling CLI arguments. Adding
> > > NULL pointer checks to correct the issues.
> > > 
> > > Signed-off-by: Nicholas Vinson <nvinson234@...il.com>
> > A similar theoretical issue was discussed recently:
> > 
> >    https://patchwork.kernel.org/project/netdevbpf/patch/20221208011122.2343363-8-jesse.brandeburg@intel.com/
> > 
> > My position is still the same: argv[] members cannot be actually null
> > unless there is a serious kernel bug (see the link above for an
> > explanation). I'm not opposed to doing a sanity check just in case but
> > if we do, I believe we should check the whole argv[] array right at the
> > beginning and be done with it rather than add specific checks to random
> > places inside parser code.
> 
> By convention and POSIX standard, the last argv[] member is always set to
> NULL and is accessed from main(int argc, char **argp) via argp[argc] (see
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html).
> It's also possible for argc to be zero. In such a case, find_option(NULL)
> would get called.

Please note that ethtool is not a utility for a general POSIX system.
It is a very specific utility which works and makes sense only on Linux.
That's why it can and does take many assumptions which are only
guaranteed on Linux but may not be true on other POSIX systems.

> However, after reviewing main(), I recommend changing:
> 
>         if (argc == 0)
> 
>                 exit_bad_args();
> 
> to
> 
>         if (argc <= 0 || !*argp)
> 
>                 exit_bad_args();
> 
> 
> as this fixes the potential issue of main()'s argc being 0 (argc would be -1
> at this point in such cases), and "!*argp" silences gcc's built-in analyzer
> (and should silence all other SA with respect to the reported issue) as the
> SA doesn't recognize that it would take a buggy execve implementation to
> allow argp to be NULL at this point ).
> 
> If you don't have any objections to this change, I can draft an updated
> patch to make this change.

As I said before, I do not object to adding a sanity check of argc/argv,
I only objected to adding random checks inside the parser code just to
make static checkers happy. If you want to add (or strengthen) a sanity
check right at the beginning of the program, before any parsing, I have
no problem with that.

Michal

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)