[PATCH] xfrm: Remove inner/outer modes from input path

[PATCH] xfrm: Remove inner/outer modes from input path

[Herbert Xu]

The inner/outer modes were added to abstract out common code that
were once duplicated between IPv4 and IPv6.  As time went on the
abstractions have been removed and we are now left with empty
shells that only contain duplicate information.  These can be
removed one-by-one as the same information is already present
elsewhere in the xfrm_state object.

Removing them from the input path actually allows certain valid
combinations that are currently disallowed.  In particular, when
a transport mode SA sits beneath a tunnel mode SA that changes
address families, at present the transport mode SA cannot have
AF_UNSPEC as its selector because it will be erroneously be treated
as inter-family itself even though it simply sits beneath one.

This is a serious problem because you can't set the selector to
non-AF_UNSPEC either as that will cause the selector match to
fail as we always match selectors to the inner-most traffic.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 436d29640ac2..39fb91ff23d9 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -231,9 +231,6 @@ static int xfrm4_remove_tunnel_encap(struct xfrm_state *x, struct sk_buff *skb)
 {
 	int err = -EINVAL;
 
-	if (XFRM_MODE_SKB_CB(skb)->protocol != IPPROTO_IPIP)
-		goto out;
-
 	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
 		goto out;
 
@@ -269,8 +266,6 @@ static int xfrm6_remove_tunnel_encap(struct xfrm_state *x, struct sk_buff *skb)
 {
 	int err = -EINVAL;
 
-	if (XFRM_MODE_SKB_CB(skb)->protocol != IPPROTO_IPV6)
-		goto out;
 	if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
 		goto out;
 
@@ -331,22 +326,26 @@ static int xfrm6_remove_beet_encap(struct xfrm_state *x, struct sk_buff *skb)
  */
 static int
 xfrm_inner_mode_encap_remove(struct xfrm_state *x,
-			     const struct xfrm_mode *inner_mode,
 			     struct sk_buff *skb)
 {
-	switch (inner_mode->encap) {
+	switch (x->props.mode) {
 	case XFRM_MODE_BEET:
-		if (inner_mode->family == AF_INET)
+		switch (XFRM_MODE_SKB_CB(skb)->protocol) {
+		case IPPROTO_IPIP:
+		case IPPROTO_BEETPH:
 			return xfrm4_remove_beet_encap(x, skb);
-		if (inner_mode->family == AF_INET6)
+		case IPPROTO_IPV6:
 			return xfrm6_remove_beet_encap(x, skb);
+		}
 		break;
 	case XFRM_MODE_TUNNEL:
-		if (inner_mode->family == AF_INET)
+		switch (XFRM_MODE_SKB_CB(skb)->protocol) {
+		case IPPROTO_IPIP:
 			return xfrm4_remove_tunnel_encap(x, skb);
-		if (inner_mode->family == AF_INET6)
+		case IPPROTO_IPV6:
 			return xfrm6_remove_tunnel_encap(x, skb);
 		break;
+		}
 	}
 
 	WARN_ON_ONCE(1);
@@ -355,9 +354,7 @@ xfrm_inner_mode_encap_remove(struct xfrm_state *x,
 
 static int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
 {
-	const struct xfrm_mode *inner_mode = &x->inner_mode;
-
-	switch (x->outer_mode.family) {
+	switch (x->props.family) {
 	case AF_INET:
 		xfrm4_extract_header(skb);
 		break;
@@ -369,17 +366,12 @@ static int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
 		return -EAFNOSUPPORT;
 	}
 
-	if (x->sel.family == AF_UNSPEC) {
-		inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
-		if (!inner_mode)
-			return -EAFNOSUPPORT;
-	}
-
-	switch (inner_mode->family) {
-	case AF_INET:
+	switch (XFRM_MODE_SKB_CB(skb)->protocol) {
+	case IPPROTO_IPIP:
+	case IPPROTO_BEETPH:
 		skb->protocol = htons(ETH_P_IP);
 		break;
-	case AF_INET6:
+	case IPPROTO_IPV6:
 		skb->protocol = htons(ETH_P_IPV6);
 		break;
 	default:
@@ -387,7 +379,7 @@ static int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
 		break;
 	}
 
-	return xfrm_inner_mode_encap_remove(x, inner_mode, skb);
+	return xfrm_inner_mode_encap_remove(x, skb);
 }
 
 /* Remove encapsulation header.
@@ -433,17 +425,16 @@ static int xfrm6_transport_input(struct xfrm_state *x, struct sk_buff *skb)
 }
 
 static int xfrm_inner_mode_input(struct xfrm_state *x,
-				 const struct xfrm_mode *inner_mode,
 				 struct sk_buff *skb)
 {
-	switch (inner_mode->encap) {
+	switch (x->props.mode) {
 	case XFRM_MODE_BEET:
 	case XFRM_MODE_TUNNEL:
 		return xfrm_prepare_input(x, skb);
 	case XFRM_MODE_TRANSPORT:
-		if (inner_mode->family == AF_INET)
+		if (x->props.family == AF_INET)
 			return xfrm4_transport_input(x, skb);
-		if (inner_mode->family == AF_INET6)
+		if (x->props.family == AF_INET6)
 			return xfrm6_transport_input(x, skb);
 		break;
 	case XFRM_MODE_ROUTEOPTIMIZATION:
@@ -461,7 +452,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 {
 	const struct xfrm_state_afinfo *afinfo;
 	struct net *net = dev_net(skb->dev);
-	const struct xfrm_mode *inner_mode;
 	int err;
 	__be32 seq;
 	__be32 seq_hi;
@@ -491,7 +481,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			goto drop;
 		}
 
-		family = x->outer_mode.family;
+		family = x->props.family;
 
 		/* An encap_type of -1 indicates async resumption. */
 		if (encap_type == -1) {
@@ -676,17 +666,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		XFRM_MODE_SKB_CB(skb)->protocol = nexthdr;
 
-		inner_mode = &x->inner_mode;
-
-		if (x->sel.family == AF_UNSPEC) {
-			inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
-			if (inner_mode == NULL) {
-				XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMODEERROR);
-				goto drop;
-			}
-		}
-
-		if (xfrm_inner_mode_input(x, inner_mode, skb)) {
+		if (xfrm_inner_mode_input(x, skb)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMODEERROR);
 			goto drop;
 		}
@@ -701,7 +681,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		 * transport mode so the outer address is identical.
 		 */
 		daddr = &x->id.daddr;
-		family = x->outer_mode.family;
+		family = x->props.family;
 
 		err = xfrm_parse_spi(skb, nexthdr, &spi, &seq);
 		if (err < 0) {
@@ -732,7 +712,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		err = -EAFNOSUPPORT;
 		rcu_read_lock();
-		afinfo = xfrm_state_afinfo_get_rcu(x->inner_mode.family);
+		afinfo = xfrm_state_afinfo_get_rcu(x->props.family);
 		if (likely(afinfo))
 			err = afinfo->transport_finish(skb, xfrm_gro || async);
 		rcu_read_unlock();
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: Remove inner/outer modes from input path

[Steffen Klassert]

On Fri, Mar 10, 2023 at 05:26:05PM +0800, Herbert Xu wrote:
> The inner/outer modes were added to abstract out common code that
> were once duplicated between IPv4 and IPv6.  As time went on the
> abstractions have been removed and we are now left with empty
> shells that only contain duplicate information.  These can be
> removed one-by-one as the same information is already present
> elsewhere in the xfrm_state object.
> 
> Removing them from the input path actually allows certain valid
> combinations that are currently disallowed.  In particular, when
> a transport mode SA sits beneath a tunnel mode SA that changes
> address families, at present the transport mode SA cannot have
> AF_UNSPEC as its selector because it will be erroneously be treated
> as inter-family itself even though it simply sits beneath one.
> 
> This is a serious problem because you can't set the selector to
> non-AF_UNSPEC either as that will cause the selector match to
> fail as we always match selectors to the inner-most traffic.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied to ipsec-next, thanks a lot Herbert!

Re: [PATCH] xfrm: Remove inner/outer modes from input path

[Steffen Klassert]

Hi Herbert,

On Fri, Mar 10, 2023 at 05:26:05PM +0800, Herbert Xu wrote:
...
> @@ -369,17 +366,12 @@ static int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
>  		return -EAFNOSUPPORT;
>  	}
>  
> -	if (x->sel.family == AF_UNSPEC) {
> -		inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
> -		if (!inner_mode)
> -			return -EAFNOSUPPORT;
> -	}
> -
> -	switch (inner_mode->family) {
> -	case AF_INET:
> +	switch (XFRM_MODE_SKB_CB(skb)->protocol) {
> +	case IPPROTO_IPIP:
> +	case IPPROTO_BEETPH:

the assumption that the L4 protocol on BEET mode can be
just IPIP or BEETPH seems not to be correct. One of
our testcaces hit the second WARN_ON_ONCE() in
xfrm_prepare_input. In that case the L4 protocol
is UDP. Looks like we need some other way to
dertermine the inner protocol family.

[PATCH] xfrm: Use xfrm_state selector for BEET input

[Herbert Xu]

On Tue, Jun 06, 2023 at 12:45:29PM +0200, Steffen Klassert wrote:
>
> the assumption that the L4 protocol on BEET mode can be
> just IPIP or BEETPH seems not to be correct. One of
> our testcaces hit the second WARN_ON_ONCE() in
> xfrm_prepare_input. In that case the L4 protocol
> is UDP. Looks like we need some other way to
> dertermine the inner protocol family.

Oops, that was a thinko on my part:

---8<---
For BEET the inner address and therefore family is stored in the
xfrm_state selector.  Use that when decapsulating an input packet
instead of incorrectly relying on a non-existent tunnel protocol.

Fixes: 5f24f41e8ea6 ("xfrm: Remove inner/outer modes from input path")
Reported-by: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 39fb91ff23d9..bdaed1d1ff97 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -330,11 +330,10 @@ xfrm_inner_mode_encap_remove(struct xfrm_state *x,
 {
 	switch (x->props.mode) {
 	case XFRM_MODE_BEET:
-		switch (XFRM_MODE_SKB_CB(skb)->protocol) {
-		case IPPROTO_IPIP:
-		case IPPROTO_BEETPH:
+		switch (x->sel.family) {
+		case AF_INET:
 			return xfrm4_remove_beet_encap(x, skb);
-		case IPPROTO_IPV6:
+		case AF_INET6:
 			return xfrm6_remove_beet_encap(x, skb);
 		}
 		break;
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: Use xfrm_state selector for BEET input

[Steffen Klassert]

On Wed, Jun 07, 2023 at 04:38:47PM +0800, Herbert Xu wrote:
> On Tue, Jun 06, 2023 at 12:45:29PM +0200, Steffen Klassert wrote:
> >
> > the assumption that the L4 protocol on BEET mode can be
> > just IPIP or BEETPH seems not to be correct. One of
> > our testcaces hit the second WARN_ON_ONCE() in
> > xfrm_prepare_input. In that case the L4 protocol
> > is UDP. Looks like we need some other way to
> > dertermine the inner protocol family.
> 
> Oops, that was a thinko on my part:
> 
> ---8<---
> For BEET the inner address and therefore family is stored in the
> xfrm_state selector.  Use that when decapsulating an input packet
> instead of incorrectly relying on a non-existent tunnel protocol.
> 
> Fixes: 5f24f41e8ea6 ("xfrm: Remove inner/outer modes from input path")
> Reported-by: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 39fb91ff23d9..bdaed1d1ff97 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -330,11 +330,10 @@ xfrm_inner_mode_encap_remove(struct xfrm_state *x,
>  {
>  	switch (x->props.mode) {
>  	case XFRM_MODE_BEET:
> -		switch (XFRM_MODE_SKB_CB(skb)->protocol) {
> -		case IPPROTO_IPIP:
> -		case IPPROTO_BEETPH:
> +		switch (x->sel.family) {

Hm, I thought about that one too. But x->sel.family can
also be AF_UNSPEC, in which case we used the address
family of the inner mode before your change.

> +		case AF_INET:
>  			return xfrm4_remove_beet_encap(x, skb);
> -		case IPPROTO_IPV6:
> +		case AF_INET6:
>  			return xfrm6_remove_beet_encap(x, skb);
>  		}
>  		break;
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: Use xfrm_state selector for BEET input

[Herbert Xu]

On Wed, Jun 07, 2023 at 10:47:45AM +0200, Steffen Klassert wrote:
>
> Hm, I thought about that one too. But x->sel.family can
> also be AF_UNSPEC, in which case we used the address
> family of the inner mode before your change.

It must not be AF_UNSPECT for BEET.  How would you even get the
inner addresses if it were UNSPEC?

With BEET the inner addresses are stored in the IPsec SA rather
than the actual packet.  So the family is also fixed for a given
SA (which we call xfrm_state).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: Use xfrm_state selector for BEET input

[Steffen Klassert]

On Wed, Jun 07, 2023 at 04:50:51PM +0800, Herbert Xu wrote:
> On Wed, Jun 07, 2023 at 10:47:45AM +0200, Steffen Klassert wrote:
> >
> > Hm, I thought about that one too. But x->sel.family can
> > also be AF_UNSPEC, in which case we used the address
> > family of the inner mode before your change.
> 
> It must not be AF_UNSPECT for BEET.  How would you even get the
> inner addresses if it were UNSPEC?
> 
> With BEET the inner addresses are stored in the IPsec SA rather
> than the actual packet.  So the family is also fixed for a given
> SA (which we call xfrm_state).

Right, Good point.

Thanks!

Re: [PATCH] xfrm: Use xfrm_state selector for BEET input

[Herbert Xu]

On Wed, Jun 07, 2023 at 10:57:38AM +0200, Steffen Klassert wrote:
>
> > With BEET the inner addresses are stored in the IPsec SA rather
> > than the actual packet.  So the family is also fixed for a given
> > SA (which we call xfrm_state).
> 
> Right, Good point.

We should probably add some checks in xfrm_user to ensure that
BEET-mode SAs come with valid inner addresses (and family) just
in case user-space tries something funny on us.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] xfrm: Use xfrm_state selector for BEET input

[Steffen Klassert]

On Wed, Jun 07, 2023 at 04:38:47PM +0800, Herbert Xu wrote:
> On Tue, Jun 06, 2023 at 12:45:29PM +0200, Steffen Klassert wrote:
> >
> > the assumption that the L4 protocol on BEET mode can be
> > just IPIP or BEETPH seems not to be correct. One of
> > our testcaces hit the second WARN_ON_ONCE() in
> > xfrm_prepare_input. In that case the L4 protocol
> > is UDP. Looks like we need some other way to
> > dertermine the inner protocol family.
> 
> Oops, that was a thinko on my part:
> 
> ---8<---
> For BEET the inner address and therefore family is stored in the
> xfrm_state selector.  Use that when decapsulating an input packet
> instead of incorrectly relying on a non-existent tunnel protocol.
> 
> Fixes: 5f24f41e8ea6 ("xfrm: Remove inner/outer modes from input path")
> Reported-by: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks a lot Herbert!