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
| ||
|
Message-ID: <6520971d.a70a0220.758e3.8cf7@mx.google.com> Date: Sat, 7 Oct 2023 02:24:05 +0300 From: Andrew Kanner <andrew.kanner@...il.com> To: Martin KaFai Lau <martin.lau@...ux.dev> Cc: linux-kernel-mentees@...ts.linuxfoundation.org, netdev@...r.kernel.org, bpf@...r.kernel.org, linux-kernel@...r.kernel.org, syzbot+fae676d3cf469331fc89@...kaller.appspotmail.com, syzbot+b132693e925cbbd89e26@...kaller.appspotmail.com, bjorn@...nel.org, magnus.karlsson@...el.com, maciej.fijalkowski@...el.com, jonathan.lemon@...il.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, aleksander.lobakin@...el.com, xuanzhuo@...ux.alibaba.com, ast@...nel.org, hawk@...nel.org, john.fastabend@...il.com, daniel@...earbox.net Subject: Re: [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create() On Fri, Oct 06, 2023 at 10:37:44AM -0700, Martin KaFai Lau wrote: [...] > > > What if "size" is SIZE_MAX-1? Would it still overflow the PAGE_ALIGN below? > > > > > > > + kfree(q); > > > > + return NULL; > > > > + } > > > > + > > > > size = PAGE_ALIGN(size); > > > > q->ring = vmalloc_user(size); > > > > > > > I asked myself the same question before v1. E.g. thinking about the > > check: (size > SIZE_MAX - PAGE_SIZE + 1) > > > > But xskq_create() is called after the check for > > !is_power_of_2(entries) in xsk_init_queue(). So I tried the same > > reproducer and divided the (nentries) value by 2 in a loop - it hits > > either SIZE_MAX case or the normal cases without overflow (sometimes > > throwing vmalloc error complaining about size which exceed total pages > > in my arm setup). > > > > So I can't see a way size will be SIZE_MAX-1, etc. Correct me if I'm > > wrong, please. > > > > PS: In the output below the first 2 values of (nentries) hit SIZE_MAX > > Thanks for the explanation, so iiuc it means it will overflow the > struct_size() first because of the is_power_of_2(nentries) requirement? > Could you help adding some comment to explain? Thanks. > The overflow happens because there's no upper limit for nentries (userspace input). Let me add more context, e.g. from net/xdp/xsk.c: static int xsk_setsockopt(struct socket *sock, int level, int optname, sockptr_t optval, unsigned int optlen) { [...] if (copy_from_sockptr(&entries, optval, sizeof(entries))) return -EFAULT; [...] err = xsk_init_queue(entries, q, false); [...] } 'entries' is passed to xsk_init_queue() and there're 2 checks: for 0 and is_power_of_2() only, no upper bound check: static int xsk_init_queue(u32 entries, struct xsk_queue **queue, bool umem_queue) { struct xsk_queue *q; if (entries == 0 || *queue || !is_power_of_2(entries)) return -EINVAL; q = xskq_create(entries, umem_queue); if (!q) return -ENOMEM; [...] } The 'entries' value is next passed to struct_size() in net/xdp/xsk_queue.c. If it's large enough - SIZE_MAX will be returned. I'm not sure if some appropriate limit for the size of XDP_RX_RING / XDP_TX_RING and XDP_UMEM_FILL_RING / XDP_UMEM_COMPLETION_RING rings should be used. But anyway, vmalloc() will tell if it's not ok with the requested allocation size. -- Andrew Kanner
Powered by blists - more mailing lists