[PATCH ethtool 0/3] Fix issues found by gcc -fanalyze

[PATCH ethtool 0/3] Fix issues found by gcc -fanalyze

[Nicholas Vinson]

This patch series provides updates to correct issues found by gcc -fanalyze. The
issues were found by specifying the following flags when building:

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

Nicholas Vinson (3):
  Update FAM syntax to conform to std C.
  Fix reported memory leak.
  Fix potentinal null-pointer derference issues.

 ethtool.c          | 24 +++++++++++++-----------
 netlink/features.c | 24 ++++++++++++++++++------
 2 files changed, 31 insertions(+), 17 deletions(-)

-- 
2.40.1

[PATCH ethtool 1/3] Update FAM syntax to conform to std C.

[Nicholas Vinson]

Found via gcc -fanalyzer. When using the non-standard FAM syntax:

    uint32_t req_mask[0];

gcc-13 with the -fanalyzer flag generates an internal compiler error.
Updating the syntax to use the standard C syntax:

    uint32_t req_mask[];

works around the gcc bug.

Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>
---
 netlink/features.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/netlink/features.c b/netlink/features.c
index a4dae8f..a93f3e7 100644
--- a/netlink/features.c
+++ b/netlink/features.c
@@ -266,7 +266,7 @@ int nl_gfeatures(struct cmd_context *ctx)
 
 struct sfeatures_context {
 	bool			nothing_changed;
-	uint32_t		req_mask[0];
+	uint32_t		req_mask[];
 };
 
 static int find_feature(const char *name,
-- 
2.40.1

[PATCH ethtool 2/3] Fix reported memory leak.

[Nicholas Vinson]

Found via gcc -fanalyzer. In the function nl_sfeatures() malloc() is
called to allocate a block of memory; however, that memory block is
never explictily freed.

Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>
---
 netlink/features.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/netlink/features.c b/netlink/features.c
index a93f3e7..5711ff4 100644
--- a/netlink/features.c
+++ b/netlink/features.c
@@ -534,24 +534,36 @@ int nl_sfeatures(struct cmd_context *ctx)
 	nlctx->devname = ctx->devname;
 	ret = msg_init(nlctx, msgbuff, ETHTOOL_MSG_FEATURES_SET,
 		       NLM_F_REQUEST | NLM_F_ACK);
-	if (ret < 0)
+	if (ret < 0) {
+		free(sfctx);
 		return 2;
+	}
 	if (ethnla_fill_header(msgbuff, ETHTOOL_A_FEATURES_HEADER, ctx->devname,
-			       ETHTOOL_FLAG_COMPACT_BITSETS))
+			       ETHTOOL_FLAG_COMPACT_BITSETS)) {
+		free(sfctx);
 		return -EMSGSIZE;
+	}
 	ret = fill_sfeatures_bitmap(nlctx, feature_names);
-	if (ret < 0)
+	if (ret < 0) {
+		free(sfctx);
 		return ret;
+	}
 
 	ret = nlsock_sendmsg(nlsk, NULL);
-	if (ret < 0)
+	if (ret < 0) {
+		free(sfctx);
 		return 92;
+	}
 	ret = nlsock_process_reply(nlsk, sfeatures_reply_cb, nlctx);
 	if (sfctx->nothing_changed) {
 		fprintf(stderr, "Could not change any device features\n");
+		free(sfctx);
 		return nlctx->exit_code ?: 1;
 	}
-	if (ret == 0)
+	if (ret == 0) {
+		free(sfctx);
 		return 0;
+	}
+	free(sfctx);
 	return nlctx->exit_code ?: 92;
 }
-- 
2.40.1

[PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.

[Nicholas Vinson]

Found via gcc -fanalyzer. Analyzer claims that it's possible certain
functions may receive a NULL pointer when handling CLI arguments. Adding
NULL pointer checks to correct the issues.

Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>
---
 ethtool.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 98690df..4ec1e23 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -6182,16 +6182,18 @@ static int find_option(char *arg)
 	size_t len;
 	int k;
 
-	for (k = 1; args[k].opts; k++) {
-		opt = args[k].opts;
-		for (;;) {
-			len = strcspn(opt, "|");
-			if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
-				return k;
-
-			if (opt[len] == 0)
-				break;
-			opt += len + 1;
+	if (arg) {
+		for (k = 1; args[k].opts; k++) {
+			opt = args[k].opts;
+			for (;;) {
+				len = strcspn(opt, "|");
+				if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
+					return k;
+
+				if (opt[len] == 0)
+					break;
+				opt += len + 1;
+			}
 		}
 	}
 
@@ -6457,7 +6459,7 @@ int main(int argc, char **argp)
 		argp++;
 		argc--;
 	} else {
-		if ((*argp)[0] == '-')
+		if (!*argp || (*argp)[0] == '-')
 			exit_bad_args();
 		k = 0;
 	}
-- 
2.40.1

Re: [PATCH ethtool 0/3] Fix issues found by gcc -fanalyze

[patchwork-bot+netdevbpf]

Hello:

This series was applied to ethtool/ethtool.git (master)
by Michal Kubecek <mkubecek@suse.cz>:

On Sun, 30 Apr 2023 18:50:49 -0400 you wrote:
> This patch series provides updates to correct issues found by gcc -fanalyze. The
> issues were found by specifying the following flags when building:
> 
> 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"
> 
> [...]

Here is the summary with links:
  - [ethtool,1/3] Update FAM syntax to conform to std C.
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=77599cf04110
  - [ethtool,2/3] Fix reported memory leak.
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=7de97fb99868
  - [ethtool,3/3] Fix potentinal null-pointer derference issues.
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]

On Sun, Apr 30, 2023 at 06:50:52PM -0400, Nicholas Vinson wrote:
> Found via gcc -fanalyzer. Analyzer claims that it's possible certain
> functions may receive a NULL pointer when handling CLI arguments. Adding
> NULL pointer checks to correct the issues.
> 
> Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>

A similar theoretical issue was discussed recently:

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

My position is still the same: argv[] members cannot be actually null
unless there is a serious kernel bug (see the link above for an
explanation). I'm not opposed to doing a sanity check just in case but
if we do, I believe we should check the whole argv[] array right at the
beginning and be done with it rather than add specific checks to random
places inside parser code.

> @@ -6182,16 +6182,18 @@ static int find_option(char *arg)
>  	size_t len;
>  	int k;
>  
> -	for (k = 1; args[k].opts; k++) {
> -		opt = args[k].opts;
> -		for (;;) {
> -			len = strcspn(opt, "|");
> -			if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
> -				return k;
> -
> -			if (opt[len] == 0)
> -				break;
> -			opt += len + 1;
> +	if (arg) {
> +		for (k = 1; args[k].opts; k++) {
> +			opt = args[k].opts;
> +			for (;;) {
> +				len = strcspn(opt, "|");
> +				if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
> +					return k;
> +
> +				if (opt[len] == 0)
> +					break;
> +				opt += len + 1;
> +			}
>  		}
>  	}
>  

I would rather prefer a simple

	if (!arg)
		return -1;

to avoid closing almost all of the function body inside an if block.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.

[Nicholas Vinson]

On 5/7/23 18:57, Michal Kubecek wrote:
> On Sun, Apr 30, 2023 at 06:50:52PM -0400, Nicholas Vinson wrote:
>> Found via gcc -fanalyzer. Analyzer claims that it's possible certain
>> functions may receive a NULL pointer when handling CLI arguments. Adding
>> NULL pointer checks to correct the issues.
>>
>> Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>
> A similar theoretical issue was discussed recently:
>
>    https://patchwork.kernel.org/project/netdevbpf/patch/20221208011122.2343363-8-jesse.brandeburg@intel.com/
>
> My position is still the same: argv[] members cannot be actually null
> unless there is a serious kernel bug (see the link above for an
> explanation). I'm not opposed to doing a sanity check just in case but
> if we do, I believe we should check the whole argv[] array right at the
> beginning and be done with it rather than add specific checks to random
> places inside parser code.

By convention and POSIX standard, the last argv[] member is always set 
to NULL and is accessed from main(int argc, char **argp) via argp[argc] 
(see 
https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html). 
It's also possible for argc to be zero. In such a case, 
find_option(NULL) would get called.

However, after reviewing main(), I recommend changing:

         if (argc == 0)

                 exit_bad_args();

to

         if (argc <= 0 || !*argp)

                 exit_bad_args();


as this fixes the potential issue of main()'s argc being 0 (argc would 
be -1 at this point in such cases), and "!*argp" silences gcc's built-in 
analyzer (and should silence all other SA with respect to the reported 
issue) as the SA doesn't recognize that it would take a buggy execve 
implementation to allow argp to be NULL at this point ).

If you don't have any objections to this change, I can draft an updated 
patch to make this change.

Thanks,

Nicholas Vinson

>> @@ -6182,16 +6182,18 @@ static int find_option(char *arg)
>>   	size_t len;
>>   	int k;
>>   
>> -	for (k = 1; args[k].opts; k++) {
>> -		opt = args[k].opts;
>> -		for (;;) {
>> -			len = strcspn(opt, "|");
>> -			if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
>> -				return k;
>> -
>> -			if (opt[len] == 0)
>> -				break;
>> -			opt += len + 1;
>> +	if (arg) {
>> +		for (k = 1; args[k].opts; k++) {
>> +			opt = args[k].opts;
>> +			for (;;) {
>> +				len = strcspn(opt, "|");
>> +				if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
>> +					return k;
>> +
>> +				if (opt[len] == 0)
>> +					break;
>> +				opt += len + 1;
>> +			}
>>   		}
>>   	}
>>   
> I would rather prefer a simple
>
> 	if (!arg)
> 		return -1;
>
> to avoid closing almost all of the function body inside an if block.
>
> Michal

Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

On Sun, May 07, 2023 at 10:46:05PM -0400, Nicholas Vinson wrote:
> 
> On 5/7/23 18:57, Michal Kubecek wrote:
> > On Sun, Apr 30, 2023 at 06:50:52PM -0400, Nicholas Vinson wrote:
> > > Found via gcc -fanalyzer. Analyzer claims that it's possible certain
> > > functions may receive a NULL pointer when handling CLI arguments. Adding
> > > NULL pointer checks to correct the issues.
> > > 
> > > Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>
> > A similar theoretical issue was discussed recently:
> > 
> >    https://patchwork.kernel.org/project/netdevbpf/patch/20221208011122.2343363-8-jesse.brandeburg@intel.com/
> > 
> > My position is still the same: argv[] members cannot be actually null
> > unless there is a serious kernel bug (see the link above for an
> > explanation). I'm not opposed to doing a sanity check just in case but
> > if we do, I believe we should check the whole argv[] array right at the
> > beginning and be done with it rather than add specific checks to random
> > places inside parser code.
> 
> By convention and POSIX standard, the last argv[] member is always set to
> NULL and is accessed from main(int argc, char **argp) via argp[argc] (see
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html).
> It's also possible for argc to be zero. In such a case, find_option(NULL)
> would get called.

Please note that ethtool is not a utility for a general POSIX system.
It is a very specific utility which works and makes sense only on Linux.
That's why it can and does take many assumptions which are only
guaranteed on Linux but may not be true on other POSIX systems.

> However, after reviewing main(), I recommend changing:
> 
>         if (argc == 0)
> 
>                 exit_bad_args();
> 
> to
> 
>         if (argc <= 0 || !*argp)
> 
>                 exit_bad_args();
> 
> 
> as this fixes the potential issue of main()'s argc being 0 (argc would be -1
> at this point in such cases), and "!*argp" silences gcc's built-in analyzer
> (and should silence all other SA with respect to the reported issue) as the
> SA doesn't recognize that it would take a buggy execve implementation to
> allow argp to be NULL at this point ).
> 
> If you don't have any objections to this change, I can draft an updated
> patch to make this change.

As I said before, I do not object to adding a sanity check of argc/argv,
I only objected to adding random checks inside the parser code just to
make static checkers happy. If you want to add (or strengthen) a sanity
check right at the beginning of the program, before any parsing, I have
no problem with that.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ethtool 3/3] Fix potentinal null-pointer derference issues.

