Message ID | 1471438586.4943.28.camel@edumazet-glaptop3.roam.corp.google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, Aug 17, 2016 at 8:56 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > > When tcp_sendmsg() allocates a fresh and empty skb, it puts it at the > tail of the write queue using tcp_add_write_queue_tail() > > Then it attempts to copy user data into this fresh skb. > > If the copy fails, we undo the work and remove the fresh skb. > > Unfortunately, this undo lacks the change done to tp->highest_sack and > we can leave a dangling pointer (to a freed skb) > > Later, tcp_xmit_retransmit_queue() can dereference this pointer and > access freed memory. For regular kernels where memory is not unmapped, > this might cause SACK bugs because tcp_highest_sack_seq() is buggy, > returning garbage instead of tp->snd_nxt, but with various debug > features like CONFIG_DEBUG_PAGEALLOC, this can crash the kernel. > > This bug was found by Marco Grassi thanks to syzkaller. > > Fixes: 6859d49475d4 ("[TCP]: Abstract tp->highest_sack accessing & point to next skb") > Reported-by: Marco Grassi <marco.gra@gmail.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> > Cc: Yuchung Cheng <ycheng@google.com> > Cc: Neal Cardwell <ncardwell@google.com> Acked-by: Neal Cardwell <ncardwell@google.com> Thanks, Eric. neal
On Wed, Aug 17, 2016 at 5:56 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > When tcp_sendmsg() allocates a fresh and empty skb, it puts it at the > tail of the write queue using tcp_add_write_queue_tail() > > Then it attempts to copy user data into this fresh skb. > > If the copy fails, we undo the work and remove the fresh skb. > > Unfortunately, this undo lacks the change done to tp->highest_sack and > we can leave a dangling pointer (to a freed skb) > > Later, tcp_xmit_retransmit_queue() can dereference this pointer and > access freed memory. For regular kernels where memory is not unmapped, > this might cause SACK bugs because tcp_highest_sack_seq() is buggy, > returning garbage instead of tp->snd_nxt, but with various debug > features like CONFIG_DEBUG_PAGEALLOC, this can crash the kernel. > > This bug was found by Marco Grassi thanks to syzkaller. > > Fixes: 6859d49475d4 ("[TCP]: Abstract tp->highest_sack accessing & point to next skb") > Reported-by: Marco Grassi <marco.gra@gmail.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> > Cc: Yuchung Cheng <ycheng@google.com> > Cc: Neal Cardwell <ncardwell@google.com> > --- > include/net/tcp.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index c00e7d51bb18..7717302cab91 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -1523,6 +1523,8 @@ static inline void tcp_check_send_head(struct sock *sk, struct sk_buff *skb_unli > { > if (sk->sk_send_head == skb_unlinked) > sk->sk_send_head = NULL; > + if (tcp_sk(sk)->highest_sack == skb_unlinked) > + tcp_sk(sk)->highest_sack = NULL; > } > Nit: the function name probably needs to change too, since it now checks more than just send_head. ;) But we can always do this for net-next, don't let this be a blocker for this security fix. Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com> Thanks!
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 17 Aug 2016 05:56:26 -0700 > From: Eric Dumazet <edumazet@google.com> > > When tcp_sendmsg() allocates a fresh and empty skb, it puts it at the > tail of the write queue using tcp_add_write_queue_tail() > > Then it attempts to copy user data into this fresh skb. > > If the copy fails, we undo the work and remove the fresh skb. > > Unfortunately, this undo lacks the change done to tp->highest_sack and > we can leave a dangling pointer (to a freed skb) > > Later, tcp_xmit_retransmit_queue() can dereference this pointer and > access freed memory. For regular kernels where memory is not unmapped, > this might cause SACK bugs because tcp_highest_sack_seq() is buggy, > returning garbage instead of tp->snd_nxt, but with various debug > features like CONFIG_DEBUG_PAGEALLOC, this can crash the kernel. > > This bug was found by Marco Grassi thanks to syzkaller. > > Fixes: 6859d49475d4 ("[TCP]: Abstract tp->highest_sack accessing & point to next skb") > Reported-by: Marco Grassi <marco.gra@gmail.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied and queued up for -stable.
diff --git a/include/net/tcp.h b/include/net/tcp.h index c00e7d51bb18..7717302cab91 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1523,6 +1523,8 @@ static inline void tcp_check_send_head(struct sock *sk, struct sk_buff *skb_unli { if (sk->sk_send_head == skb_unlinked) sk->sk_send_head = NULL; + if (tcp_sk(sk)->highest_sack == skb_unlinked) + tcp_sk(sk)->highest_sack = NULL; } static inline void tcp_init_send_head(struct sock *sk)