[PATCH net 0/2] tcp: fix possible crash in tcp_v4_err()

[PATCH net 0/2] tcp: fix possible crash in tcp_v4_err()

[Eric Dumazet]

soukjin bae reported a crash in tcp_v4_err() that we
root caused to a missing initialization.

Second patch adds a sanity check in tcp_v4_err() to avoid
future potential problems. Ignoring an ICMP message
is probably better than crashing a machine.

Eric Dumazet (2):
  tcp: clear icsk_backoff in tcp_write_queue_purge()
  tcp: tcp_v4_err() should be more careful

 net/ipv4/tcp.c      | 2 +-
 net/ipv4/tcp_ipv4.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH net 1/2] tcp: clear icsk_backoff in tcp_write_queue_purge()

[Eric Dumazet]

soukjin bae reported a crash in tcp_v4_err() handling
ICMP_DEST_UNREACH after tcp_write_queue_head(sk)
returned a NULL pointer.

Current logic should have prevented this :

  if (seq != tp->snd_una  || !icsk->icsk_retransmits ||
      !icsk->icsk_backoff || fastopen)
      break;

Problem is the write queue might have been purged
and icsk_backoff has not been cleared.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: soukjin bae <soukjin.bae@samsung.com>
---
 net/ipv4/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 2079145a3b7c5f498af429c9a8289342e4421fca..cf3c5095c10e8e7e56621beae2f93c93de184489 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2528,6 +2528,7 @@ void tcp_write_queue_purge(struct sock *sk)
 	sk_mem_reclaim(sk);
 	tcp_clear_all_retrans_hints(tcp_sk(sk));
 	tcp_sk(sk)->packets_out = 0;
+	inet_csk(sk)->icsk_backoff = 0;
 }
 
 int tcp_disconnect(struct sock *sk, int flags)
@@ -2576,7 +2577,6 @@ int tcp_disconnect(struct sock *sk, int flags)
 	tp->write_seq += tp->max_window + 2;
 	if (tp->write_seq == 0)
 		tp->write_seq = 1;
-	icsk->icsk_backoff = 0;
 	tp->snd_cwnd = 2;
 	icsk->icsk_probes_out = 0;
 	tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH net 1/2] tcp: clear icsk_backoff in tcp_write_queue_purge()

[Neal Cardwell]

On Fri, Feb 15, 2019 at 4:36 PM Eric Dumazet <edumazet@google.com> wrote:
>
> soukjin bae reported a crash in tcp_v4_err() handling
> ICMP_DEST_UNREACH after tcp_write_queue_head(sk)
> returned a NULL pointer.
>
> Current logic should have prevented this :
>
>   if (seq != tp->snd_una  || !icsk->icsk_retransmits ||
>       !icsk->icsk_backoff || fastopen)
>       break;
>
> Problem is the write queue might have been purged
> and icsk_backoff has not been cleared.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: soukjin bae <soukjin.bae@samsung.com>
> ---
>  net/ipv4/tcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks!

neal

[PATCH net 2/2] tcp: tcp_v4_err() should be more careful

[Eric Dumazet]

ICMP handlers are not very often stressed, we should
make them more resilient to bugs that might surface in
the future.

If there is no packet in retransmit queue, we should
avoid a NULL deref.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: soukjin bae <soukjin.bae@samsung.com>
---
 net/ipv4/tcp_ipv4.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index efc6fef692ffdca4dcdd3f4b87a837656dd66c8c..ec3cea9d68288244d8e03b655d06f91640c36ee7 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -536,12 +536,15 @@ int tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
 		if (sock_owned_by_user(sk))
 			break;
 
+		skb = tcp_rtx_queue_head(sk);
+		if (WARN_ON_ONCE(!skb))
+			break;
+
 		icsk->icsk_backoff--;
 		icsk->icsk_rto = tp->srtt_us ? __tcp_set_rto(tp) :
 					       TCP_TIMEOUT_INIT;
 		icsk->icsk_rto = inet_csk_rto_backoff(icsk, TCP_RTO_MAX);
 
-		skb = tcp_rtx_queue_head(sk);
 
 		tcp_mstamp_refresh(tp);
 		delta_us = (u32)(tp->tcp_mstamp - tcp_skb_timestamp_us(skb));
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH net 2/2] tcp: tcp_v4_err() should be more careful

[Neal Cardwell]

On Fri, Feb 15, 2019 at 4:36 PM Eric Dumazet <edumazet@google.com> wrote:
>
> ICMP handlers are not very often stressed, we should
> make them more resilient to bugs that might surface in
> the future.
>
> If there is no packet in retransmit queue, we should
> avoid a NULL deref.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: soukjin bae <soukjin.bae@samsung.com>
> ---
>  net/ipv4/tcp_ipv4.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks!

neal

Re: [PATCH net 2/2] tcp: tcp_v4_err() should be more careful

[Soheil Hassas Yeganeh]

On Fri, Feb 15, 2019 at 9:21 PM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Fri, Feb 15, 2019 at 4:36 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > ICMP handlers are not very often stressed, we should
> > make them more resilient to bugs that might surface in
> > the future.
> >
> > If there is no packet in retransmit queue, we should
> > avoid a NULL deref.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Reported-by: soukjin bae <soukjin.bae@samsung.com>
> > ---
> >  net/ipv4/tcp_ipv4.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
>
> Acked-by: Neal Cardwell <ncardwell@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

Thanks you for the fix, Eric!

>
> Thanks!
>
> neal

Re: [PATCH net 0/2] tcp: fix possible crash in tcp_v4_err()

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Fri, 15 Feb 2019 13:36:19 -0800

> soukjin bae reported a crash in tcp_v4_err() that we
> root caused to a missing initialization.
> 
> Second patch adds a sanity check in tcp_v4_err() to avoid
> future potential problems. Ignoring an ICMP message
> is probably better than crashing a machine.

Series applied, thanks Eric.

Want me to queue these up for -stable?

Re: [PATCH net 0/2] tcp: fix possible crash in tcp_v4_err()

[Eric Dumazet]

On 02/17/2019 03:47 PM, David Miller wrote:
> From: Eric Dumazet <edumazet@google.com>
> Date: Fri, 15 Feb 2019 13:36:19 -0800
> 
>> soukjin bae reported a crash in tcp_v4_err() that we
>> root caused to a missing initialization.
>>
>> Second patch adds a sanity check in tcp_v4_err() to avoid
>> future potential problems. Ignoring an ICMP message
>> is probably better than crashing a machine.
> 
> Series applied, thanks Eric.
> 
> Want me to queue these up for -stable?
> 

Yes please, I put no Fixes: tag because it seemed to be a day-0 bug.

Thanks !

Re: [PATCH net 0/2] tcp: fix possible crash in tcp_v4_err()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 17 Feb 2019 18:20:21 -0800

> 
> 
> On 02/17/2019 03:47 PM, David Miller wrote:
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Fri, 15 Feb 2019 13:36:19 -0800
>> 
>>> soukjin bae reported a crash in tcp_v4_err() that we
>>> root caused to a missing initialization.
>>>
>>> Second patch adds a sanity check in tcp_v4_err() to avoid
>>> future potential problems. Ignoring an ICMP message
>>> is probably better than crashing a machine.
>> 
>> Series applied, thanks Eric.
>> 
>> Want me to queue these up for -stable?
>> 
> 
> Yes please, I put no Fixes: tag because it seemed to be a day-0 bug.

Done.