[PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Jonathan Basseri]

If an IPv6 socket has a valid dst cache, then xfrm_lookup_route will get
skipped. However, the cache is not invalidated when applying policy to a
socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
sometimes ignored on those sockets.

This can be demonstrated like so,
1. Create UDPv6 socket.
2. connect() the socket.
3. Apply an outbound XFRM policy to the socket.
4. send() data on the socket.

Packets will continue to be sent in the clear instead of matching an
xfrm or returning a no-match error (EAGAIN). This affects calls to
send() and not sendto().

Note: Creating normal XFRM policies should have a similar effect on
sk_dst_cache entries that match the policy, but that is not fixed in
this patch.

Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
Tested: https://android-review.googlesource.com/418659
Signed-off-by: Jonathan Basseri <misterikkit@google.com>
---
 net/xfrm/xfrm_state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 6c0956d10db6..46294cc833f3 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2028,33 +2028,34 @@ EXPORT_SYMBOL(km_is_alive);
 int xfrm_user_policy(struct sock *sk, int optname, u8 __user *optval, int optlen)
 {
 	int err;
 	u8 *data;
 	struct xfrm_mgr *km;
 	struct xfrm_policy *pol = NULL;
 
 	if (optlen <= 0 || optlen > PAGE_SIZE)
 		return -EMSGSIZE;
 
 	data = memdup_user(optval, optlen);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
 	err = -EINVAL;
 	rcu_read_lock();
 	list_for_each_entry_rcu(km, &xfrm_km_list, list) {
 		pol = km->compile_policy(sk, optname, data,
 					 optlen, &err);
 		if (err >= 0)
 			break;
 	}
 	rcu_read_unlock();
 
 	if (err >= 0) {
 		xfrm_sk_policy_insert(sk, err, pol);
 		xfrm_pol_put(pol);
+		sk_dst_reset(sk);
 		err = 0;
 	}
 
 	kfree(data);
 	return err;
 }
-- 
2.14.1.480.gb18f417b89-goog

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Lorenzo Colitti]

On Wed, Aug 16, 2017 at 7:25 AM, Jonathan Basseri
<misterikkit@google.com> wrote:
> If an IPv6 socket has a valid dst cache

Did you look into why IPv4 does not suffer from this problem?

That said, clearing the dst cache entry does seem prudent in general.

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Jakub Sitnicki]

On Tue, 15 Aug 2017 15:25:10 -0700
Jonathan Basseri <misterikkit@google.com> wrote:

> If an IPv6 socket has a valid dst cache, then xfrm_lookup_route will get
> skipped. However, the cache is not invalidated when applying policy to a
> socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> sometimes ignored on those sockets.
> 
> This can be demonstrated like so,
> 1. Create UDPv6 socket.
> 2. connect() the socket.
> 3. Apply an outbound XFRM policy to the socket.
> 4. send() data on the socket.
> 
> Packets will continue to be sent in the clear instead of matching an
> xfrm or returning a no-match error (EAGAIN). This affects calls to
> send() and not sendto().
> 
> Note: Creating normal XFRM policies should have a similar effect on
> sk_dst_cache entries that match the policy, but that is not fixed in
> this patch.
> 
> Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
> Tested: https://android-review.googlesource.com/418659
> Signed-off-by: Jonathan Basseri <misterikkit@google.com>
> ---

Thank you for the fix.

Acked-by: Jakub Sitnicki <jkbs@redhat.com>

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Eric Dumazet]

