Re: [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create()

From: Andrew Kanner <andrew.kanner@gmail.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
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,
	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
Subject: Re: [PATCH bpf v3] net/xdp: fix zero-size allocation warning in xskq_create()
Date: Sat, 7 Oct 2023 02:24:05 +0300	[thread overview]
Message-ID: <6520971d.a70a0220.758e3.8cf7@mx.google.com> (raw)
In-Reply-To: <57c35480-983d-2056-1d72-f6e555069b83@linux.dev>

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