On Wed, Aug 17, 2016 at 5:56 AM, Eric Dumazet <eric.duma...@gmail.com> wrote:
> From: Eric Dumazet <eduma...@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....@gmail.com>
> Signed-off-by: Eric Dumazet <eduma...@google.com>
> Cc: Ilpo Järvinen <ilpo.jarvi...@helsinki.fi>
> Cc: Yuchung Cheng <ych...@google.com>
> Cc: Neal Cardwell <ncardw...@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.wangc...@gmail.com>
Thanks!
