[iproute PATCH v2 0/3] Covscan: Fixes for obvious programming mistakes

[iproute PATCH v2 0/3] Covscan: Fixes for obvious programming mistakes

[Phil Sutter]

This series collects those patches from v1 which are clear programming
flaws.

No changes to the actual patches, just splitting into smaller series.

Phil Sutter (3):
  iproute_lwtunnel: csum_mode value checking was ineffective
  iproute_lwtunnel: Argument to strerror must be positive
  tipc/node: Fix socket fd check in cmd_node_get_addr()

 ip/iproute_lwtunnel.c | 9 +++++----
 tipc/node.c           | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.13.1

[iproute PATCH v2 1/3] iproute_lwtunnel: csum_mode value checking was ineffective

[Phil Sutter]

ila_csum_name2mode() returning -1 on error but being declared as
returning __u8 doesn't make much sense. Change the code to correctly
detect this issue. Checking for __u8 overruns shouldn't be necessary
though since ila_csum_name2mode() return values are well-defined.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iproute_lwtunnel.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
index 5c0c7d110d23e..398ab5e077ed8 100644
--- a/ip/iproute_lwtunnel.c
+++ b/ip/iproute_lwtunnel.c
@@ -171,7 +171,7 @@ static char *ila_csum_mode2name(__u8 csum_mode)
 	}
 }
 
-static __u8 ila_csum_name2mode(char *name)
+static int ila_csum_name2mode(char *name)
 {
 	if (strcmp(name, "adj-transport") == 0)
 		return ILA_CSUM_ADJUST_TRANSPORT;
@@ -517,7 +517,7 @@ static int parse_encap_ila(struct rtattr *rta, size_t len,
 
 	while (argc > 0) {
 		if (strcmp(*argv, "csum-mode") == 0) {
-			__u8 csum_mode;
+			int csum_mode;
 
 			NEXT_ARG();
 
@@ -526,7 +526,8 @@ static int parse_encap_ila(struct rtattr *rta, size_t len,
 				invarg("\"csum-mode\" value is invalid\n",
 				       *argv);
 
-			rta_addattr8(rta, 1024, ILA_ATTR_CSUM_MODE, csum_mode);
+			rta_addattr8(rta, 1024, ILA_ATTR_CSUM_MODE,
+				     (__u8)csum_mode);
 
 			argc--; argv++;
 		} else {
-- 
2.13.1

[iproute PATCH v2 2/3] iproute_lwtunnel: Argument to strerror must be positive

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iproute_lwtunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
index 398ab5e077ed8..1a3dc4d4c0ed9 100644
--- a/ip/iproute_lwtunnel.c
+++ b/ip/iproute_lwtunnel.c
@@ -643,7 +643,7 @@ static int lwt_parse_bpf(struct rtattr *rta, size_t len,
 	err = bpf_parse_common(bpf_type, &cfg, &bpf_cb_ops, &x);
 	if (err < 0) {
 		fprintf(stderr, "Failed to parse eBPF program: %s\n",
-			strerror(err));
+			strerror(-err));
 		return -1;
 	}
 	rta_nest_end(rta, nest);
-- 
2.13.1

RE: [iproute PATCH v2 2/3] iproute_lwtunnel: Argument to strerror must be positive

[David Laight]

From: Phil Sutter
> Sent: 17 August 2017 18:10
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/iproute_lwtunnel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
> index 398ab5e077ed8..1a3dc4d4c0ed9 100644
> --- a/ip/iproute_lwtunnel.c
> +++ b/ip/iproute_lwtunnel.c
> @@ -643,7 +643,7 @@ static int lwt_parse_bpf(struct rtattr *rta, size_t len,
>  	err = bpf_parse_common(bpf_type, &cfg, &bpf_cb_ops, &x);
>  	if (err < 0) {
>  		fprintf(stderr, "Failed to parse eBPF program: %s\n",
> -			strerror(err));
> +			strerror(-err));

If we are in userspace I'd expect errno values to be +ve.
Returning a -ve errno is very non-standard.

	David

Re: [iproute PATCH v2 2/3] iproute_lwtunnel: Argument to strerror must be positive

[Phil Sutter]

On Fri, Aug 18, 2017 at 09:21:34AM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 17 August 2017 18:10
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  ip/iproute_lwtunnel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c
> > index 398ab5e077ed8..1a3dc4d4c0ed9 100644
> > --- a/ip/iproute_lwtunnel.c
> > +++ b/ip/iproute_lwtunnel.c
> > @@ -643,7 +643,7 @@ static int lwt_parse_bpf(struct rtattr *rta, size_t len,
> >  	err = bpf_parse_common(bpf_type, &cfg, &bpf_cb_ops, &x);
> >  	if (err < 0) {
> >  		fprintf(stderr, "Failed to parse eBPF program: %s\n",
> > -			strerror(err));
> > +			strerror(-err));
> 
> If we are in userspace I'd expect errno values to be +ve.
> Returning a -ve errno is very non-standard.

This is because bpf_parse() returns the number of instructions parsed or
a negative return code. We could change it to return instructions * -1
or a positive return code, but that's even more insane than calling
strerror(-err), isn't it?

Cheers, Phil

[iproute PATCH v2 3/3] tipc/node: Fix socket fd check in cmd_node_get_addr()

[Phil Sutter]

socket() returns -1 on error, not 0.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tipc/node.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tipc/node.c b/tipc/node.c
index 201fe1a4df3bd..fe085aec9b4ac 100644
--- a/tipc/node.c
+++ b/tipc/node.c
@@ -109,7 +109,8 @@ static int cmd_node_get_addr(struct nlmsghdr *nlh, const struct cmd *cmd,
 	socklen_t sz = sizeof(struct sockaddr_tipc);
 	struct sockaddr_tipc addr;
 
-	if (!(sk = socket(AF_TIPC, SOCK_RDM, 0))) {
+	sk = socket(AF_TIPC, SOCK_RDM, 0);
+	if (sk < 0) {
 		fprintf(stderr, "opening TIPC socket: %s\n", strerror(errno));
 		return -1;
 	}
-- 
2.13.1

Re: [iproute PATCH v2 0/3] Covscan: Fixes for obvious programming mistakes

[Stephen Hemminger]

On Thu, 17 Aug 2017 19:09:29 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This series collects those patches from v1 which are clear programming
> flaws.
> 
> No changes to the actual patches, just splitting into smaller series.
> 
> Phil Sutter (3):
>   iproute_lwtunnel: csum_mode value checking was ineffective
>   iproute_lwtunnel: Argument to strerror must be positive
>   tipc/node: Fix socket fd check in cmd_node_get_addr()
> 
>  ip/iproute_lwtunnel.c | 9 +++++----
>  tipc/node.c           | 3 ++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 

Accepted.