[dpdk-dev] xstats: fix behavior when a null array is provided

Message ID 1459784718-22856-1-git-send-email-olivier.matz@6wind.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Olivier Matz April 4, 2016, 3:45 p.m. UTC
  Coverity reports an issue in ethdev:

  *** CID 124562:  Null pointer dereferences  (FORWARD_NULL)
  /lib/librte_ether/rte_ethdev.c: 1518 in rte_eth_xstats_get()
  1512
  1513		/* global stats */
  1514     	for (i = 0; i < RTE_NB_STATS; i++) {
  1515     	    stats_ptr = RTE_PTR_ADD(&eth_stats,
  1516
  rte_stats_strings[i].offset);
  1517			val = *stats_ptr;
  >>>     CID 124562:  Null pointer dereferences  (FORWARD_NULL)
  >>>     Dereferencing null pointer "xstats".
  1518     	      	   snprintf(xstats[count].name,
  sizeof(xstats[count].name),
  1519				"%s", rte_stats_strings[i].name);
  1520     			      xstats[count++].value = val;
  1521     			      }
  1522
  1523		/* per-rxq stats */

If a user calls rte_eth_xstats_get(portid, NULL, n) with n != 0,
it may result in a crash. Although the API documentation says that
n is the size of the table and xstats can be NULL if n == 0, we
can add an additional check here to make Coverity happy.

In that case, the return value is the same than when n == 0 is
passed, it returns the number of statistics.

Fixes: ce757f5c9a ("ethdev: new method to retrieve extended statistics")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Van Haaren, Harry April 5, 2016, 11:06 a.m. UTC | #1
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Subject: [dpdk-dev] [PATCH] xstats: fix behavior when a null array is provided
> 
> Coverity reports an issue in ethdev:
> 
>   *** CID 124562:  Null pointer dereferences  (FORWARD_NULL)
>   /lib/librte_ether/rte_ethdev.c: 1518 in rte_eth_xstats_get()
>   1512
>   1513		/* global stats */
>   1514     	for (i = 0; i < RTE_NB_STATS; i++) {
>   1515     	    stats_ptr = RTE_PTR_ADD(&eth_stats,
>   1516
>   rte_stats_strings[i].offset);
>   1517			val = *stats_ptr;
>   >>>     CID 124562:  Null pointer dereferences  (FORWARD_NULL)
>   >>>     Dereferencing null pointer "xstats".
>   1518     	      	   snprintf(xstats[count].name,
>   sizeof(xstats[count].name),
>   1519				"%s", rte_stats_strings[i].name);
>   1520     			      xstats[count++].value = val;
>   1521     			      }
>   1522
>   1523		/* per-rxq stats */
> 
> If a user calls rte_eth_xstats_get(portid, NULL, n) with n != 0,
> it may result in a crash. Although the API documentation says that
> n is the size of the table and xstats can be NULL if n == 0, we
> can add an additional check here to make Coverity happy.
> 
> In that case, the return value is the same than when n == 0 is
> passed, it returns the number of statistics.
> 
> Fixes: ce757f5c9a ("ethdev: new method to retrieve extended statistics")
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

I'm unsure on how verbose commit messages are ideal,
but there's certainly enough description here :)

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
  
Thomas Monjalon April 6, 2016, 10:13 a.m. UTC | #2
2016-04-05 11:06, Van Haaren, Harry:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> > Subject: [dpdk-dev] [PATCH] xstats: fix behavior when a null array is provided
> > 
> > Coverity reports an issue in ethdev:
> > 
> >   *** CID 124562:  Null pointer dereferences  (FORWARD_NULL)
> >   /lib/librte_ether/rte_ethdev.c: 1518 in rte_eth_xstats_get()
> >   1512
> >   1513		/* global stats */
> >   1514     	for (i = 0; i < RTE_NB_STATS; i++) {
> >   1515     	    stats_ptr = RTE_PTR_ADD(&eth_stats,
> >   1516
> >   rte_stats_strings[i].offset);
> >   1517			val = *stats_ptr;
> >   >>>     CID 124562:  Null pointer dereferences  (FORWARD_NULL)
> >   >>>     Dereferencing null pointer "xstats".
> >   1518     	      	   snprintf(xstats[count].name,
> >   sizeof(xstats[count].name),
> >   1519				"%s", rte_stats_strings[i].name);
> >   1520     			      xstats[count++].value = val;
> >   1521     			      }
> >   1522
> >   1523		/* per-rxq stats */
> > 
> > If a user calls rte_eth_xstats_get(portid, NULL, n) with n != 0,
> > it may result in a crash. Although the API documentation says that
> > n is the size of the table and xstats can be NULL if n == 0, we
> > can add an additional check here to make Coverity happy.
> > 
> > In that case, the return value is the same than when n == 0 is
> > passed, it returns the number of statistics.
> > 
> > Fixes: ce757f5c9a ("ethdev: new method to retrieve extended statistics")
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> I'm unsure on how verbose commit messages are ideal,
> but there's certainly enough description here :)
> 
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Applied, thanks
  

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 76a30fd..60d2573 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1503,7 +1503,7 @@  rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats,
 			return xcount;
 	}
 
-	if (n < count + xcount)
+	if (n < count + xcount || xstats == NULL)
 		return count + xcount;
 
 	/* now fill the xstats structure */