On Wed, 2017-08-16 at 11:03 +0200, Jakub Sitnicki wrote:
> On Tue, 15 Aug 2017 15:25:10 -0700
> Jonathan Basseri <misterikkit@google.com> wrote:
> 
> > If an IPv6 socket has a valid dst cache, then xfrm_lookup_route will get
> > skipped. However, the cache is not invalidated when applying policy to a
> > socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> > sometimes ignored on those sockets.
> > 
> > This can be demonstrated like so,
> > 1. Create UDPv6 socket.
> > 2. connect() the socket.
> > 3. Apply an outbound XFRM policy to the socket.
> > 4. send() data on the socket.
> > 
> > Packets will continue to be sent in the clear instead of matching an
> > xfrm or returning a no-match error (EAGAIN). This affects calls to
> > send() and not sendto().
> > 
> > Note: Creating normal XFRM policies should have a similar effect on
> > sk_dst_cache entries that match the policy, but that is not fixed in
> > this patch.
> > 
> > Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
> > Tested: https://android-review.googlesource.com/418659
> > Signed-off-by: Jonathan Basseri <misterikkit@google.com>
> > ---
> 
> Thank you for the fix.
> 
> Acked-by: Jakub Sitnicki <jkbs@redhat.com>

I do not believe this fix is correct.

What happens if the socket is TCP ?

sk_dst_reset(sk) is not safe for them.

This might add use-after-free, and eventually crash.

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Jakub Sitnicki]

On Wed, 16 Aug 2017 03:43:54 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Wed, 2017-08-16 at 11:03 +0200, Jakub Sitnicki wrote:
> > On Tue, 15 Aug 2017 15:25:10 -0700
> > Jonathan Basseri <misterikkit@google.com> wrote:
> >   
> > > If an IPv6 socket has a valid dst cache, then xfrm_lookup_route will get
> > > skipped. However, the cache is not invalidated when applying policy to a
> > > socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> > > sometimes ignored on those sockets.
> > > 
> > > This can be demonstrated like so,
> > > 1. Create UDPv6 socket.
> > > 2. connect() the socket.
> > > 3. Apply an outbound XFRM policy to the socket.
> > > 4. send() data on the socket.
> > > 
> > > Packets will continue to be sent in the clear instead of matching an
> > > xfrm or returning a no-match error (EAGAIN). This affects calls to
> > > send() and not sendto().
> > > 
> > > Note: Creating normal XFRM policies should have a similar effect on
> > > sk_dst_cache entries that match the policy, but that is not fixed in
> > > this patch.
> > > 
> > > Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
> > > Tested: https://android-review.googlesource.com/418659
> > > Signed-off-by: Jonathan Basseri <misterikkit@google.com>
> > > ---  
> > 
> > Thank you for the fix.
> > 
> > Acked-by: Jakub Sitnicki <jkbs@redhat.com>  
> 
> I do not believe this fix is correct.
> 
> What happens if the socket is TCP ?
> 
> sk_dst_reset(sk) is not safe for them.
> 
> This might add use-after-free, and eventually crash.

You are right. I see that RCU-variant __sk_dst_reset() is used
throughout TCP stack. Thank you for pointing it out.

Please disregard my earlier ACK.

-Jakub

[PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Jonathan Basseri]

If a socket has a valid dst cache, then xfrm_lookup_route will get
skipped. However, the cache is not invalidated when applying policy to a
socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
sometimes ignored on those sockets. (Note: This was broken for IPv4 and
IPv6 at different times.)

This can be demonstrated like so,
1. Create UDP socket.
2. connect() the socket.
3. Apply an outbound XFRM policy to the socket.
4. send() data on the socket.

Packets will continue to be sent in the clear instead of matching an
xfrm or returning a no-match error (EAGAIN). This affects calls to
send() and not sendto().

Invalidating the sk_dst_cache is necessary to correctly apply xfrm
policies. Since we do this in xfrm_user_policy(), the sk_lock was
already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
and we may call __sk_dst_reset().

Performance impact should be negligible, since this code is only called
when changing xfrm policy, and only affects the socket in question.

