Subject: Re: [PATCH net-next 29/33] xdp: allow bpf_xdp_adjust_tail() to grow packet size

From: Jesper Dangaard Brouer <[email-masked]>
Date: Tue, 28 Apr 2020 18:37:43 +0200

On Mon, 27 Apr 2020 21:01:14 +0200
Daniel Borkmann <[email-masked]> 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 <[email-masked]>
> > ---
> >   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