netdev - Re: [PATCH] common: fix potential NULL dereference in print_rss

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]

Message-ID: <x27vanog6blm5ckllzbfe7lh4yevvernbjwwcotyq5rikpa6ac@p7akbzr4p23m>
Date: Wed, 11 Jun 2025 00:27:18 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: ant.v.moryakov@...il.com
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH] common: fix potential NULL dereference in
 print_rss_hkey()

Dne Sun, May 18, 2025 at 04:08:28PM GMT, ant.v.moryakov@...il.com napsal:
> From: AntonMoryakov <ant.v.moryakov@...il.com>
> 
> 
> Static analyzer (Svace) reported a possible null pointer dereference
> in print_rss_hkey(). Specifically, when the 'hkey' pointer is NULL,
> the function continues execution after printing an error message,
> leading to dereferencing hkey[i].
> 
> This patch adds an early return after the NULL check to prevent
> execution from continuing in such cases.
> 
> This resolves:
> DEREF_AFTER_NULL: common.c:209
> 
> Found by Svace static analysis tool.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@...il.com>
> 
> ---

For the record, I believe the null pointer dereference is not actually
possible with current ethtool code. As far as I can see, in the ioctl
path hkey always points inside a block returned by calloc() so that it
cannot actually be null (we bail out immediately if that calloc() call
fails). In the netlink path hkey can only be null if ETHTOOL_A_RSS_HKEY
attribute is not present in the message in which case hkey_size will be
zero and the for cycle body is not executed at all.

That being said, I'll accept the patch as a safety precaution in case
the calling code changes later. But I wanted to point this out for
future reference as this kind of commits tends to come back later and
haunt us in the form of CVEs.

Michal

>  common.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/common.c b/common.c
> index 1ba27e7..35ec36d 100644
> --- a/common.c
> +++ b/common.c
> @@ -233,8 +233,10 @@ void print_rss_hkey(u8 *hkey, u32 hkey_size)
>  	u32 i;
>  
>  	printf("RSS hash key:\n");
> -	if (!hkey_size || !hkey)
> +	if (!hkey_size || !hkey) {
>  		printf("Operation not supported\n");
> +		return;
> +	}
>  
>  	for (i = 0; i < hkey_size; i++) {
>  		if (i == (hkey_size - 1))
> -- 
> 2.34.1
> 

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