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]

Date: Mon, 8 May 2023 12:26:37 -0700
From: Jesse Brandeburg <jesse.brandeburg@...el.com>
To: Nicholas Vinson <nvinson234@...il.com>, <mkubeck@...e.cz>
CC: <netdev@...r.kernel.org>
Subject: Re: [PATCH ethtool] Fix argc and argp handling issues

On 5/8/2023 5:45 AM, 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

I'm happy to see someone else looking at this stuff!

So you're missign signed-off-by, please add it.

However when you resend, feel free to add my

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@...el.com>

and possibly:

Link:
https://patchwork.kernel.org/project/netdevbpf/patch/20221208011122.2343363-8-jesse.brandeburg@intel.com/



> ---
>  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);