Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <6521010e.170a0220.1f5b5.aaa6@mx.google.com>
Date:   Sat, 7 Oct 2023 09:56:05 +0300
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()
References: <20231005193548.515-1-andrew.kanner@gmail.com>
 <7aa47549-5a95-22d7-1d03-ffdd251cec6d@linux.dev>
 <651fb2a8.c20a0220.8d6c3.0fd9@mx.google.com>
 <57c35480-983d-2056-1d72-f6e555069b83@linux.dev>
 <6520971d.a70a0220.758e3.8cf7@mx.google.com>
 <eb61966f-8666-80f6-1eab-c89bffe496b8@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <eb61966f-8666-80f6-1eab-c89bffe496b8@linux.dev>
Precedence: bulk

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