[net] tcp: fix use after free in tcp_xmit_retransmit_queue()

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(+)

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.