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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8483d4dc-1ef6-20b5-735f-8d78da579a28@gmail.com>
Date:   Tue, 25 Jun 2019 21:47:15 +0200
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: make skb_dst_force return false when dst was
 cleared



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@...len.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.