[Nicholas Vinson]

On 5/8/23 03:40, Michal Kubecek wrote:
> On Sun, May 07, 2023 at 10:46:05PM -0400, Nicholas Vinson wrote:
>> On 5/7/23 18:57, Michal Kubecek wrote:
>>> On Sun, Apr 30, 2023 at 06:50:52PM -0400, Nicholas Vinson wrote:
>>>> Found via gcc -fanalyzer. Analyzer claims that it's possible certain
>>>> functions may receive a NULL pointer when handling CLI arguments. Adding
>>>> NULL pointer checks to correct the issues.
>>>>
>>>> Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>
>>> A similar theoretical issue was discussed recently:
>>>
>>>     https://patchwork.kernel.org/project/netdevbpf/patch/20221208011122.2343363-8-jesse.brandeburg@intel.com/
>>>
>>> My position is still the same: argv[] members cannot be actually null
>>> unless there is a serious kernel bug (see the link above for an
>>> explanation). I'm not opposed to doing a sanity check just in case but
>>> if we do, I believe we should check the whole argv[] array right at the
>>> beginning and be done with it rather than add specific checks to random
>>> places inside parser code.
>> By convention and POSIX standard, the last argv[] member is always set to
>> NULL and is accessed from main(int argc, char **argp) via argp[argc] (see
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html).
>> It's also possible for argc to be zero. In such a case, find_option(NULL)
>> would get called.
> Please note that ethtool is not a utility for a general POSIX system.
> It is a very specific utility which works and makes sense only on Linux.
> That's why it can and does take many assumptions which are only
> guaranteed on Linux but may not be true on other POSIX systems.
>
>> However, after reviewing main(), I recommend changing:
>>
>>          if (argc == 0)
>>
>>                  exit_bad_args();
>>
>> to
>>
>>          if (argc <= 0 || !*argp)
>>
>>                  exit_bad_args();
>>
>>
>> as this fixes the potential issue of main()'s argc being 0 (argc would be -1
>> at this point in such cases), and "!*argp" silences gcc's built-in analyzer
>> (and should silence all other SA with respect to the reported issue) as the
>> SA doesn't recognize that it would take a buggy execve implementation to
>> allow argp to be NULL at this point ).
>>
>> If you don't have any objections to this change, I can draft an updated
>> patch to make this change.
> As I said before, I do not object to adding a sanity check of argc/argv,
> I only objected to adding random checks inside the parser code just to
> make static checkers happy. If you want to add (or strengthen) a sanity
> check right at the beginning of the program, before any parsing, I have
> no problem with that.

I am abandoning this patch in favor of a new one with the subject line 
"[PATCH ethtool] Fix argc and argp handling issues".

Thanks,

Nicholas Vinson


> Michal