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

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

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

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

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

From: Phil Sutter

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

	David

On Fri, Aug 18, 2017 at 09:21:34AM +0000, David Laight wrote:

From: Phil Sutter

quoted

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

On Thu, 17 Aug 2017 19:09:29 +0200
Phil Sutter [off-list ref] 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.