[PATCH net] tcp: md5: fix potential overestimation of TCP option space

[PATCH net] tcp: md5: fix potential overestimation of TCP option space

[Eric Dumazet]

Back in 2008, Adam Langley fixed the corner case of packets for flows
having all of the following options : MD5 TS SACK

Since MD5 needs 20 bytes, and TS needs 12 bytes, no sack block
can be cooked from the remaining 8 bytes.

tcp_established_options() correctly sets opts->num_sack_blocks
to zero, but returns 36 instead of 32.

This means TCP cooks packets with 4 extra bytes at the end
of options, containing unitialized bytes.

Fixes: 33ad798c924b ("tcp: options clean up")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
 net/ipv4/tcp_output.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index be6d22b8190fa375074062032105879270af4be5..b184f03d743715ef4b2d166ceae651529be77953 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -755,8 +755,9 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb
 			min_t(unsigned int, eff_sacks,
 			      (remaining - TCPOLEN_SACK_BASE_ALIGNED) /
 			      TCPOLEN_SACK_PERBLOCK);
-		size += TCPOLEN_SACK_BASE_ALIGNED +
-			opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK;
+		if (likely(opts->num_sack_blocks))
+			size += TCPOLEN_SACK_BASE_ALIGNED +
+				opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK;
 	}
 
 	return size;
-- 
2.24.0.393.g34dc348eaf-goog

Re: [PATCH net] tcp: md5: fix potential overestimation of TCP option space

[Neal Cardwell]

On Thu, Dec 5, 2019 at 1:10 PM Eric Dumazet <edumazet@google.com> wrote:
>
> Back in 2008, Adam Langley fixed the corner case of packets for flows
> having all of the following options : MD5 TS SACK
>
> Since MD5 needs 20 bytes, and TS needs 12 bytes, no sack block
> can be cooked from the remaining 8 bytes.
>
> tcp_established_options() correctly sets opts->num_sack_blocks
> to zero, but returns 36 instead of 32.
>
> This means TCP cooks packets with 4 extra bytes at the end
> of options, containing unitialized bytes.
>
> Fixes: 33ad798c924b ("tcp: options clean up")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
> ---

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

Thanks, Eric!

neal

Re: [PATCH net] tcp: md5: fix potential overestimation of TCP option space

[Soheil Hassas Yeganeh]

On Fri, Dec 6, 2019 at 9:49 AM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Thu, Dec 5, 2019 at 1:10 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > Back in 2008, Adam Langley fixed the corner case of packets for flows
> > having all of the following options : MD5 TS SACK
> >
> > Since MD5 needs 20 bytes, and TS needs 12 bytes, no sack block
> > can be cooked from the remaining 8 bytes.
> >
> > tcp_established_options() correctly sets opts->num_sack_blocks
> > to zero, but returns 36 instead of 32.
> >
> > This means TCP cooks packets with 4 extra bytes at the end
> > of options, containing unitialized bytes.
> >
> > Fixes: 33ad798c924b ("tcp: options clean up")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > ---
>
> Acked-by: Neal Cardwell <ncardwell@google.com>

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

Thanks for the fix!

> Thanks, Eric!
>
> neal

Re: [PATCH net] tcp: md5: fix potential overestimation of TCP option space

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Thu,  5 Dec 2019 10:10:15 -0800

> Back in 2008, Adam Langley fixed the corner case of packets for flows
> having all of the following options : MD5 TS SACK
> 
> Since MD5 needs 20 bytes, and TS needs 12 bytes, no sack block
> can be cooked from the remaining 8 bytes.
> 
> tcp_established_options() correctly sets opts->num_sack_blocks
> to zero, but returns 36 instead of 32.
> 
> This means TCP cooks packets with 4 extra bytes at the end
> of options, containing unitialized bytes.
> 
> Fixes: 33ad798c924b ("tcp: options clean up")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>

Applied and queued up for -stable, thanks Eric.