netdev - Re: [PATCH] ethtool: fix potential NULL pointer dereference in find

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] [day] [month] [year] [list]

Message-ID: <bgad547yea4te5v36p3hoanl6o64skilv74gugf6jof2y2kryb@to24utsoxqmz>
Date: Mon, 21 Jul 2025 23:16:46 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Anton Moryakov <ant.v.moryakov@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] ethtool: fix potential NULL pointer dereference in
 find_option

On Wed, Jul 09, 2025 at 01:41:20PM GMT, Anton Moryakov wrote:
> Static analyzer reported a possible NULL pointer dereference:
> 
> - In main(), 'argp' is dereferenced and passed to find_option()
> 	without checking if *argp is NULL.

Not true. This is what the code looks like:

	if (!*argp)
		exit_bad_args();

	k = find_option(*argp); 

On the other hand, there is no such check when find_option() is called
from do_perqueue(). But if we really want to address this, I would
rather do it in a consistent way, i.e. the same way in both places.

However, I'm still not convinced the inconsistency between argc and
argp[] you are trying to prevent can actually happen on Linux. AFAIK all
exec* functions are only library wrappers for execve() which is passed
only the argv[] array and the argc count is determined from it by
finding the first null pointer. Unless I'm missing something important,
this means that argp[i] can never be null for i < argc.

Do you have a scenario (not assuming a serious kernel bug) that would
result in null pointer in argp[] before the argc index?

> Additionally, it improves robustness by making sure that the input argument
> is valid before passing it to find_option().

The patch does not actually do this, it only changes the code inside
find_option().

> 
> Found by Svace static analysis tool.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@...il.com>
> ---
>  ethtool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 0513a1a..4250add 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -6395,6 +6395,9 @@ static int show_usage(struct cmd_context *ctx __maybe_unused)
>  
>  static int find_option(char *arg)
>  {
> +	if(!arg)

Missing space between "if" and "(".

> +		return -1;
> +		

You add whitespace on this empty line.

Michal

>  	const char *opt;
>  	size_t len;
>  	int k;
> -- 
> 2.39.2
> 

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