lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANn89i+Oru1kXuL+o8p-JPRKJxC9Cc=oXe-002hPoP_Kxq79GQ@mail.gmail.com>
Date: Wed, 1 Nov 2023 05:23:39 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Dmitry Safonov <dima@...sta.com>
Cc: "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	syzbot <syzkaller@...glegroups.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>, 
	Francesco Ruggeri <fruggeri05@...il.com>, Salam Noureddine <noureddine@...sta.com>
Subject: Re: [PATCH net] net/tcp: fix possible out-of-bounds reads in tcp_hash_fail()

On Wed, Nov 1, 2023 at 4:59 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Wed, Nov 1, 2023 at 1:02 AM Dmitry Safonov <dima@...sta.com> wrote:
> >
> > Hi Eric,
> >
> > On 10/30/23 17:47, Dmitry Safonov wrote:
> > > On 10/30/23 08:45, Eric Dumazet wrote:
> > >> 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 <syzkaller@...glegroups.com>
> > >> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > >> Cc: Dmitry Safonov <dima@...sta.com>
> > >> Cc: Francesco Ruggeri <fruggeri@...sta.com>
> > >> Cc: David Ahern <dsahern@...nel.org>
> > >
> > > Thanks for fixing this,
> > > Reviewed-by: Dmitry Safonov <dima@...sta.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;"
> >
> > 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.
>
> 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__);              \


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++ = '.';                                             \
        *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__);              \

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