netdev - Re: [PATCH ethtool] Fix argc and argp handling 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]

Message-ID: <20230508200104.ktrzgazsn3t54n2a@lion.mk-sys.cz>
Date: Mon, 8 May 2023 22:01:04 +0200
From: Michal Kubecek <mkubecek@...e.cz>
To: Nicholas Vinson <nvinson234@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH ethtool] Fix argc and argp handling issues

On Mon, May 08, 2023 at 08:45:33AM -0400, Nicholas Vinson wrote:
> Fixes issues that were originally found using gcc's static analyzer. The
> flags used to invoke the analyzer are given below.
> 
> Upon manual review of the results and discussion of the previous patch
> '[PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.', it
> was determined that when using a kernel lacking the execve patch ( see
> https://github.com/gregkh/linux/commit/dcd46d897adb70d63e025f175a00a89797d31a43),
> it is possible for argc to be 0 and argp to be an array with only a
> single NULL entry. This scenario would cause ethtool to read beyond the
> bounds of the argp array. However, this scenario should not be possible
> for any Linux kernel released within the last two years should have the
> execve patch applied.
> 
>     CFLAGS=-march=native -O2 -pipe -fanalyzer       \
>         -Werror=analyzer-va-arg-type-mismatch       \
>         -Werror=analyzer-va-list-exhausted          \
>         -Werror=analyzer-va-list-leak               \
>         -Werror=analyzer-va-list-use-after-va-end
> 
>     CXXCFLAGS=-march=native -O2                     \
>         -pipe -fanalyzer                            \
>         -Werror=analyzer-va-arg-type-mismatch       \
>         -Werror=analyzer-va-list-exhausted          \
>         -Werror=analyzer-va-list-leak               \
>         -Werror=analyzer-va-list-use-after-va-end
> 
>     LDFLAGS="-Wl,-O1 -Wl,--as-needed"
> 
>     GCC version is gcc (Gentoo 13.1.0-r1 p1) 13.1.0

This looks good to me, except for the missing Signed-off-by (as
mentioned by Jesse). IMHO it's not necessary to resubmit the patch,
replying with the Signed-off-by line should suffice. If you can do that
by tomorrow, I'll include the patch in 6.3 release.

Michal

> ---
>  ethtool.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 98690df..0752fe4 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -6405,6 +6405,9 @@ int main(int argc, char **argp)
>  
>  	init_global_link_mode_masks();
>  
> +	if (argc < 2)
> +		exit_bad_args();
> +
>  	/* Skip command name */
>  	argp++;
>  	argc--;
> @@ -6449,7 +6452,7 @@ int main(int argc, char **argp)
>  	 * name to get settings for (which we don't expect to begin
>  	 * with '-').
>  	 */
> -	if (argc == 0)
> +	if (!*argp)
>  		exit_bad_args();
>  
>  	k = find_option(*argp);
> -- 
> 2.40.1
> 
> 

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