netdev - Re: [PATCH ethtool v2 03/13] ethtool: disallow passing null to 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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Dec 2022 10:14:22 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ethtool v2 03/13] ethtool: disallow passing null to
 find_option

On Wed, Dec 07, 2022 at 05:11:12PM -0800, Jesse Brandeburg wrote:
> After testing with this code in the debugger, it is technically possible
> to pass a NULL argument to ethtool which then prods it to call strncmp
> with a NULL value, which triggers this warning:
> 
> Description: Null pointer passed to 1st parameter expecting 'nonnull'
> File: /git/ethtool/ethtool.c
> Line: 6129
> 
> Since segfaults are bad, let's just add a check for NULL when parsing
> the initial arguments. The other cases of a NULL option are seemingly
> handled.
> 
> Fixes: 127f80691f96 ("Move argument parsing to sub-command functions")
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>

Learning about (kernel) mainline commit dcd46d897adb ("exec: Force
single empty string when argv is empty"), I'm not opposed to mistrusting
kernel provided argc and argv[] but rather than hunting all places where
we might possibly hit a null pointer in argv[] (mind that we may iterate
over argv[] up to three times), I would simply check the whole array at
the beginning of main() with something like

	k = 0;
	while (k <= argc && argv[k])
		k++;
	if (k != argc) {
		fprintf(stderr, "ethtool: inconsistent command line (OS bug?)\n");
		return 1;
	}

and be done with it.

Michal

> ---
>  ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 3207e49137c4..a72577b32601 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -6389,7 +6389,7 @@ int main(int argc, char **argp)
>  	 * name to get settings for (which we don't expect to begin
>  	 * with '-').
>  	 */
> -	if (argc == 0)
> +	if (argc == 0 || *argp == NULL)
>  		exit_bad_args();
>  
>  	k = find_option(*argp);
> -- 
> 2.31.1
> 

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