[PATCH net] net/tcp: fix possible out-of-bounds reads in tcp_hash_fail()

[Eric Dumazet]

syzbot managed to trigger a fault by sending TCP packets
with all flags being set.

BUG: KASAN: stack-out-of-bounds in string_nocheck lib/vsprintf.c:645 [inline]
BUG: KASAN: stack-out-of-bounds in string+0x394/0x3d0 lib/vsprintf.c:727
Read of size 1 at addr ffffc9000397f3f5 by task syz-executor299/5039

CPU: 1 PID: 5039 Comm: syz-executor299 Not tainted 6.6.0-rc7-syzkaller-02075-g55c900477f5b #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:364 [inline]
print_report+0xc4/0x620 mm/kasan/report.c:475
kasan_report+0xda/0x110 mm/kasan/report.c:588
string_nocheck lib/vsprintf.c:645 [inline]
string+0x394/0x3d0 lib/vsprintf.c:727
vsnprintf+0xc5f/0x1870 lib/vsprintf.c:2818
vprintk_store+0x3a0/0xb80 kernel/printk/printk.c:2191
vprintk_emit+0x14c/0x5f0 kernel/printk/printk.c:2288
vprintk+0x7b/0x90 kernel/printk/printk_safe.c:45
_printk+0xc8/0x100 kernel/printk/printk.c:2332
tcp_inbound_hash.constprop.0+0xdb2/0x10d0 include/net/tcp.h:2760
tcp_v6_rcv+0x2b31/0x34d0 net/ipv6/tcp_ipv6.c:1882
ip6_protocol_deliver_rcu+0x33b/0x13d0 net/ipv6/ip6_input.c:438
ip6_input_finish+0x14f/0x2f0 net/ipv6/ip6_input.c:483
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ip6_input+0xce/0x440 net/ipv6/ip6_input.c:492
dst_input include/net/dst.h:461 [inline]
ip6_rcv_finish net/ipv6/ip6_input.c:79 [inline]
NF_HOOK include/linux/netfilter.h:314 [inline]
NF_HOOK include/linux/netfilter.h:308 [inline]
ipv6_rcv+0x563/0x720 net/ipv6/ip6_input.c:310
__netif_receive_skb_one_core+0x115/0x180 net/core/dev.c:5527
__netif_receive_skb+0x1f/0x1b0 net/core/dev.c:5641
netif_receive_skb_internal net/core/dev.c:5727 [inline]
netif_receive_skb+0x133/0x700 net/core/dev.c:5786
tun_rx_batched+0x429/0x780 drivers/net/tun.c:1579
tun_get_user+0x29e7/0x3bc0 drivers/net/tun.c:2002
tun_chr_write_iter+0xe8/0x210 drivers/net/tun.c:2048
call_write_iter include/linux/fs.h:1956 [inline]
new_sync_write fs/read_write.c:491 [inline]
vfs_write+0x650/0xe40 fs/read_write.c:584
ksys_write+0x12f/0x250 fs/read_write.c:637
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

