netdev - Re: [PATCH net-next 29/33] xdp: allow bpf_xdp_adjust

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200428183743.19dee96e@carbon>
Date:   Tue, 28 Apr 2020 18:37:43 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     sameehj@...zon.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
        zorik@...zon.com, akiyano@...zon.com, gtzalik@...zon.com,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        David Ahern <dsahern@...il.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        steffen.klassert@...unet.com, brouer@...hat.com
Subject: Re: [PATCH net-next 29/33] xdp: allow bpf_xdp_adjust_tail() to grow
 packet size

On Mon, 27 Apr 2020 21:01:14 +0200
Daniel Borkmann <daniel@...earbox.net> wrote:

> On 4/22/20 6:09 PM, Jesper Dangaard Brouer wrote:
> > Finally, after all drivers have a frame size, allow BPF-helper
> > bpf_xdp_adjust_tail() to grow or extend packet size at frame tail.
> > 
> > Remember that helper/macro xdp_data_hard_end have reserved some
> > tailroom.  Thus, this helper makes sure that the BPF-prog don't have
> > access to this tailroom area.
> > 
> > Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> > ---
> >   include/uapi/linux/bpf.h |    4 ++--
> >   net/core/filter.c        |   15 +++++++++++++--
> >   2 files changed, 15 insertions(+), 4 deletions(-)
> > 
[...]
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 7d6ceaa54d21..5e9c387f74eb 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -3422,12 +3422,23 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
> >   
> >   BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
> >   {
> > +	void *data_hard_end = xdp_data_hard_end(xdp);
> >   	void *data_end = xdp->data_end + offset;
> >   
> > -	/* only shrinking is allowed for now. */
> > -	if (unlikely(offset >= 0))
> > +	/* Notice that xdp_data_hard_end have reserved some tailroom */
> > +	if (unlikely(data_end > data_hard_end))
> >   		return -EINVAL;
> >   
> > +	/* ALL drivers MUST init xdp->frame_sz, some chicken checks below */
> > +	if (unlikely(xdp->frame_sz < (xdp->data_end - xdp->data_hard_start))) {
> > +		WARN(1, "Too small xdp->frame_sz = %d\n", xdp->frame_sz);
> > +		return -EINVAL;
> > +	}

I will remove this "too small" check, as it is useless, given it will
already get caught by above check.


> > +	if (unlikely(xdp->frame_sz > PAGE_SIZE)) {
> > +		WARN(1, "Too BIG xdp->frame_sz = %d\n", xdp->frame_sz);
> > +		return -EINVAL;
> > +	}  
> 
> I don't think we can add the WARN()s here. If there is a bug in the
> driver in this area and someone deploys an XDP-based application
> (otherwise known to work well elsewhere) on top of this, then an
> attacker can basically remote DoS the machine with malicious packets
> that end up triggering these WARN()s over and over.

Good point.  I've changed this to WARN_ONCE(), but I'm still
considering to remove it completely...

> If you are worried that not all your driver changes are correct,
> maybe only add those that you were able to actually test yourself or
> that have been acked, and otherwise pre-init the frame_sz to a known
> invalid value so this helper would only allow shrinking for them in
> here (as today)?

Hmm... no, I really want to require ALL drivers to set a valid value,
because else we will have the "data_meta" feature situation, where a lot
of drivers still doesn't support this.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer