[PATCH ipsec] xfrm: Pass template address family to xfrm_state_look_at

[PATCH ipsec] xfrm: Pass template address family to xfrm_state_look_at

[Anthony DeRossi]

This fixes a regression where valid selectors are incorrectly skipped
when xfrm_state_find is called with a non-matching address family (e.g.
when using IPv6-in-IPv4 ESP in transport mode).

The state's address family is matched against the template's family
(encap_family) in xfrm_state_find before checking the selector in
xfrm_state_look_at.  The template's family should also be used for
selector matching, otherwise valid selectors may be skipped.

Fixes: e94ee171349d ("xfrm: Use correct address family in xfrm_state_find")
Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
---
 net/xfrm/xfrm_state.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index ee6ac32bb06d..065f1bd8479a 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1075,7 +1075,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		    tmpl->mode == x->props.mode &&
 		    tmpl->id.proto == x->id.proto &&
 		    (tmpl->id.spi == x->id.spi || !tmpl->id.spi))
-			xfrm_state_look_at(pol, x, fl, family,
+			xfrm_state_look_at(pol, x, fl, encap_family,
 					   &best, &acquire_in_progress, &error);
 	}
 	if (best || acquire_in_progress)
@@ -1092,7 +1092,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		    tmpl->mode == x->props.mode &&
 		    tmpl->id.proto == x->id.proto &&
 		    (tmpl->id.spi == x->id.spi || !tmpl->id.spi))
-			xfrm_state_look_at(pol, x, fl, family,
+			xfrm_state_look_at(pol, x, fl, encap_family,
 					   &best, &acquire_in_progress, &error);
 	}
 
-- 
2.26.2

Re: [PATCH ipsec] xfrm: Pass template address family to xfrm_state_look_at

[Herbert Xu]

On Mon, Nov 02, 2020 at 06:32:19PM -0800, Anthony DeRossi wrote:
> This fixes a regression where valid selectors are incorrectly skipped
> when xfrm_state_find is called with a non-matching address family (e.g.
> when using IPv6-in-IPv4 ESP in transport mode).
> 
> The state's address family is matched against the template's family
> (encap_family) in xfrm_state_find before checking the selector in
> xfrm_state_look_at.  The template's family should also be used for
> selector matching, otherwise valid selectors may be skipped.
> 
> Fixes: e94ee171349d ("xfrm: Use correct address family in xfrm_state_find")
> Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
> ---
>  net/xfrm/xfrm_state.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Your patch reintroduces the same bug that my patch was trying to
fix, namely that when you do the comparison on flow you must use
the original family and not some other value.

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 ipsec] xfrm: Pass template address family to xfrm_state_look_at

[Herbert Xu]

On Mon, Nov 02, 2020 at 06:32:19PM -0800, Anthony DeRossi wrote:
> This fixes a regression where valid selectors are incorrectly skipped
> when xfrm_state_find is called with a non-matching address family (e.g.
> when using IPv6-in-IPv4 ESP in transport mode).

Why are we even allowing v6-over-v4 in transport mode? Isn't that
the whole point of BEET mode?

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 ipsec] xfrm: Pass template address family to xfrm_state_look_at

[Anthony DeRossi]

On Tue, Nov 3, 2020 at 4:05 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Nov 02, 2020 at 06:32:19PM -0800, Anthony DeRossi wrote:
> > This fixes a regression where valid selectors are incorrectly skipped
> > when xfrm_state_find is called with a non-matching address family (e.g.
> > when using IPv6-in-IPv4 ESP in transport mode).
> >
> > The state's address family is matched against the template's family
> > (encap_family) in xfrm_state_find before checking the selector in
> > xfrm_state_look_at.  The template's family should also be used for
> > selector matching, otherwise valid selectors may be skipped.
> >
> > Fixes: e94ee171349d ("xfrm: Use correct address family in xfrm_state_find")
> > Signed-off-by: Anthony DeRossi <ajderossi@gmail.com>
> > ---
> >  net/xfrm/xfrm_state.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Your patch reintroduces the same bug that my patch was trying to
> fix, namely that when you do the comparison on flow you must use
> the original family and not some other value.

My mistake, I misunderstood the original bug.

Anthony

Re: [PATCH ipsec] xfrm: Pass template address family to xfrm_state_look_at

[Anthony DeRossi]

On Tue, Nov 3, 2020 at 4:08 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Nov 02, 2020 at 06:32:19PM -0800, Anthony DeRossi wrote:
> > This fixes a regression where valid selectors are incorrectly skipped
> > when xfrm_state_find is called with a non-matching address family (e.g.
> > when using IPv6-in-IPv4 ESP in transport mode).
>
> Why are we even allowing v6-over-v4 in transport mode? Isn't that
> the whole point of BEET mode?

I'm not sure. This is the outgoing policy that strongSwan creates for
an IPv6-in-IPv4 tunnel when compression is enabled:

src fd02::/16 dst fd02::2/128
        dir out priority 326271 ptype main
        tmpl src 10.0.0.8 dst 192.168.1.231
                proto comp spi 0x0000d00e reqid 1 mode tunnel
        tmpl src 0.0.0.0 dst 0.0.0.0
                proto esp spi 0xc543e950 reqid 1 mode transport

After your patch, outgoing IPv6 packets fail to match the associated state:

src 10.0.0.8 dst 192.168.1.231
        proto esp spi 0xc543e950 reqid 1 mode transport
        replay-window 0
        auth-trunc hmac(sha256)
0x143b570f59b23eaa560905f19a922451c6dfa5694ba2e45e1b065bb1863421aa 128
        enc cbc(aes) 0x526ed144ca087125ce30e36c8f20d972
        encap type espinudp sport 4501 dport 4500 addr 0.0.0.0
        anti-replay context: seq 0x0, oseq 0x0, bitmap 0x00000000
        sel src 0.0.0.0/0 dst 0.0.0.0/0

Is this an invalid configuration?

Anthony