[PATCH dpdk 0/2] IPv6: Fix coverity issues

[PATCH dpdk 0/2] IPv6: Fix coverity issues

[Robin Jarry]

Hi all,

Here are fixes for three coverity issues:

/lib/net/rte_ip6.h: 91 in rte_ipv6_addr_mask()
*** CID 446754:  Memory - illegal accesses  (OVERRUN)
85     {
86             if (depth < RTE_IPV6_MAX_DEPTH) {
87                     uint8_t d = depth / 8;
88                     uint8_t mask = ~(UINT8_MAX >> (depth % 8));
89                     ip->a[d] &= mask;
90                     d++;
>>>     CID 446754:  Memory - illegal accesses  (OVERRUN)
>>>     Overrunning array of 16 bytes at byte offset 16 by dereferencing pointer
>>>     "&ip->a[d]".
91                     memset(&ip->a[d], 0, sizeof(*ip) - d);
92             }
93     }

/lib/net/rte_ip6.h: 114 in rte_ipv6_addr_eq_prefix()
*** CID 446756:  Memory - illegal accesses  (INTEGER_OVERFLOW)
108     rte_ipv6_addr_eq_prefix(const struct rte_ipv6_addr *a, const struct
                                rte_ipv6_addr *b, uint8_t depth)
109     {
110             if (depth < RTE_IPV6_MAX_DEPTH) {
111                     uint8_t d = depth / 8;
112                     uint8_t mask = ~(UINT8_MAX >> (depth % 8));
113
>>>     CID 446756:  Memory - illegal accesses  (INTEGER_OVERFLOW)
>>>     "d", which might have overflowed, is used in a pointer index in "a->a[d]".
114                     if ((a->a[d] ^ b->a[d]) & mask)
115                             return false;
116
117                     return memcmp(a, b, d) == 0;
118             }
119             return rte_ipv6_addr_eq(a, b);

/lib/net/rte_ip6.h: 89 in rte_ipv6_addr_mask()
*** CID 446758:  Memory - corruptions  (INTEGER_OVERFLOW)
83     static inline void
84     rte_ipv6_addr_mask(struct rte_ipv6_addr *ip, uint8_t depth)
85     {
86             if (depth < RTE_IPV6_MAX_DEPTH) {
87                     uint8_t d = depth / 8;
88                     uint8_t mask = ~(UINT8_MAX >> (depth % 8));
>>>     CID 446758:  Memory - corruptions  (INTEGER_OVERFLOW)
>>>     "d", which might have overflowed, is used in a pointer index in "ip->a[d]".
89                     ip->a[d] &= mask;
90                     d++;
91                     memset(&ip->a[d], 0, sizeof(*ip) - d);
92             }
93     }

Cheers.

Robin Jarry (2):
  net/ipv6: fix overflowed array index reads
  net/ipv6: fix out-of-bounds read

 lib/net/rte_ip6.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.47.0

[PATCH dpdk 1/2] net/ipv6: fix overflowed array index reads

[Robin Jarry]

Fix the following overflowed array index reads reported by Coverity:

107 static inline bool
108 rte_ipv6_addr_eq_prefix(const struct rte_ipv6_addr *a,
                            const struct rte_ipv6_addr *b, uint8_t depth)
109 {
        1. Condition depth < 128 /* 16 * 8 */, taking true branch.
110        if (depth < RTE_IPV6_MAX_DEPTH) {
        2. cast_overflow: Truncation due to cast operation on depth / 8
	                  from 32 to 8 bits.
        3. overflow_assign: d is assigned from depth / 8.
111                uint8_t d = depth / 8;
112                uint8_t mask = ~(UINT8_MAX >> (depth % 8));
113
        CID 446756: (#1 of 1): Overflowed array index read
        4. deref_overflow: d, which might have overflowed, is used in
	                   a pointer index in a->a[d].
114                if ((a->a[d] ^ b->a[d]) & mask)
115                        return false;
116
117                return memcmp(a, b, d) == 0;
118        }
119        return rte_ipv6_addr_eq(a, b);
120 }

The same issue has been reported both in rte_ipv6_addr_eq_prefix() and
rte_ipv6_addr_mask(). All arithmetic operations are made using regular
integers and then truncated on assign if necessary (or if explicitly
down cast to a smaller type). In this case, the result of (depth / 8) is
assumed to be on 32 bits and is implicitly down cast 8 bits. This is
causing a warning because it may result in unexpected behaviour.

Change the type of the d variables to unsigned int (32bit by default) to
avoid the overflow warning. Since depth is strictly lesser than
RTE_IPV6_MAX_DEPTH, d will always be lesser than RTE_IPV6_ADDR_SIZE.

Replace the magic 8 literals with CHAR_BIT to be consistent with the
definition of RTE_IPV6_MAX_DEPTH.

Coverity issue: 446756, 446758
Fixes: ca786def84ca ("net: add IPv6 address structure and utils")

Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
 lib/net/rte_ip6.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/net/rte_ip6.h b/lib/net/rte_ip6.h
index 3ae38811b27c..c015c977573d 100644
--- a/lib/net/rte_ip6.h
+++ b/lib/net/rte_ip6.h
@@ -84,8 +84,8 @@ static inline void
 rte_ipv6_addr_mask(struct rte_ipv6_addr *ip, uint8_t depth)
 {
 	if (depth < RTE_IPV6_MAX_DEPTH) {
-		uint8_t d = depth / 8;
-		uint8_t mask = ~(UINT8_MAX >> (depth % 8));
+		unsigned int d = depth / CHAR_BIT;
+		uint8_t mask = ~(UINT8_MAX >> (depth % CHAR_BIT));
 		ip->a[d] &= mask;
 		d++;
 		memset(&ip->a[d], 0, sizeof(*ip) - d);
@@ -108,8 +108,8 @@ static inline bool
 rte_ipv6_addr_eq_prefix(const struct rte_ipv6_addr *a, const struct rte_ipv6_addr *b, uint8_t depth)
 {
 	if (depth < RTE_IPV6_MAX_DEPTH) {
-		uint8_t d = depth / 8;
-		uint8_t mask = ~(UINT8_MAX >> (depth % 8));
+		unsigned int d = depth / CHAR_BIT;
+		uint8_t mask = ~(UINT8_MAX >> (depth % CHAR_BIT));
 
 		if ((a->a[d] ^ b->a[d]) & mask)
 			return false;
-- 
2.47.0

[PATCH dpdk 2/2] net/ipv6: fix out-of-bounds read

[Robin Jarry]

Fix the following out-of-bounds read in rte_ipv6_addr_mask() reported by
Coverity:

83 static inline void
84 rte_ipv6_addr_mask(struct rte_ipv6_addr *ip, uint8_t depth)
85 {
       1. Condition depth < 128 /* 16 * 8 */, taking true branch.
       2. cond_at_most: Checking depth < 128 implies that depth may be
                        up to 127 on the true branch.
86        if (depth < RTE_IPV6_MAX_DEPTH) {
       3. assignment: Assigning: d = depth / 8.
                      The value of d may now be up to 15.
87                uint8_t d = depth / 8;
88                uint8_t mask = ~(UINT8_MAX >> (depth % 8));
89                ip->a[d] &= mask;
       4. incr: Incrementing d. The value of d may now be up to 16.
90                d++;
       CID 446754: (#1 of 1): Out-of-bounds read (OVERRUN)
       5. overrun-local: Overrunning array of 16 bytes at byte offset
                         16 by dereferencing pointer &ip->a[d].
91                memset(&ip->a[d], 0, sizeof(*ip) - d);
92        }
93 }

Use a simple loop instead of memset.

Coverity issue: 446754
Fixes: ca786def84ca ("net: add IPv6 address structure and utils")

Signed-off-by: Robin Jarry <rjarry@redhat.com>
---
 lib/net/rte_ip6.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/net/rte_ip6.h b/lib/net/rte_ip6.h
index c015c977573d..3843fb7c2fd6 100644
--- a/lib/net/rte_ip6.h
+++ b/lib/net/rte_ip6.h
@@ -88,7 +88,8 @@ rte_ipv6_addr_mask(struct rte_ipv6_addr *ip, uint8_t depth)
 		uint8_t mask = ~(UINT8_MAX >> (depth % CHAR_BIT));
 		ip->a[d] &= mask;
 		d++;
-		memset(&ip->a[d], 0, sizeof(*ip) - d);
+		while (d < sizeof(*ip))
+			ip->a[d++] = 0;
 	}
 }
 
-- 
2.47.0

RE: [PATCH dpdk 0/2] IPv6: Fix coverity issues

[Morten Brørup]

For the series,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

BTW:
It's probably too late to change now, but the common term describing the number of set bits in an IP netmask is "prefix length", not "depth".
E.g. the subnet 192.0.2.0/24 has a prefix length (not depth) of 24.

Re: [PATCH dpdk 0/2] IPv6: Fix coverity issues

[David Marchand]

Hello John, Patrick,

On Thu, Oct 24, 2024 at 5:20 PM Robin Jarry <rjarry@redhat.com> wrote:
>
> Here are fixes for three coverity issues:

This series fixes two coverity issues. The fixes look correct.
But the Coverity reports are not obvious to me.

Is there a way to pass those fixes through Coverity to confirm the two
issues are indeed fixed?


-- 
David Marchand

Re: [PATCH dpdk 0/2] IPv6: Fix coverity issues

[David Marchand]

On Tue, Nov 5, 2024 at 10:05 PM David Marchand
<david.marchand@redhat.com> wrote:
> On Thu, Oct 24, 2024 at 5:20 PM Robin Jarry <rjarry@redhat.com> wrote:
> >
> > Here are fixes for three coverity issues:
>
> This series fixes two coverity issues. The fixes look correct.
> But the Coverity reports are not obvious to me.
>
> Is there a way to pass those fixes through Coverity to confirm the two
> issues are indeed fixed?

Well, let's get those fixes in.
We will have the answer tomorrow.

Series applied, thanks.


-- 
David Marchand

Re: [PATCH dpdk 0/2] IPv6: Fix coverity issues

[Patrick Robb]

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

Hi,

There isn't a way to pass the patch through to coverity through our
scripting with how we have it written currently. It probably wouldn't be a
big update to start running it based on a given patch provided, if you
think there may be a somewhat regular need for this. I assume in this case
we would not want to push the result to the coverity website, but rather
just print out the result (to be shared on the mailing list or whatnot).

Otherwise, presumably it would be possible for us to manually spin up our
coverity container image, apply the series, and run the coverity check.

I see you have merged this though. I will go ahead and trigger a coverity
run now, just so that you get the results faster. Sorry about the wait here.

On Wed, Nov 6, 2024 at 3:24 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Tue, Nov 5, 2024 at 10:05 PM David Marchand
> <david.marchand@redhat.com> wrote:
> > On Thu, Oct 24, 2024 at 5:20 PM Robin Jarry <rjarry@redhat.com> wrote:
> > >
> > > Here are fixes for three coverity issues:
> >
> > This series fixes two coverity issues. The fixes look correct.
> > But the Coverity reports are not obvious to me.
> >
> > Is there a way to pass those fixes through Coverity to confirm the two
> > issues are indeed fixed?
>
> Well, let's get those fixes in.
> We will have the answer tomorrow.
>
> Series applied, thanks.
>
>
> --
> David Marchand
>
>

[-- Attachment #2: Type: text/html, Size: 2004 bytes --]

RE: [PATCH dpdk 0/2] IPv6: Fix coverity issues

[Mcnamara, John]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, November 5, 2024 9:06 PM
> To: Mcnamara, John <john.mcnamara@intel.com>; Patrick Robb
> <probb@iol.unh.edu>
> Cc: dev@dpdk.org; Robin Jarry <rjarry@redhat.com>; Morten Brørup
> <mb@smartsharesystems.com>
> Subject: Re: [PATCH dpdk 0/2] IPv6: Fix coverity issues
> 
> Hello John, Patrick,
> 
> On Thu, Oct 24, 2024 at 5:20 PM Robin Jarry <rjarry@redhat.com> wrote:
> >
> > Here are fixes for three coverity issues:
> 
> This series fixes two coverity issues. The fixes look correct.
> But the Coverity reports are not obvious to me.
> 
> Is there a way to pass those fixes through Coverity to confirm the two
> issues are indeed fixed?

There is no easy way to do that. The Coverity analysis only runs on the main after patches have been merged. It isn't structured to allow patch or fix based analysis.