[PATCH RFC ipsec-next] xfrm: Check Reverse-Mark Lookup Before ADDSA/DELSA

[Nathan Harold <nharold@google.com>]

It's possible to insert an SA into the SADB that will
preclude the lookup of other SAs when using the MARK
attribute. The problem occurs based on a particular
sequencing with the marks where a new mark matches
requests for the SA mark of an existing SA but the
inverse is not true. For an example:
1) Add SA with mark=0, mask=0
2) Add an otherwise-identical SA
   with mark=0x1234, mask=0xFFFFFFFF

This will fail; however, if done in the reverse order
the second add will succeed:
1) Add an SA with mark=0x1234, mask=0xFFFFFFFF
2) Add an otherwise-identical SA
   except with mark=0, mask=0
Then:
3) Delete the SA using mark=0x1234, mask=0xFFFFFFF
4) Dump the SADB, and there will be one SA, and it will
   have mark=0x1234, mask=0xFFFFFFFF; the 0/0 SA will
   be deleted

This patch addresses the problem by performing a
reverse-match on the mark and preventing ADDSA for
any SA that would 'shadow' an existing SA in the SADB.

This patch also address a bug where it was possible to
add an SA with a mark broader than its mask, which
could never be deleted:
1) ADDSA with mark=0x1234, mask=0xFF
2) DELSA with mark=0x1234, mask=ZZZZ; error=ESRCH

By applying both masks to each mark, bits outside the
mask of a given SA mark will be ignored, and the match
will succeed.

This patch does not make any changes to the 'data'
path, so SAs with such oddly-defined marks will still
be unmatch-able.

Signed-off-by: Nathan Harold <nharold@google.com>
---
 net/xfrm/xfrm_state.c | 44 +++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index b669262682c9..ee212a7c91a9 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -815,10 +815,10 @@ xfrm_init_tempstate(struct xfrm_state *x, const struct flowi *fl,
 	afinfo->init_temprop(x, tmpl, daddr, saddr);
 }
 
-static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark,
-					      const xfrm_address_t *daddr,
-					      __be32 spi, u8 proto,
-					      unsigned short family)
+static struct xfrm_state *
+__xfrm_state_lookup(struct net *net, u32 mark, u32 mask,
+		    const xfrm_address_t *daddr,
+		    __be32 spi, u8 proto, unsigned short family)
 {
 	unsigned int h = xfrm_spi_hash(net, daddr, spi, proto, family);
 	struct xfrm_state *x;
@@ -830,7 +830,7 @@ static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark,
 		    !xfrm_addr_equal(&x->id.daddr, daddr, family))
 			continue;
 
-		if ((mark & x->mark.m) != x->mark.v)
+		if ((mark ^ x->mark.v) & mask & x->mark.m)
 			continue;
 		if (!xfrm_state_hold_rcu(x))
 			continue;
@@ -840,10 +840,11 @@ static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark,
 	return NULL;
 }
 
-static struct xfrm_state *__xfrm_state_lookup_byaddr(struct net *net, u32 mark,
-						     const xfrm_address_t *daddr,
-						     const xfrm_address_t *saddr,
-						     u8 proto, unsigned short family)
+static struct xfrm_state *
+__xfrm_state_lookup_byaddr(struct net *net, u32 mark, u32 mask,
+			   const xfrm_address_t *daddr,
+			   const xfrm_address_t *saddr,
+			   u8 proto, unsigned short family)
 {
 	unsigned int h = xfrm_src_hash(net, daddr, saddr, family);
 	struct xfrm_state *x;
@@ -855,7 +856,7 @@ static struct xfrm_state *__xfrm_state_lookup_byaddr(struct net *net, u32 mark,
 		    !xfrm_addr_equal(&x->props.saddr, saddr, family))
 			continue;
 
-		if ((mark & x->mark.m) != x->mark.v)
+		if ((mark ^ x->mark.v) & mask & x->mark.m)
 			continue;
 		if (!xfrm_state_hold_rcu(x))
 			continue;
@@ -869,15 +870,14 @@ static inline struct xfrm_state *
 __xfrm_state_locate(struct xfrm_state *x, int use_spi, int family)
 {
 	struct net *net = xs_net(x);
-	u32 mark = x->mark.v & x->mark.m;
 
 	if (use_spi)
-		return __xfrm_state_lookup(net, mark, &x->id.daddr,
-					   x->id.spi, x->id.proto, family);
+		return __xfrm_state_lookup(net, x->mark.v, x->mark.m,
+					   &x->id.daddr, x->id.spi,
+					   x->id.proto, family);
 	else
-		return __xfrm_state_lookup_byaddr(net, mark,
-						  &x->id.daddr,
-						  &x->props.saddr,
+		return __xfrm_state_lookup_byaddr(net, x->mark.v, x->mark.m,
+						  &x->id.daddr, &x->props.saddr,
 						  x->id.proto, family);
 }
 
@@ -985,8 +985,10 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 	x = best;
 	if (!x && !error && !acquire_in_progress) {
 		if (tmpl->id.spi &&
-		    (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi,
-					      tmpl->id.proto, encap_family)) != NULL) {
+		    (x0 = __xfrm_state_lookup(net, mark, 0xFFFFFFFF,
+					      daddr, tmpl->id.spi,
+					      tmpl->id.proto,
+					      encap_family)) != NULL) {
 			to_put = x0;
 			error = -EEXIST;
 			goto out;
@@ -1617,7 +1619,8 @@ xfrm_state_lookup(struct net *net, u32 mark, const xfrm_address_t *daddr, __be32
 	struct xfrm_state *x;
 
 	rcu_read_lock();
-	x = __xfrm_state_lookup(net, mark, daddr, spi, proto, family);
+	x = __xfrm_state_lookup(net, mark, 0xFFFFFFFF,
+				daddr, spi, proto, family);
 	rcu_read_unlock();
 	return x;
 }
@@ -1631,7 +1634,8 @@ xfrm_state_lookup_byaddr(struct net *net, u32 mark,
 	struct xfrm_state *x;
 
 	spin_lock_bh(&net->xfrm.xfrm_state_lock);
-	x = __xfrm_state_lookup_byaddr(net, mark, daddr, saddr, proto, family);
+	x = __xfrm_state_lookup_byaddr(net, mark, 0xFFFFFFFF,
+				       daddr, saddr, proto, family);
 	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 	return x;
 }
-- 
2.18.0.345.g5c9ce644c3-goog