[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6521010e.170a0220.1f5b5.aaa6@mx.google.com>
Date: Sat, 7 Oct 2023 09:56: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 04:58:18PM -0700, Martin KaFai Lau wrote:
> On 10/6/23 4:24 PM, Andrew Kanner wrote:
> > > 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.
>
> All make sense. I was mostly asking to add a comment at the "if
> (unlikely(size == SIZE_MAX)" check to explain this details on why checking
> SIZE_MAX is enough.
Ok, I got it. Will add in v4.
Thanks.
--
Andrew Kanner
Powered by blists - more mailing lists