Date:	Sat, 25 Jan 2014 17:18:25 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Arnaud Ebalard <arno@...isbad.org>
Cc:	David Miller <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Daniel Borkmann <dborkman@...hat.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Willy Tarreau <w@....eu>, netdev@...r.kernel.org
Subject: Re: [BUG] null pointer dereference in tcp_gso_segment()

On Sun, 2014-01-26 at 00:54 +0100, Arnaud Ebalard wrote:

> Thanks for the explanation and sorry for the delay, I only just found
> the time to take a look at the code. For the discussion, a simplified
> version of tcp_gso_segment() is:
> 
> 
>   th = tcp_hdr(skb);
>   thlen = th->doff * 4;
> 
>   ...
> 
>   __skb_pull(skb, thlen);
> 
>   ...
> 
>   mss = tcp_skb_mss(skb);
>   if (unlikely(skb->len <= mss))
>  	goto out;
> 
>   ...
> 
>   segs = skb_segment(skb, features);
>   skb = segs;
> 
>   ...
> 
> 		skb = skb->next;
> 		th = tcp_hdr(skb);   <- bug occurs here
> 
> 
> So the logic seems to be that if we pass the mss test (i.e. skb->len >
> mss), then skb_segment() *should* indeed create at least two segments
> from the skb. I took a look at skb_segment() but the code is !trivial,
> i.e. it is not obvious that there is no way for the function to deliver
> a sk_buff skb w/ a NULL skb->next. Eric, I guess you or Herbert are
> familiar enough w/ the code to tell. But before checking that, your lead
> below is interesting ...
> 
> > Since TCP stack seemed to be the provider of the packet in your stack
> > trace, check tcp_set_skb_tso_segs()
> 
> It is indeed called in tcp_write_xmit() which appears in the
> backtrace. That function you point has an interesting property:
> 
>  static void tcp_set_skb_tso_segs(const struct sock *sk, struct sk_buff *skb,
>  				 unsigned int mss_now)
>  {
>  	/* Make sure we own this skb before messing gso_size/gso_segs */
>  	WARN_ON_ONCE(skb_cloned(skb));
>  
>  	if (skb->len <= mss_now || skb->ip_summed == CHECKSUM_NONE) {
>  		/* Avoid the costly divide in the normal
>  		 * non-TSO case.
>  		 */
>  		skb_shinfo(skb)->gso_segs = 1;
>  		skb_shinfo(skb)->gso_size = 0;
>  		skb_shinfo(skb)->gso_type = 0;
>  	} else {
>  		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss_now);
>  		skb_shinfo(skb)->gso_size = mss_now;
>  		skb_shinfo(skb)->gso_type = sk->sk_gso_type;
>  	}
>  }
>  
> If it is called with skb->len <= mss, the resulting skb will be modified
> so that you will then have skb_shinfo(skb)->gso_size set to 0,
> i.e. skb->len > skb_shinfo(skb)->gso_size.
> 

The key part is that gso_size is set to 0.

Meaning its not a gso packet :

static inline bool skb_is_gso(const struct sk_buff *skb)
{
        return skb_shinfo(skb)->gso_size;
}

So the packet cannot possibly go up to tcp_gso_segment() because
we fail the first test in :

static inline bool netif_needs_gso(struct sk_buff *skb,
                                   netdev_features_t features)
{
        return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
                unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
                         (skb->ip_summed != CHECKSUM_UNNECESSARY)));
}


> In tcp_gso_segment(), mss is grabbed using tcp_skb_mss() which simply
> returns skb_shinfo(skb)->gso_size. That function comes with a comment
> indicating that it provides the mss only when tcp_skb_pcount() > 1,
> i.e when skb_shinfo(skb)->gso_segs > 1. Said differently, one should
> never call tcp_skb_mss() after tcp_set_skb_tso_segs() has been called on
> a skb *unless* she tests explicitly that tcp_skb_pcount() > 1.
> 
> This test (tcp_skb_pcount() > 1) is not done in tcp_gso_segment() before
> getting the mss value w/ tcp_skb_mss(). I may have missed a test
> somewhere in a caller but I do not see why tcp_gso_segment() makes the
> assumption it can safely call tcp_skb_mss().


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists