[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b245a442-bfba-aa7e-0437-928090ca1f26@intel.com>
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);
Powered by blists - more mailing lists