From: Andrew Kanner <andrew.kanner@gmail.com> To: bjorn@kernel.org, magnus.karlsson@intel.com, maciej.fijalkowski@intel.com, jonathan.lemon@gmail.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, aleksander.lobakin@intel.com, xuanzhuo@linux.alibaba.com, ast@kernel.org, hawk@kernel.org, john.fastabend@gmail.com, daniel@iogearbox.net Cc: linux-kernel-mentees@lists.linuxfoundation.org, netdev@vger.kernel.org, bpf@vger.kernel.org, linux-kernel@vger.kernel.org, syzbot+fae676d3cf469331fc89@syzkaller.appspotmail.com, syzbot+b132693e925cbbd89e26@syzkaller.appspotmail.com, Andrew Kanner <andrew.kanner@gmail.com> Subject: [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create() Date: Thu, 5 Oct 2023 22:35:49 +0300 [thread overview] Message-ID: <20231005193548.515-1-andrew.kanner@gmail.com> (raw) Syzkaller reported the following issue: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 2807 at mm/vmalloc.c:3247 __vmalloc_node_range (mm/vmalloc.c:3361) Modules linked in: CPU: 0 PID: 2807 Comm: repro Not tainted 6.6.0-rc2+ #12 Hardware name: Generic DT based system unwind_backtrace from show_stack (arch/arm/kernel/traps.c:258) show_stack from dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1)) dump_stack_lvl from __warn (kernel/panic.c:633 kernel/panic.c:680) __warn from warn_slowpath_fmt (./include/linux/context_tracking.h:153 kernel/panic.c:700) warn_slowpath_fmt from __vmalloc_node_range (mm/vmalloc.c:3361 (discriminator 3)) __vmalloc_node_range from vmalloc_user (mm/vmalloc.c:3478) vmalloc_user from xskq_create (net/xdp/xsk_queue.c:40) xskq_create from xsk_setsockopt (net/xdp/xsk.c:953 net/xdp/xsk.c:1286) xsk_setsockopt from __sys_setsockopt (net/socket.c:2308) __sys_setsockopt from ret_fast_syscall (arch/arm/kernel/entry-common.S:68) xskq_get_ring_size() uses struct_size() macro to safely calculate the size of struct xsk_queue and q->nentries of desc members. But the syzkaller repro was able to set q->nentries with the value initially taken from copy_from_sockptr() high enough to return SIZE_MAX by struct_size(). The next PAGE_ALIGN(size) is such case will overflow the size_t value and set it to 0. This will trigger WARN_ON_ONCE in vmalloc_user() -> __vmalloc_node_range(). The issue is reproducible on 32-bit arm kernel. Reported-and-tested-by: syzbot+fae676d3cf469331fc89@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000c84b4705fb31741e@google.com/T/ Link: https://syzkaller.appspot.com/bug?extid=fae676d3cf469331fc89 Reported-by: syzbot+b132693e925cbbd89e26@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000e20df20606ebab4f@google.com/T/ Fixes: 9f78bf330a66 ("xsk: support use vaddr as ring") Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com> --- Notes (akanner): v3: - free kzalloc-ed memory before return, the leak was noticed by Daniel Borkmann <daniel@iogearbox.net> v2: https://lore.kernel.org/all/20231002222939.1519-1-andrew.kanner@gmail.com/raw - use unlikely() optimization for the case with SIZE_MAX return from struct_size(), suggested by Alexander Lobakin <aleksander.lobakin@intel.com> - cc-ed 4 more maintainers, mentioned by cc_maintainers patchwork test v1: https://lore.kernel.org/all/20230928204440.543-1-andrew.kanner@gmail.com/T/ - RFC notes: It was found that net/xdp/xsk.c:xsk_setsockopt() uses copy_from_sockptr() to get the number of entries (int) for cases with XDP_RX_RING / XDP_TX_RING and XDP_UMEM_FILL_RING / XDP_UMEM_COMPLETION_RING. Next in xsk_init_queue() there're 2 sanity checks (entries == 0) and (!is_power_of_2(entries)) for which -EINVAL will be returned. After that net/xdp/xsk_queue.c:xskq_create() will calculate the size multipling the number of entries (int) with the size of u64, at least. I wonder if there should be the upper bound (e.g. the 3rd sanity check inside xsk_init_queue()). It seems that without the upper limit it's quiet easy to overflow the allocated size (SIZE_MAX), especially for 32-bit architectures, for example arm nodes which were used by the syzkaller. In this patch I added a naive check for SIZE_MAX which helped to skip zero-size allocation after overflow, but maybe it's not quite right. Please, suggest if you have any thoughts about the appropriate limit for the size of these xdp rings. PS: the initial number of entries is 0x20000000 in syzkaller repro: syscall(__NR_setsockopt, (intptr_t)r[0], 0x11b, 3, 0x20000040, 0x20); Link: https://syzkaller.appspot.com/text?tag=ReproC&x=10910f18280000 net/xdp/xsk_queue.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/net/xdp/xsk_queue.c b/net/xdp/xsk_queue.c index f8905400ee07..c7e8bbb12752 100644 --- a/net/xdp/xsk_queue.c +++ b/net/xdp/xsk_queue.c @@ -34,6 +34,11 @@ struct xsk_queue *xskq_create(u32 nentries, bool umem_queue) q->ring_mask = nentries - 1; size = xskq_get_ring_size(q, umem_queue); + if (unlikely(size == SIZE_MAX)) { + kfree(q); + return NULL; + } + size = PAGE_ALIGN(size); q->ring = vmalloc_user(size); -- 2.39.3
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Kanner <andrew.kanner@gmail.com> To: bjorn@kernel.org, magnus.karlsson@intel.com, maciej.fijalkowski@intel.com, jonathan.lemon@gmail.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, aleksander.lobakin@intel.com, xuanzhuo@linux.alibaba.com, ast@kernel.org, hawk@kernel.org, john.fastabend@gmail.com, daniel@iogearbox.net Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, syzbot+b132693e925cbbd89e26@syzkaller.appspotmail.com, bpf@vger.kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, syzbot+fae676d3cf469331fc89@syzkaller.appspotmail.com Subject: [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create() Date: Thu, 5 Oct 2023 22:35:49 +0300 [thread overview] Message-ID: <20231005193548.515-1-andrew.kanner@gmail.com> (raw) Syzkaller reported the following issue: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 2807 at mm/vmalloc.c:3247 __vmalloc_node_range (mm/vmalloc.c:3361) Modules linked in: CPU: 0 PID: 2807 Comm: repro Not tainted 6.6.0-rc2+ #12 Hardware name: Generic DT based system unwind_backtrace from show_stack (arch/arm/kernel/traps.c:258) show_stack from dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1)) dump_stack_lvl from __warn (kernel/panic.c:633 kernel/panic.c:680) __warn from warn_slowpath_fmt (./include/linux/context_tracking.h:153 kernel/panic.c:700) warn_slowpath_fmt from __vmalloc_node_range (mm/vmalloc.c:3361 (discriminator 3)) __vmalloc_node_range from vmalloc_user (mm/vmalloc.c:3478) vmalloc_user from xskq_create (net/xdp/xsk_queue.c:40) xskq_create from xsk_setsockopt (net/xdp/xsk.c:953 net/xdp/xsk.c:1286) xsk_setsockopt from __sys_setsockopt (net/socket.c:2308) __sys_setsockopt from ret_fast_syscall (arch/arm/kernel/entry-common.S:68) xskq_get_ring_size() uses struct_size() macro to safely calculate the size of struct xsk_queue and q->nentries of desc members. But the syzkaller repro was able to set q->nentries with the value initially taken from copy_from_sockptr() high enough to return SIZE_MAX by struct_size(). The next PAGE_ALIGN(size) is such case will overflow the size_t value and set it to 0. This will trigger WARN_ON_ONCE in vmalloc_user() -> __vmalloc_node_range(). The issue is reproducible on 32-bit arm kernel. Reported-and-tested-by: syzbot+fae676d3cf469331fc89@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000c84b4705fb31741e@google.com/T/ Link: https://syzkaller.appspot.com/bug?extid=fae676d3cf469331fc89 Reported-by: syzbot+b132693e925cbbd89e26@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/000000000000e20df20606ebab4f@google.com/T/ Fixes: 9f78bf330a66 ("xsk: support use vaddr as ring") Signed-off-by: Andrew Kanner <andrew.kanner@gmail.com> --- Notes (akanner): v3: - free kzalloc-ed memory before return, the leak was noticed by Daniel Borkmann <daniel@iogearbox.net> v2: https://lore.kernel.org/all/20231002222939.1519-1-andrew.kanner@gmail.com/raw - use unlikely() optimization for the case with SIZE_MAX return from struct_size(), suggested by Alexander Lobakin <aleksander.lobakin@intel.com> - cc-ed 4 more maintainers, mentioned by cc_maintainers patchwork test v1: https://lore.kernel.org/all/20230928204440.543-1-andrew.kanner@gmail.com/T/ - RFC notes: It was found that net/xdp/xsk.c:xsk_setsockopt() uses copy_from_sockptr() to get the number of entries (int) for cases with XDP_RX_RING / XDP_TX_RING and XDP_UMEM_FILL_RING / XDP_UMEM_COMPLETION_RING. Next in xsk_init_queue() there're 2 sanity checks (entries == 0) and (!is_power_of_2(entries)) for which -EINVAL will be returned. After that net/xdp/xsk_queue.c:xskq_create() will calculate the size multipling the number of entries (int) with the size of u64, at least. I wonder if there should be the upper bound (e.g. the 3rd sanity check inside xsk_init_queue()). It seems that without the upper limit it's quiet easy to overflow the allocated size (SIZE_MAX), especially for 32-bit architectures, for example arm nodes which were used by the syzkaller. In this patch I added a naive check for SIZE_MAX which helped to skip zero-size allocation after overflow, but maybe it's not quite right. Please, suggest if you have any thoughts about the appropriate limit for the size of these xdp rings. PS: the initial number of entries is 0x20000000 in syzkaller repro: syscall(__NR_setsockopt, (intptr_t)r[0], 0x11b, 3, 0x20000040, 0x20); Link: https://syzkaller.appspot.com/text?tag=ReproC&x=10910f18280000 net/xdp/xsk_queue.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/net/xdp/xsk_queue.c b/net/xdp/xsk_queue.c index f8905400ee07..c7e8bbb12752 100644 --- a/net/xdp/xsk_queue.c +++ b/net/xdp/xsk_queue.c @@ -34,6 +34,11 @@ struct xsk_queue *xskq_create(u32 nentries, bool umem_queue) q->ring_mask = nentries - 1; size = xskq_get_ring_size(q, umem_queue); + if (unlikely(size == SIZE_MAX)) { + kfree(q); + return NULL; + } + size = PAGE_ALIGN(size); q->ring = vmalloc_user(size); -- 2.39.3 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next reply other threads:[~2023-10-05 19:49 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-10-05 19:35 Andrew Kanner [this message] 2023-10-05 19:35 ` [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create() Andrew Kanner 2023-10-06 0:40 ` patchwork-bot+netdevbpf 2023-10-06 0:40 ` patchwork-bot+netdevbpf 2023-10-06 1:00 ` Martin KaFai Lau 2023-10-06 1:00 ` Martin KaFai Lau 2023-10-06 7:09 ` Andrew Kanner 2023-10-06 7:09 ` Andrew Kanner 2023-10-06 17:37 ` Martin KaFai Lau 2023-10-06 17:37 ` Martin KaFai Lau 2023-10-06 23:24 ` Andrew Kanner 2023-10-06 23:24 ` Andrew Kanner 2023-10-06 23:58 ` Martin KaFai Lau 2023-10-06 23:58 ` Martin KaFai Lau 2023-10-07 6:56 ` Andrew Kanner 2023-10-07 6:56 ` Andrew Kanner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20231005193548.515-1-andrew.kanner@gmail.com \ --to=andrew.kanner@gmail.com \ --cc=aleksander.lobakin@intel.com \ --cc=ast@kernel.org \ --cc=bjorn@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=hawk@kernel.org \ --cc=john.fastabend@gmail.com \ --cc=jonathan.lemon@gmail.com \ --cc=kuba@kernel.org \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maciej.fijalkowski@intel.com \ --cc=magnus.karlsson@intel.com \ --cc=netdev@vger.kernel.org \ --cc=pabeni@redhat.com \ --cc=syzbot+b132693e925cbbd89e26@syzkaller.appspotmail.com \ --cc=syzbot+fae676d3cf469331fc89@syzkaller.appspotmail.com \ --cc=xuanzhuo@linux.alibaba.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.