DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [RFC 0/5] more Coverity related bug fixes
@ 2018-11-06 21:48 Stephen Hemminger
  2018-11-06 21:48 ` [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path Stephen Hemminger
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Stephen Hemminger @ 2018-11-06 21:48 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

There are some real bugs in here, not just noise.
Labeling this version as RFC since compile tested only.

Stephen Hemminger (5):
  bus/pci: fix allocation of pci device path
  bus/pci: fix TOCTOU issue
  bpf: fix validation of eal_divmod
  eal/memory: avoid double munmap in error path
  pipeline: remove dead code

 drivers/bus/pci/linux/pci_uio.c          | 40 +++++++++++-------------
 lib/librte_bpf/bpf_validate.c            |  2 +-
 lib/librte_eal/linuxapp/eal/eal_memory.c |  1 +
 lib/librte_pipeline/rte_table_action.c   |  2 --
 4 files changed, 21 insertions(+), 24 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path
  2018-11-06 21:48 [dpdk-dev] [RFC 0/5] more Coverity related bug fixes Stephen Hemminger
@ 2018-11-06 21:48 ` Stephen Hemminger
  2018-11-18 15:03   ` Thomas Monjalon
                     ` (2 more replies)
  2018-11-06 21:48 ` [dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue Stephen Hemminger
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 20+ messages in thread
From: Stephen Hemminger @ 2018-11-06 21:48 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

The pci_resource_by_index called strlen() on uninitialized
memory which would lead to the wrong size of memory allocated
for the path portion of the resource map. This would either cause
excessively large allocation, or worse memory corruption.

Coverity Issue: 300868
Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/pci/linux/pci_uio.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index a7c14421aa79..112ac51dddcc 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -295,14 +295,6 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	loc = &dev->addr;
 	maps = uio_res->maps;
 
-	/* allocate memory to keep path */
-	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
-	if (maps[map_idx].path == NULL) {
-		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
-				strerror(errno));
-		return -1;
-	}
-
 	/*
 	 * open resource file, to mmap it
 	 */
@@ -335,10 +327,19 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 		if (fd < 0) {
 			RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
 				devname, strerror(errno));
-			goto error;
+			return -1;
 		}
 	}
 
+	/* allocate memory to keep path */
+	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+	if (maps[map_idx].path == NULL) {
+		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
+				strerror(errno));
+		close(fd);
+		return -1;
+	}
+
 	/* try mapping somewhere close to the end of hugepages */
 	if (pci_map_addr == NULL)
 		pci_map_addr = pci_find_max_end_va();
@@ -346,8 +347,10 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	mapaddr = pci_map_resource(pci_map_addr, fd, 0,
 			(size_t)dev->mem_resource[res_idx].len, 0);
 	close(fd);
-	if (mapaddr == MAP_FAILED)
-		goto error;
+	if (mapaddr == MAP_FAILED) {
+		rte_free(maps[map_idx].path);
+		return -1;
+	}
 
 	pci_map_addr = RTE_PTR_ADD(mapaddr,
 			(size_t)dev->mem_resource[res_idx].len);
@@ -360,10 +363,6 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	dev->mem_resource[res_idx].addr = mapaddr;
 
 	return 0;
-
-error:
-	rte_free(maps[map_idx].path);
-	return -1;
 }
 
 #if defined(RTE_ARCH_X86)
-- 
2.17.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue
  2018-11-06 21:48 [dpdk-dev] [RFC 0/5] more Coverity related bug fixes Stephen Hemminger
  2018-11-06 21:48 ` [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path Stephen Hemminger
@ 2018-11-06 21:48 ` Stephen Hemminger
  2018-11-18 15:04   ` Thomas Monjalon
  2018-11-06 21:48 ` [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod Stephen Hemminger
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Stephen Hemminger @ 2018-11-06 21:48 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

Using access followed by open causes a static analysis warning
about Time of check versus Time of use. Also, access() and
open() have different UID permission checks.

This is not a serious problem; but easy to fix by using errno instead.

Coverity issue: 300870
Fixes: 4a928ef9f611 ("bus/pci: enable write combining during mapping")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/bus/pci/linux/pci_uio.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 112ac51dddcc..8521fe63b0ae 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -306,12 +306,11 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 			loc->domain, loc->bus, loc->devid,
 			loc->function, res_idx);
 
-		if (access(devname, R_OK|W_OK) != -1) {
-			fd = open(devname, O_RDWR);
-			if (fd < 0)
-				RTE_LOG(INFO, EAL, "%s cannot be mapped. "
-					"Fall-back to non prefetchable mode.\n",
-					devname);
+		fd = open(devname, O_RDWR);
+		if (fd < 0 && errno != ENOENT) {
+			RTE_LOG(INFO, EAL, "%s cannot be mapped. "
+				"Fall-back to non prefetchable mode.\n",
+				devname);
 		}
 	}
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
  2018-11-06 21:48 [dpdk-dev] [RFC 0/5] more Coverity related bug fixes Stephen Hemminger
  2018-11-06 21:48 ` [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path Stephen Hemminger
  2018-11-06 21:48 ` [dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue Stephen Hemminger
@ 2018-11-06 21:48 ` Stephen Hemminger
  2018-11-07 12:54   ` Ananyev, Konstantin
  2018-11-06 21:49 ` [dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path Stephen Hemminger
  2018-11-06 21:49 ` [dpdk-dev] [RFC 5/5] pipeline: remove dead code Stephen Hemminger
  4 siblings, 1 reply; 20+ messages in thread
From: Stephen Hemminger @ 2018-11-06 21:48 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

Coverity spotted self assignment in BPF eval_divmod.
This looks like a bug where the incoming source register
should have been used instead.

Coverity issue: 302850
Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_bpf/bpf_validate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
index 83983efc4e5c..b768f72c4c02 100644
--- a/lib/librte_bpf/bpf_validate.c
+++ b/lib/librte_bpf/bpf_validate.c
@@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
 		if (op == BPF_MOD)
 			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
 		else
-			rd->u.max = rd->u.max;
+			rd->u.max = rs->u.max;
 		rd->u.min = 0;
 	}
 
-- 
2.17.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path
  2018-11-06 21:48 [dpdk-dev] [RFC 0/5] more Coverity related bug fixes Stephen Hemminger
                   ` (2 preceding siblings ...)
  2018-11-06 21:48 ` [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod Stephen Hemminger
@ 2018-11-06 21:49 ` Stephen Hemminger
  2018-11-06 23:10   ` Thomas Monjalon
  2018-11-06 21:49 ` [dpdk-dev] [RFC 5/5] pipeline: remove dead code Stephen Hemminger
  4 siblings, 1 reply; 20+ messages in thread
From: Stephen Hemminger @ 2018-11-06 21:49 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

Don't call munmap of hugepage memory twice in the error path.

Coverity issue: 325730
Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index c1b5e079117a..48b23ce19ad3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1617,6 +1617,7 @@ eal_legacy_hugepage_init(void)
 	tmp_hp = NULL;
 
 	munmap(hugepage, nr_hugefiles * sizeof(struct hugepage_file));
+	hugepage = NULL;
 
 	/* we're not going to allocate more pages, so release VA space for
 	 * unused memseg lists
-- 
2.17.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [RFC 5/5] pipeline: remove dead code
  2018-11-06 21:48 [dpdk-dev] [RFC 0/5] more Coverity related bug fixes Stephen Hemminger
                   ` (3 preceding siblings ...)
  2018-11-06 21:49 ` [dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path Stephen Hemminger
@ 2018-11-06 21:49 ` Stephen Hemminger
  2018-11-18 15:07   ` Thomas Monjalon
  4 siblings, 1 reply; 20+ messages in thread
From: Stephen Hemminger @ 2018-11-06 21:49 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger

Coverity detected dead code since pointer into a global
array can never be NULL.

Coverity issue: 323523
Fixes: 96303217a606 ("pipeline: add symmetric crypto table action")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_pipeline/rte_table_action.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_pipeline/rte_table_action.c b/lib/librte_pipeline/rte_table_action.c
index 537e6593e4a0..69ab41a7ab2d 100644
--- a/lib/librte_pipeline/rte_table_action.c
+++ b/lib/librte_pipeline/rte_table_action.c
@@ -1696,8 +1696,6 @@ get_block_size(const struct rte_crypto_sym_xform *xform, uint8_t cdev_id)
 
 	for (i = 0;; i++) {
 		cap = &dev_info.capabilities[i];
-		if (!cap)
-			break;
 
 		if (cap->sym.xform_type != xform->type)
 			continue;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path
  2018-11-06 21:49 ` [dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path Stephen Hemminger
@ 2018-11-06 23:10   ` Thomas Monjalon
  0 siblings, 0 replies; 20+ messages in thread
From: Thomas Monjalon @ 2018-11-06 23:10 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

06/11/2018 22:49, Stephen Hemminger:
> Don't call munmap of hugepage memory twice in the error path.
> 
> Coverity issue: 325730
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Thank you
Anatoly submitted the same fix:
	https://patches.dpdk.org/patch/47898/

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
  2018-11-06 21:48 ` [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod Stephen Hemminger
@ 2018-11-07 12:54   ` Ananyev, Konstantin
  2018-11-07 19:51     ` Stephen Hemminger
  0 siblings, 1 reply; 20+ messages in thread
From: Ananyev, Konstantin @ 2018-11-07 12:54 UTC (permalink / raw)
  To: Stephen Hemminger, dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Tuesday, November 6, 2018 9:49 PM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> 
> Coverity spotted self assignment in BPF eval_divmod.

Yep, there is one.
As I remember I have to add it because one of old versions 
of compiler (clang???) complained about 'variable being used uninitialized'.

> This looks like a bug where the incoming source register
> should have been used instead.

Nope, that's a wrong guess.
We shouldn't do it here.
Konstantin

> 
> Coverity issue: 302850
> Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_bpf/bpf_validate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> index 83983efc4e5c..b768f72c4c02 100644
> --- a/lib/librte_bpf/bpf_validate.c
> +++ b/lib/librte_bpf/bpf_validate.c
> @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
>  		if (op == BPF_MOD)
>  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
>  		else
> -			rd->u.max = rd->u.max;
> +			rd->u.max = rs->u.max;
>  		rd->u.min = 0;
>  	}
> 
> --
> 2.17.1

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
  2018-11-07 12:54   ` Ananyev, Konstantin
@ 2018-11-07 19:51     ` Stephen Hemminger
  2018-11-07 20:07       ` Ananyev, Konstantin
  2018-11-07 23:04       ` Ananyev, Konstantin
  0 siblings, 2 replies; 20+ messages in thread
From: Stephen Hemminger @ 2018-11-07 19:51 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: dev

On Wed, 7 Nov 2018 12:54:54 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Tuesday, November 6, 2018 9:49 PM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> > 
> > Coverity spotted self assignment in BPF eval_divmod.  
> 
> Yep, there is one.
> As I remember I have to add it because one of old versions 
> of compiler (clang???) complained about 'variable being used uninitialized'.
> 
> > This looks like a bug where the incoming source register
> > should have been used instead.  
> 
> Nope, that's a wrong guess.
> We shouldn't do it here.
> Konstantin
> 
> > 
> > Coverity issue: 302850
> > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_bpf/bpf_validate.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > index 83983efc4e5c..b768f72c4c02 100644
> > --- a/lib/librte_bpf/bpf_validate.c
> > +++ b/lib/librte_bpf/bpf_validate.c
> > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> >  		if (op == BPF_MOD)
> >  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> >  		else
> > -			rd->u.max = rd->u.max;
> > +			rd->u.max = rs->u.max;
> >  		rd->u.min = 0;
> >  	}
> > 
> > --
> > 2.17.1  
> 

Well it was being used unintialized, your trick of self assignment fooled clang
but did not fool Coverity.  What does the other BPF validator do?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
  2018-11-07 19:51     ` Stephen Hemminger
@ 2018-11-07 20:07       ` Ananyev, Konstantin
  2018-11-07 23:04       ` Ananyev, Konstantin
  1 sibling, 0 replies; 20+ messages in thread
From: Ananyev, Konstantin @ 2018-11-07 20:07 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev



> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, November 7, 2018 7:52 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> 
> On Wed, 7 Nov 2018 12:54:54 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Tuesday, November 6, 2018 9:49 PM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > > Subject: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
> > >
> > > Coverity spotted self assignment in BPF eval_divmod.
> >
> > Yep, there is one.
> > As I remember I have to add it because one of old versions
> > of compiler (clang???) complained about 'variable being used uninitialized'.
> >
> > > This looks like a bug where the incoming source register
> > > should have been used instead.
> >
> > Nope, that's a wrong guess.
> > We shouldn't do it here.
> > Konstantin
> >
> > >
> > > Coverity issue: 302850
> > > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > >  lib/librte_bpf/bpf_validate.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > > index 83983efc4e5c..b768f72c4c02 100644
> > > --- a/lib/librte_bpf/bpf_validate.c
> > > +++ b/lib/librte_bpf/bpf_validate.c
> > > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> > >  		if (op == BPF_MOD)
> > >  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> > >  		else
> > > -			rd->u.max = rd->u.max;
> > > +			rd->u.max = rs->u.max;
> > >  		rd->u.min = 0;
> > >  	}
> > >
> > > --
> > > 2.17.1
> >
> 
> Well it was being used unintialized, 

I don't think so, but if you can point to me where
exactly it is used uninitialized, we can discuss it further.

> your trick of self assignment fooled clang

It was one particular and pretty old version of clang
(if my memory serves me right).
With latest versions (let say 6.0) it doesn't complain,
if I remove that self-assignment.
gcc also doesn't see any problem here.
That makes me think it was a false-positive with old
version of the compiler.
Konstantin 

> but did not fool Coverity.  What does the other BPF validator do?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod
  2018-11-07 19:51     ` Stephen Hemminger
  2018-11-07 20:07       ` Ananyev, Konstantin
@ 2018-11-07 23:04       ` Ananyev, Konstantin
  1 sibling, 0 replies; 20+ messages in thread
From: Ananyev, Konstantin @ 2018-11-07 23:04 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

> > > >
> > > > Coverity spotted self assignment in BPF eval_divmod.
> > >
> > > Yep, there is one.
> > > As I remember I have to add it because one of old versions
> > > of compiler (clang???) complained about 'variable being used uninitialized'.
> > >
> > > > This looks like a bug where the incoming source register
> > > > should have been used instead.
> > >
> > > Nope, that's a wrong guess.
> > > We shouldn't do it here.
> > > Konstantin
> > >
> > > >
> > > > Coverity issue: 302850
> > > > Fixes: 8021917293d0 ("bpf: add extra validation for input BPF program")
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > ---
> > > >  lib/librte_bpf/bpf_validate.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/librte_bpf/bpf_validate.c b/lib/librte_bpf/bpf_validate.c
> > > > index 83983efc4e5c..b768f72c4c02 100644
> > > > --- a/lib/librte_bpf/bpf_validate.c
> > > > +++ b/lib/librte_bpf/bpf_validate.c
> > > > @@ -512,7 +512,7 @@ eval_divmod(uint32_t op, struct bpf_reg_val *rd, struct bpf_reg_val *rs,
> > > >  		if (op == BPF_MOD)
> > > >  			rd->u.max = RTE_MIN(rd->u.max, rs->u.max - 1);
> > > >  		else
> > > > -			rd->u.max = rd->u.max;
> > > > +			rd->u.max = rs->u.max;
> > > >  		rd->u.min = 0;
> > > >  	}
> > > >
> > > > --
> > > > 2.17.1
> > >
> >
> > Well it was being used unintialized,
> 
> I don't think so, but if you can point to me where
> exactly it is used uninitialized, we can discuss it further.
> 
> > your trick of self assignment fooled clang
> 
> It was one particular and pretty old version of clang
> (if my memory serves me right).
> With latest versions (let say 6.0) it doesn't complain,
> if I remove that self-assignment.
> gcc also doesn't see any problem here.
> That makes me think it was a false-positive with old
> version of the compiler.
> Konstantin

As a another thought - it wouldn't take much effort to 
send a patch with NOP self-assignment removed.
If it will pass our build-harness test, then it is probably
ok to integrate.
Konstantin

> 
> > but did not fool Coverity.  What does the other BPF validator do?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path
  2018-11-06 21:48 ` [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path Stephen Hemminger
@ 2018-11-18 15:03   ` Thomas Monjalon
  2018-11-22 23:52   ` Ferruh Yigit
  2018-11-23  0:29   ` [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI " Ferruh Yigit
  2 siblings, 0 replies; 20+ messages in thread
From: Thomas Monjalon @ 2018-11-18 15:03 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Ferruh Yigit

06/11/2018 22:48, Stephen Hemminger:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.
> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

This patch is RFC but Ferruh (UIO maintainer) was not Cc'ed.
I feel this bug is critical. Please advise.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue
  2018-11-06 21:48 ` [dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue Stephen Hemminger
@ 2018-11-18 15:04   ` Thomas Monjalon
  0 siblings, 0 replies; 20+ messages in thread
From: Thomas Monjalon @ 2018-11-18 15:04 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, ferruh.yigit

+Cc Ferruh

06/11/2018 22:48, Stephen Hemminger:
> Using access followed by open causes a static analysis warning
> about Time of check versus Time of use. Also, access() and
> open() have different UID permission checks.
> 
> This is not a serious problem; but easy to fix by using errno instead.
> 
> Coverity issue: 300870
> Fixes: 4a928ef9f611 ("bus/pci: enable write combining during mapping")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index 112ac51dddcc..8521fe63b0ae 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -306,12 +306,11 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  			loc->domain, loc->bus, loc->devid,
>  			loc->function, res_idx);
>  
> -		if (access(devname, R_OK|W_OK) != -1) {
> -			fd = open(devname, O_RDWR);
> -			if (fd < 0)
> -				RTE_LOG(INFO, EAL, "%s cannot be mapped. "
> -					"Fall-back to non prefetchable mode.\n",
> -					devname);
> +		fd = open(devname, O_RDWR);
> +		if (fd < 0 && errno != ENOENT) {
> +			RTE_LOG(INFO, EAL, "%s cannot be mapped. "
> +				"Fall-back to non prefetchable mode.\n",
> +				devname);
>  		}
>  	}

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 5/5] pipeline: remove dead code
  2018-11-06 21:49 ` [dpdk-dev] [RFC 5/5] pipeline: remove dead code Stephen Hemminger
@ 2018-11-18 15:07   ` Thomas Monjalon
  0 siblings, 0 replies; 20+ messages in thread
From: Thomas Monjalon @ 2018-11-18 15:07 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Cristian Dumitrescu

+Cc Cristian

06/11/2018 22:49, Stephen Hemminger:
> Coverity detected dead code since pointer into a global
> array can never be NULL.
> 
> Coverity issue: 323523
> Fixes: 96303217a606 ("pipeline: add symmetric crypto table action")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path
  2018-11-06 21:48 ` [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path Stephen Hemminger
  2018-11-18 15:03   ` Thomas Monjalon
@ 2018-11-22 23:52   ` Ferruh Yigit
  2018-11-23  0:29   ` [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI " Ferruh Yigit
  2 siblings, 0 replies; 20+ messages in thread
From: Ferruh Yigit @ 2018-11-22 23:52 UTC (permalink / raw)
  To: Stephen Hemminger, dev

On 11/6/2018 9:48 PM, Stephen Hemminger wrote:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.

Yes this may corrupt memory, I wonder how nobody hit this. I am for including
the fix for the release.

> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a7c14421aa79..112ac51dddcc 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -295,14 +295,6 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  	loc = &dev->addr;
>  	maps = uio_res->maps;
>  
> -	/* allocate memory to keep path */
> -	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> -	if (maps[map_idx].path == NULL) {
> -		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
> -				strerror(errno));
> -		return -1;
> -	}

What about simply:

-       maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+       maps[map_idx].path = rte_malloc(NULL, sizeof(devname) + 1, 0);

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path
  2018-11-06 21:48 ` [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path Stephen Hemminger
  2018-11-18 15:03   ` Thomas Monjalon
  2018-11-22 23:52   ` Ferruh Yigit
@ 2018-11-23  0:29   ` Ferruh Yigit
  2018-11-23 10:45     ` Thomas Monjalon
  2018-11-23 11:01     ` Maxime Coquelin
  2 siblings, 2 replies; 20+ messages in thread
From: Ferruh Yigit @ 2018-11-23  0:29 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, Thomas Monjalon, stable, Stephen Hemminger

The pci_resource_by_index called strlen() on uninitialized
memory which would lead to the wrong size of memory allocated
for the path portion of the resource map. This would either cause
excessively large allocation, or worse memory corruption.

Coverity Issue: 300868
Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/bus/pci/linux/pci_uio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index a7c14421a..09ecbb7aa 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -296,7 +296,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
 	maps = uio_res->maps;
 
 	/* allocate memory to keep path */
-	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+	maps[map_idx].path = rte_malloc(NULL, sizeof(devname), 0);
 	if (maps[map_idx].path == NULL) {
 		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
 				strerror(errno));
-- 
2.17.2

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path
  2018-11-23  0:29   ` [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI " Ferruh Yigit
@ 2018-11-23 10:45     ` Thomas Monjalon
  2018-11-23 10:55       ` Andrew Rybchenko
  2018-11-23 11:01     ` Maxime Coquelin
  1 sibling, 1 reply; 20+ messages in thread
From: Thomas Monjalon @ 2018-11-23 10:45 UTC (permalink / raw)
  To: dev; +Cc: Ferruh Yigit, stable, Stephen Hemminger, gaetan.rivet, arybchenko

Please, anyone for a review and a test?

23/11/2018 01:29, Ferruh Yigit:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.
> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  drivers/bus/pci/linux/pci_uio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a7c14421a..09ecbb7aa 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -296,7 +296,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>  	maps = uio_res->maps;
>  
>  	/* allocate memory to keep path */
> -	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> +	maps[map_idx].path = rte_malloc(NULL, sizeof(devname), 0);
>  	if (maps[map_idx].path == NULL) {
>  		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
>  				strerror(errno));
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path
  2018-11-23 10:45     ` Thomas Monjalon
@ 2018-11-23 10:55       ` Andrew Rybchenko
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Rybchenko @ 2018-11-23 10:55 UTC (permalink / raw)
  To: Thomas Monjalon, dev
  Cc: Ferruh Yigit, stable, Stephen Hemminger, gaetan.rivet

On 11/23/18 1:45 PM, Thomas Monjalon wrote:
> Please, anyone for a review and a test?
>
> 23/11/2018 01:29, Ferruh Yigit:
>> The pci_resource_by_index called strlen() on uninitialized
>> memory which would lead to the wrong size of memory allocated
>> for the path portion of the resource map. This would either cause
>> excessively large allocation, or worse memory corruption.
>>
>> Coverity Issue: 300868
>> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path
  2018-11-23  0:29   ` [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI " Ferruh Yigit
  2018-11-23 10:45     ` Thomas Monjalon
@ 2018-11-23 11:01     ` Maxime Coquelin
  2018-11-25 10:53       ` Thomas Monjalon
  1 sibling, 1 reply; 20+ messages in thread
From: Maxime Coquelin @ 2018-11-23 11:01 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: Thomas Monjalon, stable, Stephen Hemminger



On 11/23/18 1:29 AM, Ferruh Yigit wrote:
> The pci_resource_by_index called strlen() on uninitialized
> memory which would lead to the wrong size of memory allocated
> for the path portion of the resource map. This would either cause
> excessively large allocation, or worse memory corruption.
> 
> Coverity Issue: 300868
> Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>   drivers/bus/pci/linux/pci_uio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a7c14421a..09ecbb7aa 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -296,7 +296,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx,
>   	maps = uio_res->maps;
>   
>   	/* allocate memory to keep path */
> -	maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> +	maps[map_idx].path = rte_malloc(NULL, sizeof(devname), 0);
>   	if (maps[map_idx].path == NULL) {
>   		RTE_LOG(ERR, EAL, "Cannot allocate memory for path: %s\n",
>   				strerror(errno));
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI device path
  2018-11-23 11:01     ` Maxime Coquelin
@ 2018-11-25 10:53       ` Thomas Monjalon
  0 siblings, 0 replies; 20+ messages in thread
From: Thomas Monjalon @ 2018-11-25 10:53 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Maxime Coquelin, stable, Stephen Hemminger

23/11/2018 12:01, Maxime Coquelin:
> On 11/23/18 1:29 AM, Ferruh Yigit wrote:
> > The pci_resource_by_index called strlen() on uninitialized
> > memory which would lead to the wrong size of memory allocated
> > for the path portion of the resource map. This would either cause
> > excessively large allocation, or worse memory corruption.
> > 
> > Coverity Issue: 300868
> > Fixes: ea9d56226e72 ("pci: introduce function to map uio resource by index")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied, thanks

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2018-11-25 10:53 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-06 21:48 [dpdk-dev] [RFC 0/5] more Coverity related bug fixes Stephen Hemminger
2018-11-06 21:48 ` [dpdk-dev] [RFC 1/5] bus/pci: fix allocation of pci device path Stephen Hemminger
2018-11-18 15:03   ` Thomas Monjalon
2018-11-22 23:52   ` Ferruh Yigit
2018-11-23  0:29   ` [dpdk-dev] [PATCH] bus/pci: fix allocation of PCI " Ferruh Yigit
2018-11-23 10:45     ` Thomas Monjalon
2018-11-23 10:55       ` Andrew Rybchenko
2018-11-23 11:01     ` Maxime Coquelin
2018-11-25 10:53       ` Thomas Monjalon
2018-11-06 21:48 ` [dpdk-dev] [RFC 2/5] bus/pci: fix TOCTOU issue Stephen Hemminger
2018-11-18 15:04   ` Thomas Monjalon
2018-11-06 21:48 ` [dpdk-dev] [RFC 3/5] bpf: fix validation of eal_divmod Stephen Hemminger
2018-11-07 12:54   ` Ananyev, Konstantin
2018-11-07 19:51     ` Stephen Hemminger
2018-11-07 20:07       ` Ananyev, Konstantin
2018-11-07 23:04       ` Ananyev, Konstantin
2018-11-06 21:49 ` [dpdk-dev] [RFC 4/5] eal/memory: avoid double munmap in error path Stephen Hemminger
2018-11-06 23:10   ` Thomas Monjalon
2018-11-06 21:49 ` [dpdk-dev] [RFC 5/5] pipeline: remove dead code Stephen Hemminger
2018-11-18 15:07   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).