Note: Creating normal XFRM policies should have a similar effect on
sk_dst_cache entries that match the policy, but that is not fixed in
this patch.

Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
Tested: https://android-review.googlesource.com/517555
Tested: https://android-review.googlesource.com/418659
Signed-off-by: Jonathan Basseri <misterikkit@google.com>

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 12213477cd3a..1f5cee2269af 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2045,33 +2045,34 @@ EXPORT_SYMBOL(km_is_alive);
 int xfrm_user_policy(struct sock *sk, int optname, u8 __user *optval, int optlen)
 {
 	int err;
 	u8 *data;
 	struct xfrm_mgr *km;
 	struct xfrm_policy *pol = NULL;
 
 	if (optlen <= 0 || optlen > PAGE_SIZE)
 		return -EMSGSIZE;
 
 	data = memdup_user(optval, optlen);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
 	err = -EINVAL;
 	rcu_read_lock();
 	list_for_each_entry_rcu(km, &xfrm_km_list, list) {
 		pol = km->compile_policy(sk, optname, data,
 					 optlen, &err);
 		if (err >= 0)
 			break;
 	}
 	rcu_read_unlock();
 
 	if (err >= 0) {
 		xfrm_sk_policy_insert(sk, err, pol);
 		xfrm_pol_put(pol);
+		__sk_dst_reset(sk);
 		err = 0;
 	}
 
 	kfree(data);
 	return err;
 }
-- 
2.15.0.rc0.271.g36b669edcc-goog

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Steffen Klassert]

On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri wrote:
> If a socket has a valid dst cache, then xfrm_lookup_route will get
> skipped. However, the cache is not invalidated when applying policy to a
> socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> sometimes ignored on those sockets. (Note: This was broken for IPv4 and
> IPv6 at different times.)
> 
> This can be demonstrated like so,
> 1. Create UDP socket.
> 2. connect() the socket.
> 3. Apply an outbound XFRM policy to the socket.
> 4. send() data on the socket.
> 
> Packets will continue to be sent in the clear instead of matching an
> xfrm or returning a no-match error (EAGAIN). This affects calls to
> send() and not sendto().
> 
> Invalidating the sk_dst_cache is necessary to correctly apply xfrm
> policies. Since we do this in xfrm_user_policy(), the sk_lock was
> already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
> and we may call __sk_dst_reset().
> 
> Performance impact should be negligible, since this code is only called
> when changing xfrm policy, and only affects the socket in question.
> 
> Note: Creating normal XFRM policies should have a similar effect on
> sk_dst_cache entries that match the policy, but that is not fixed in
> this patch.

I think we don't have this problem with 'normal' policies. When
inserting such a policy, we bump the IPv4/IPv6 genid. This should
invalidate all cached dst entries, no?

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Jonathan Basseri 😶]

On Tue, Oct 24, 2017 at 12:04 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri wrote:
> > If a socket has a valid dst cache, then xfrm_lookup_route will get
> > skipped. However, the cache is not invalidated when applying policy to a
> > socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> > sometimes ignored on those sockets. (Note: This was broken for IPv4 and
> > IPv6 at different times.)
> >
> > This can be demonstrated like so,
> > 1. Create UDP socket.
> > 2. connect() the socket.
> > 3. Apply an outbound XFRM policy to the socket.
> > 4. send() data on the socket.
> >
> > Packets will continue to be sent in the clear instead of matching an
> > xfrm or returning a no-match error (EAGAIN). This affects calls to
> > send() and not sendto().
> >
> > Invalidating the sk_dst_cache is necessary to correctly apply xfrm
> > policies. Since we do this in xfrm_user_policy(), the sk_lock was
> > already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
> > and we may call __sk_dst_reset().
> >
> > Performance impact should be negligible, since this code is only called
> > when changing xfrm policy, and only affects the socket in question.
> >
> > Note: Creating normal XFRM policies should have a similar effect on
> > sk_dst_cache entries that match the policy, but that is not fixed in
> > this patch.
>
> I think we don't have this problem with 'normal' policies. When
> inserting such a policy, we bump the IPv4/IPv6 genid. This should
> invalidate all cached dst entries, no?
>
That sounds reasonable to me. I had not confirmed the behavior for
normal policies, so I was trying to point out that this fix is only
for socket policies. Should I modify the commit message?

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Steffen Klassert]

