[PATCH net] net: make skb_dst_force return false when dst was cleared

[PATCH net] net: make skb_dst_force return false when dst was cleared

[Florian Westphal]

XFRM and netfilter don't expect that skb_dst_force() can cause skb to lose
its dst entry.

I got a bug report with a skb->dst NULL dereference in netfilter
output path.  The backtrace contains nf_reinject(), so the dst
might have been cleared when skb got queued to userspace.

The xfrm part of this change was done after code inspection,
it looks like similar crash could happen here too.

One way to fix this is to add a skb_dst() check right after
skb_dst_force() call, but I think its preferable to make the
'dst might get cleared' part of the function explicit.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/dst.h        | 6 +++++-
 net/netfilter/nf_queue.c | 6 +++++-
 net/xfrm/xfrm_policy.c   | 5 ++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 12b31c602cb0..42cd53d51364 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -302,8 +302,9 @@ static inline bool dst_hold_safe(struct dst_entry *dst)
  * @skb: buffer
  *
  * If dst is not yet refcounted and not destroyed, grab a ref on it.
+ * Returns false if skb had a destroyed dst.
  */
-static inline void skb_dst_force(struct sk_buff *skb)
+static inline bool skb_dst_force(struct sk_buff *skb)
 {
 	if (skb_dst_is_noref(skb)) {
 		struct dst_entry *dst = skb_dst(skb);
@@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
 			dst = NULL;
 
 		skb->_skb_refdst = (unsigned long)dst;
+		return dst != NULL;
 	}
+
+	return true;
 }
 
 
diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index b5b2be55ca82..dc8628a919a5 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -190,6 +190,11 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		goto err;
 	}
 
+	if (!skb_dst_force(skb)) {
+		status = -ENETDOWN;
+		goto err;
+	}
+
 	*entry = (struct nf_queue_entry) {
 		.skb	= skb,
 		.state	= *state,
@@ -198,7 +203,6 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 	};
 
 	nf_queue_entry_get_refs(entry);
-	skb_dst_force(skb);
 
 	switch (entry->state.pf) {
 	case AF_INET:
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index b1694d5d15d3..5c66c18d5ff5 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2842,7 +2842,10 @@ static int xdst_queue_output(struct net *net, struct sock *sk, struct sk_buff *s
 		return -EAGAIN;
 	}
 
-	skb_dst_force(skb);
+	if (!skb_dst_force(skb)) {
+		kfree_skb(skb);
+		return -ENETDOWN;
+	}
 
 	spin_lock_bh(&pq->hold_queue.lock);
 
-- 
2.21.0

Re: [PATCH net] net: make skb_dst_force return false when dst was cleared

[Eric Dumazet]

On 6/25/19 12:22 PM, Florian Westphal wrote:
> XFRM and netfilter don't expect that skb_dst_force() can cause skb to lose
> its dst entry.
> 
> I got a bug report with a skb->dst NULL dereference in netfilter
> output path.  The backtrace contains nf_reinject(), so the dst
> might have been cleared when skb got queued to userspace.
> 
> The xfrm part of this change was done after code inspection,
> it looks like similar crash could happen here too.
> 
> One way to fix this is to add a skb_dst() check right after
> skb_dst_force() call, but I think its preferable to make the
> 'dst might get cleared' part of the function explicit.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/net/dst.h        | 6 +++++-
>  net/netfilter/nf_queue.c | 6 +++++-
>  net/xfrm/xfrm_policy.c   | 5 ++++-
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 12b31c602cb0..42cd53d51364 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -302,8 +302,9 @@ static inline bool dst_hold_safe(struct dst_entry *dst)
>   * @skb: buffer
>   *
>   * If dst is not yet refcounted and not destroyed, grab a ref on it.
> + * Returns false if skb had a destroyed dst.

>   */
> -static inline void skb_dst_force(struct sk_buff *skb)
> +static inline bool skb_dst_force(struct sk_buff *skb)
>  {
>  	if (skb_dst_is_noref(skb)) {
>  		struct dst_entry *dst = skb_dst(skb);
> @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
>  			dst = NULL;
>  
>  		skb->_skb_refdst = (unsigned long)dst;
> +		return dst != NULL;
>  	}
> +
> +	return true;

This will return true, even if skb has a NULL dst.

Say if we have two skb_dst_force() calls for some reason
on the same skb, only the first one will return false.

Re: [PATCH net] net: make skb_dst_force return false when dst was cleared

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > -static inline void skb_dst_force(struct sk_buff *skb)
> > +static inline bool skb_dst_force(struct sk_buff *skb)
> >  {
> >  	if (skb_dst_is_noref(skb)) {
> >  		struct dst_entry *dst = skb_dst(skb);
> > @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
> >  			dst = NULL;
> >  
> >  		skb->_skb_refdst = (unsigned long)dst;
> > +		return dst != NULL;
> >  	}
> > +
> > +	return true;
> 
> This will return true, even if skb has a NULL dst.

Yes, that was intentional -- it should return false to
let caller know that no reference could be obtained and
that the dst was invalidated as a result.

> Say if we have two skb_dst_force() calls for some reason
> on the same skb, only the first one will return false.

What would you suggest instead?

Alternative is something like

if (skb_dst(skb)) {
	skb_dst_force(skb);
	if (!skb_dst(skb)) {
		kfree_skb(skb);
		goto err;
	}
}

... i find this a bit ugly.

Re: [PATCH net] net: make skb_dst_force return false when dst was cleared

[Eric Dumazet]

On 6/25/19 12:59 PM, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> -static inline void skb_dst_force(struct sk_buff *skb)
>>> +static inline bool skb_dst_force(struct sk_buff *skb)
>>>  {
>>>  	if (skb_dst_is_noref(skb)) {
>>>  		struct dst_entry *dst = skb_dst(skb);
>>> @@ -313,7 +314,10 @@ static inline void skb_dst_force(struct sk_buff *skb)
>>>  			dst = NULL;
>>>  
>>>  		skb->_skb_refdst = (unsigned long)dst;
>>> +		return dst != NULL;
>>>  	}
>>> +
>>> +	return true;
>>
>> This will return true, even if skb has a NULL dst.
> 
> Yes, that was intentional -- it should return false to
> let caller know that no reference could be obtained and
> that the dst was invalidated as a result.

Problem is that some callers ignore skb_dst_force() return value.

> 
>> Say if we have two skb_dst_force() calls for some reason
>> on the same skb, only the first one will return false.
> 
> What would you suggest instead?
> 
> Alternative is something like
> 
> if (skb_dst(skb)) {
> 	skb_dst_force(skb);
> 	if (!skb_dst(skb)) {
> 		kfree_skb(skb);
> 		goto err;
> 	}
> }


Simply change 

return true;

by

return skb->_skb_refdst != 0UL;