Fixes: 2717b5adea9e ("net/tcp: Add tcp_hash_fail() ratelimited logs")
Reported-by: syzbot <syzk...@googlegroups.com>
Signed-off-by: Eric Dumazet <edum...@google.com>
Cc: Dmitry Safonov <di...@arista.com>
Cc: Francesco Ruggeri <frug...@arista.com>
Cc: David Ahern <dsa...@kernel.org>
---
include/net/tcp_ao.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index a375a171ef3cb37ab1d8246c72c6a3e83f5c9184..5daf96a3dbee14bd3786e19ea4972e351058e6e7 100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h
@@ -124,7 +124,7 @@ struct tcp_ao_info {
#define tcp_hash_fail(msg, family, skb, fmt, ...) \
do { \
const struct tcphdr *th = tcp_hdr(skb); \
- char hdr_flags[5] = {}; \
+ char hdr_flags[5]; \
char *f = hdr_flags; \
\
if (th->fin) \
@@ -135,8 +135,7 @@ do { \
*f++ = 'R'; \
if (th->ack) \
*f++ = 'A'; \
- if (f != hdr_flags) \
- *f = ' '; \
+ *f = 0; \
if ((family) == AF_INET) { \
net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d) %s" fmt "\n", \
msg, &ip_hdr(skb)->saddr, ntohs(th->source), \
--
2.42.0.820.g83a721a137-goog

[Dmitry Safonov]

Thanks for fixing this,
Reviewed-by: Dmitry Safonov <di...@arista.com>

> ---
> include/net/tcp_ao.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
> index a375a171ef3cb37ab1d8246c72c6a3e83f5c9184..5daf96a3dbee14bd3786e19ea4972e351058e6e7 100644
> --- a/include/net/tcp_ao.h
> +++ b/include/net/tcp_ao.h
> @@ -124,7 +124,7 @@ struct tcp_ao_info {
> #define tcp_hash_fail(msg, family, skb, fmt, ...) \
> do { \
> const struct tcphdr *th = tcp_hdr(skb); \
> - char hdr_flags[5] = {}; \
> + char hdr_flags[5]; \
> char *f = hdr_flags; \
> \
> if (th->fin) \
> @@ -135,8 +135,7 @@ do { \
> *f++ = 'R'; \
> if (th->ack) \
> *f++ = 'A'; \
> - if (f != hdr_flags) \
> - *f = ' '; \

Ah, that was clearly typo: I meant "*f = 0;"

> + *f = 0; \
> if ((family) == AF_INET) { \
> net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d) %s" fmt "\n", \
> msg, &ip_hdr(skb)->saddr, ntohs(th->source), \

Thank you,
Dmitry

[Dmitry Safonov]

Hi Eric,

Actually, after testing, I can see that the space was intended as well,
otherwise it "sticks" to L3index:
[ 130.965652] TCP: AO key not found for (10.0.1.1, 56920)->(10.0.254.1,
7010) Skeyid: 100 L3index: 0
[ 131.975116] TCP: AO hash is required, but not found for (10.0.1.1,
52686)->(10.0.254.1, 7011) SL3 index 0
[ 132.984024] TCP: AO hash mismatch for (10.0.1.1, 51382)->(10.0.254.1,
7012) SL3index: 0
[ 133.992221] TCP: Requested by the peer AO key id not found for
(10.0.1.1, 36548)->(10.0.254.1, 7013) SL3index: 0


If you don't mind, I'll send an updated version of your patch together
with some other small post-merge fixes this week.

[Eric Dumazet]

I will send a v2, adding the space in the format like this :

diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index 5daf96a3dbee14bd3786e19ea4972e351058e6e7..b90b4f8fb10fcbb31ddf65b825c098b215a91e67

100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h

@@ -137,7 +137,7 @@ do {
\
*f++ = 'A'; \

*f = 0; \
if ((family) == AF_INET) { \

- net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d)
%s" fmt "\n", \
+ net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d) %s

" fmt "\n", \
msg, &ip_hdr(skb)->saddr, ntohs(th->source), \

&ip_hdr(skb)->daddr, ntohs(th->dest), \
hdr_flags, ##__VA_ARGS__); \

[Eric Dumazet]

I will also change the format to use tcpdump one:

ACK is represented with a dot.

diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index b90b4f8fb10fcbb31ddf65b825c098b215a91e67..5ce1f8de0d77a1208bdbe4266e4bf2f76089f56c

100644
--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h

@@ -134,10 +134,10 @@ do {
\
if (th->rst) \

*f++ = 'R'; \
if (th->ack) \

- *f++ = 'A'; \
+ *f++ = '.'; \

[Eric Dumazet]

syzbot managed to trigger a fault by sending TCP packets
with all flags being set.

v2:
- While fixing this bug, add PSH flag handling and represent
flags the way tcpdump does : [S], [S.], [P.]
- Print 4-tuples more consistently between families.

---
include/net/tcp_ao.h | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/net/tcp_ao.h b/include/net/tcp_ao.h
index a375a171ef3cb37ab1d8246c72c6a3e83f5c9184..b56be10838f09a2cb56ab511242d2b583eb4c33b 100644

--- a/include/net/tcp_ao.h
+++ b/include/net/tcp_ao.h
@@ -124,7 +124,7 @@ struct tcp_ao_info {
#define tcp_hash_fail(msg, family, skb, fmt, ...) \
do { \
const struct tcphdr *th = tcp_hdr(skb); \
- char hdr_flags[5] = {}; \

+ char hdr_flags[6]; \

char *f = hdr_flags; \
\
if (th->fin) \

@@ -133,17 +133,18 @@ do { \
*f++ = 'S'; \

if (th->rst) \
*f++ = 'R'; \

+ if (th->psh) \
+ *f++ = 'P'; \

if (th->ack) \
- *f++ = 'A'; \

- if (f != hdr_flags) \
- *f = ' '; \

+ *f++ = '.'; \
+ *f = 0; \

if ((family) == AF_INET) { \
- net_info_ratelimited("%s for (%pI4, %d)->(%pI4, %d) %s" fmt "\n", \

+ net_info_ratelimited("%s for %pI4.%d->%pI4.%d [%s] " fmt "\n", \

msg, &ip_hdr(skb)->saddr, ntohs(th->source), \
&ip_hdr(skb)->daddr, ntohs(th->dest), \
hdr_flags, ##__VA_ARGS__); \

} else { \
- net_info_ratelimited("%s for [%pI6c]:%u->[%pI6c]:%u %s" fmt "\n", \
+ net_info_ratelimited("%s for [%pI6c].%d->[%pI6c].%d [%s]" fmt "\n", \
msg, &ipv6_hdr(skb)->saddr, ntohs(th->source), \
&ipv6_hdr(skb)->daddr, ntohs(th->dest), \
hdr_flags, ##__VA_ARGS__); \
--
2.42.0.820.g83a721a137-goog

[Dmitry Safonov]

LGTM, thanks again!

Reviewed-by: Dmitry Safonov <di...@arista.com>

Dmitry

[patchwork-b...@kernel.org]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <da...@davemloft.net>:

On Wed, 1 Nov 2023 04:52:33 +0000 you wrote:
> syzbot managed to trigger a fault by sending TCP packets
> with all flags being set.
>
> v2:
> - While fixing this bug, add PSH flag handling and represent
> flags the way tcpdump does : [S], [S.], [P.]
> - Print 4-tuples more consistently between families.
>

> [...]

Here is the summary with links:
- [v2,net] net/tcp: fix possible out-of-bounds reads in tcp_hash_fail()
https://git.kernel.org/netdev/net/c/02f0717e9835

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html