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] [day] [month] [year] [list]

Message-ID: <551ba931-3f59-6f66-93b5-b6aeeaa595ff@gmail.com>
Date: Mon, 8 May 2023 16:21:51 -0400
From: Nicholas Vinson <nvinson234@...il.com>
To: Michal Kubecek <mkubecek@...e.cz>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH ethtool] Fix argc and argp handling issues


On 5/8/23 16:01, Michal Kubecek wrote:
> 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

I'll send an updated version momentarily. I noticed some spelling 
mistakes that I'd like to fix.

Thanks,

Nicholas Vinson

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