| Message ID | 20201229145009.cGOUak0JdKIIgGAv@hankala.org |
|---|---|
| State | New |
| Headers | show |
| Series | xfrm: Fix wraparound in xfrm_policy_addr_delta() | expand |
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.
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.
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.
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:
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> asd