On Mon, 27 Apr 2020 21:01:14 +0200
Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
