| Message ID | ZIBCFyszqwJlZd/V@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xfrm: Use xfrm_state selector for BEET input | expand |
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
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,
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!
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,
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!
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;