On Tue, Oct 24, 2017 at 09:58:48AM -0700, Jonathan Basseri 😶 wrote:
> On Tue, Oct 24, 2017 at 12:04 AM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> >
> > On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri wrote:
> > > If a socket has a valid dst cache, then xfrm_lookup_route will get
> > > skipped. However, the cache is not invalidated when applying policy to a
> > > socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> > > sometimes ignored on those sockets. (Note: This was broken for IPv4 and
> > > IPv6 at different times.)
> > >
> > > This can be demonstrated like so,
> > > 1. Create UDP socket.
> > > 2. connect() the socket.
> > > 3. Apply an outbound XFRM policy to the socket.
> > > 4. send() data on the socket.
> > >
> > > Packets will continue to be sent in the clear instead of matching an
> > > xfrm or returning a no-match error (EAGAIN). This affects calls to
> > > send() and not sendto().
> > >
> > > Invalidating the sk_dst_cache is necessary to correctly apply xfrm
> > > policies. Since we do this in xfrm_user_policy(), the sk_lock was
> > > already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
> > > and we may call __sk_dst_reset().
> > >
> > > Performance impact should be negligible, since this code is only called
> > > when changing xfrm policy, and only affects the socket in question.
> > >
> > > Note: Creating normal XFRM policies should have a similar effect on
> > > sk_dst_cache entries that match the policy, but that is not fixed in
> > > this patch.
> >
> > I think we don't have this problem with 'normal' policies. When
> > inserting such a policy, we bump the IPv4/IPv6 genid. This should
> > invalidate all cached dst entries, no?
> >
> That sounds reasonable to me. I had not confirmed the behavior for
> normal policies, so I was trying to point out that this fix is only
> for socket policies. Should I modify the commit message?

Yes, please do so. This comment may lead people to the wrong direction.

Thanks!

Re: [PATCH net] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Jonathan Basseri]

On Tue, Oct 24, 2017 at 9:25 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> On Tue, Oct 24, 2017 at 09:58:48AM -0700, Jonathan Basseri 😶 wrote:
>> On Tue, Oct 24, 2017 at 12:04 AM, Steffen Klassert
>> <steffen.klassert@secunet.com> wrote:
>> >
>> > On Mon, Oct 23, 2017 at 06:18:55PM -0700, Jonathan Basseri wrote:
>> > > If a socket has a valid dst cache, then xfrm_lookup_route will get
>> > > skipped. However, the cache is not invalidated when applying policy to a
>> > > socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
>> > > sometimes ignored on those sockets. (Note: This was broken for IPv4 and
>> > > IPv6 at different times.)
>> > >
>> > > This can be demonstrated like so,
>> > > 1. Create UDP socket.
>> > > 2. connect() the socket.
>> > > 3. Apply an outbound XFRM policy to the socket.
>> > > 4. send() data on the socket.
>> > >
>> > > Packets will continue to be sent in the clear instead of matching an
>> > > xfrm or returning a no-match error (EAGAIN). This affects calls to
>> > > send() and not sendto().
>> > >
>> > > Invalidating the sk_dst_cache is necessary to correctly apply xfrm
>> > > policies. Since we do this in xfrm_user_policy(), the sk_lock was
>> > > already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
>> > > and we may call __sk_dst_reset().
>> > >
>> > > Performance impact should be negligible, since this code is only called
>> > > when changing xfrm policy, and only affects the socket in question.
>> > >
>> > > Note: Creating normal XFRM policies should have a similar effect on
>> > > sk_dst_cache entries that match the policy, but that is not fixed in
>> > > this patch.
>> >
>> > I think we don't have this problem with 'normal' policies. When
>> > inserting such a policy, we bump the IPv4/IPv6 genid. This should
>> > invalidate all cached dst entries, no?
>> >
>> That sounds reasonable to me. I had not confirmed the behavior for
>> normal policies, so I was trying to point out that this fix is only
>> for socket policies. Should I modify the commit message?
>
> Yes, please do so. This comment may lead people to the wrong direction.
>
> Thanks!
Thank you for the feedback. Sending a v2 patch with updated message.

