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
| ||
|
Message-ID: <1390699105.27806.63.camel@edumazet-glaptop2.roam.corp.google.com> 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