[PATCH] xfrm: Fix wraparound in xfrm_policy_addr_delta()

[PATCH] xfrm: Fix wraparound in xfrm_policy_addr_delta()

[Visa Hankala]

Use three-way comparison for address elements to avoid integer
wraparound in the result of xfrm_policy_addr_delta().

This ensures that the search trees are built and traversed correctly
when the difference between compared address elements is larger
than INT_MAX.

Fixes: 9cf545ebd591d ("xfrm: policy: store inexact policies in a tree ordered by destination address")
Signed-off-by: Visa Hankala <visa@hankala.org>

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index d622c2548d22..d74902f11dde 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -793,15 +793,20 @@ static int xfrm_policy_addr_delta(const xfrm_address_t *a,
 				  const xfrm_address_t *b,
 				  u8 prefixlen, u16 family)
 {
+	u32 ma, mb, mask;
 	unsigned int pdw, pbi;
 	int delta = 0;
 
 	switch (family) {
 	case AF_INET:
-		if (sizeof(long) == 4 && prefixlen == 0)
-			return ntohl(a->a4) - ntohl(b->a4);
-		return (ntohl(a->a4) & ((~0UL << (32 - prefixlen)))) -
-		       (ntohl(b->a4) & ((~0UL << (32 - prefixlen))));
+		mask = ~0U << (32 - prefixlen);
+		ma = ntohl(a->a4) & mask;
+		mb = ntohl(b->a4) & mask;
+		if (ma < mb)
+			delta = -1;
+		else if (ma > mb)
+			delta = 1;
+		break;
 	case AF_INET6:
 		pdw = prefixlen >> 5;
 		pbi = prefixlen & 0x1f;
@@ -812,10 +817,13 @@ static int xfrm_policy_addr_delta(const xfrm_address_t *a,
 				return delta;
 		}
 		if (pbi) {
-			u32 mask = ~0u << (32 - pbi);
-
-			delta = (ntohl(a->a6[pdw]) & mask) -
-				(ntohl(b->a6[pdw]) & mask);
+			mask = ~0U << (32 - pbi);
+			ma = ntohl(a->a6[pdw]) & mask;
+			mb = ntohl(b->a6[pdw]) & mask;
+			if (ma < mb)
+				delta = -1;
+			else if (ma > mb)
+				delta = 1;
 		}
 		break;
 	default:
asd

Re: [PATCH] xfrm: Fix wraparound in xfrm_policy_addr_delta()

[Florian Westphal]

Visa Hankala <visa@hankala.org> wrote:
> Use three-way comparison for address elements to avoid integer
> wraparound in the result of xfrm_policy_addr_delta().
> 
> This ensures that the search trees are built and traversed correctly
> when the difference between compared address elements is larger
> than INT_MAX.

Please provide an update to tools/testing/selftests/net/xfrm_policy.sh
that shows that this is a problem.

>  	switch (family) {
>  	case AF_INET:
> -		if (sizeof(long) == 4 && prefixlen == 0)
> -			return ntohl(a->a4) - ntohl(b->a4);
> -		return (ntohl(a->a4) & ((~0UL << (32 - prefixlen)))) -
> -		       (ntohl(b->a4) & ((~0UL << (32 - prefixlen))));
> +		mask = ~0U << (32 - prefixlen);
> +		ma = ntohl(a->a4) & mask;
> +		mb = ntohl(b->a4) & mask;

This is suspicious.  Is prefixlen == 0 impossible?

If not, then after patch
mask = ~0U << 32;

... and function returns 0.

Re: [PATCH] xfrm: Fix wraparound in xfrm_policy_addr_delta()

[Visa Hankala]

On Tue, Dec 29, 2020 at 05:01:27PM +0100, Florian Westphal wrote:
> Visa Hankala <visa@hankala.org> wrote:
> > Use three-way comparison for address elements to avoid integer
> > wraparound in the result of xfrm_policy_addr_delta().
> > 
> > This ensures that the search trees are built and traversed correctly
> > when the difference between compared address elements is larger
> > than INT_MAX.
> 
> Please provide an update to tools/testing/selftests/net/xfrm_policy.sh
> that shows that this is a problem.

I will do that in the next revision.

> >  	switch (family) {
> >  	case AF_INET:
> > -		if (sizeof(long) == 4 && prefixlen == 0)
> > -			return ntohl(a->a4) - ntohl(b->a4);
> > -		return (ntohl(a->a4) & ((~0UL << (32 - prefixlen)))) -
> > -		       (ntohl(b->a4) & ((~0UL << (32 - prefixlen))));
> > +		mask = ~0U << (32 - prefixlen);
> > +		ma = ntohl(a->a4) & mask;
> > +		mb = ntohl(b->a4) & mask;
> 
> This is suspicious.  Is prefixlen == 0 impossible?
> 
> If not, then after patch
> mask = ~0U << 32;
> 
> ... and function returns 0.

prefixlen == 0 is possible. However, I now realize that left shift
by 32 should be avoided with 32-bit integers.

With prefixlen == 0, there is only one equivalence class, so
returning 0 seems reasonable to me.

Is there a reason why the function has treated /0 prefix as /32
with IPv4? IPv6 does not have this treatment.

Re: [PATCH] xfrm: Fix wraparound in xfrm_policy_addr_delta()

[Florian Westphal]

Visa Hankala <visa@hankala.org> wrote:
> On Tue, Dec 29, 2020 at 05:01:27PM +0100, Florian Westphal wrote:
> > This is suspicious.  Is prefixlen == 0 impossible?
> > 
> > If not, then after patch
> > mask = ~0U << 32;
> > 
> > ... and function returns 0.
> 
> With prefixlen == 0, there is only one equivalence class, so
> returning 0 seems reasonable to me.

Right, that seems reasonable indeed.

> Is there a reason why the function has treated /0 prefix as /32
> with IPv4? IPv6 does not have this treatment.

Not that I recall, looks like a bug.