[PATCH net v2] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Jonathan Basseri]

If a socket has a valid dst cache, then xfrm_lookup_route will get
skipped. However, the cache is not invalidated when applying policy to a
socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
sometimes ignored on those sockets. (Note: This was broken for IPv4 and
IPv6 at different times.)

This can be demonstrated like so,
1. Create UDP socket.
2. connect() the socket.
3. Apply an outbound XFRM policy to the socket. (setsockopt)
4. send() data on the socket.

Packets will continue to be sent in the clear instead of matching an
xfrm or returning a no-match error (EAGAIN). This affects calls to
send() and not sendto().

Invalidating the sk_dst_cache is necessary to correctly apply xfrm
policies. Since we do this in xfrm_user_policy(), the sk_lock was
already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
and we may call __sk_dst_reset().

Performance impact should be negligible, since this code is only called
when changing xfrm policy, and only affects the socket in question.

Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
Tested: https://android-review.googlesource.com/517555
Tested: https://android-review.googlesource.com/418659
Signed-off-by: Jonathan Basseri <misterikkit@google.com>
---
 net/xfrm/xfrm_state.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 12213477cd3a..1f5cee2269af 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2045,33 +2045,34 @@ EXPORT_SYMBOL(km_is_alive);
 int xfrm_user_policy(struct sock *sk, int optname, u8 __user *optval, int optlen)
 {
 	int err;
 	u8 *data;
 	struct xfrm_mgr *km;
 	struct xfrm_policy *pol = NULL;
 
 	if (optlen <= 0 || optlen > PAGE_SIZE)
 		return -EMSGSIZE;
 
 	data = memdup_user(optval, optlen);
 	if (IS_ERR(data))
 		return PTR_ERR(data);
 
 	err = -EINVAL;
 	rcu_read_lock();
 	list_for_each_entry_rcu(km, &xfrm_km_list, list) {
 		pol = km->compile_policy(sk, optname, data,
 					 optlen, &err);
 		if (err >= 0)
 			break;
 	}
 	rcu_read_unlock();
 
 	if (err >= 0) {
 		xfrm_sk_policy_insert(sk, err, pol);
 		xfrm_pol_put(pol);
+		__sk_dst_reset(sk);
 		err = 0;
 	}
 
 	kfree(data);
 	return err;
 }
-- 
2.15.0.rc2.357.g7e34df9404-goog

Re: [PATCH net v2] xfrm: Clear sk_dst_cache when applying per-socket policy.

[Steffen Klassert]

On Wed, Oct 25, 2017 at 09:52:27AM -0700, Jonathan Basseri wrote:
> If a socket has a valid dst cache, then xfrm_lookup_route will get
> skipped. However, the cache is not invalidated when applying policy to a
> socket (i.e. IPV6_XFRM_POLICY). The result is that new policies are
> sometimes ignored on those sockets. (Note: This was broken for IPv4 and
> IPv6 at different times.)
> 
> This can be demonstrated like so,
> 1. Create UDP socket.
> 2. connect() the socket.
> 3. Apply an outbound XFRM policy to the socket. (setsockopt)
> 4. send() data on the socket.
> 
> Packets will continue to be sent in the clear instead of matching an
> xfrm or returning a no-match error (EAGAIN). This affects calls to
> send() and not sendto().
> 
> Invalidating the sk_dst_cache is necessary to correctly apply xfrm
> policies. Since we do this in xfrm_user_policy(), the sk_lock was
> already acquired in either do_ip_setsockopt() or do_ipv6_setsockopt(),
> and we may call __sk_dst_reset().
> 
> Performance impact should be negligible, since this code is only called
> when changing xfrm policy, and only affects the socket in question.
> 
> Fixes: 00bc0ef5880d ("ipv6: Skip XFRM lookup if dst_entry in socket cache is valid")
> Tested: https://android-review.googlesource.com/517555
> Tested: https://android-review.googlesource.com/418659
> Signed-off-by: Jonathan Basseri <misterikkit@google.com>

Applied, thanks for your work Jonathan!