| Message ID | 20191205181015.169651-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] tcp: md5: fix potential overestimation of TCP option space | expand |
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
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
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.
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;
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(-)