| Message ID | 158757178840.1370371.13037637865133257416.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [net-next,01/33] xdp: add frame size to xdp_buff | expand |
Jesper Dangaard Brouer <brouer@redhat.com> writes: > 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@redhat.com> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
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@redhat.com> > --- > include/uapi/linux/bpf.h | 4 ++-- > net/core/filter.c | 15 +++++++++++++-- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 2e29a671d67e..0e5abe991ca3 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1969,8 +1969,8 @@ union bpf_attr { > * int bpf_xdp_adjust_tail(struct xdp_buff *xdp_md, int delta) > * Description > * Adjust (move) *xdp_md*\ **->data_end** by *delta* bytes. It is > - * only possible to shrink the packet as of this writing, > - * therefore *delta* must be a negative integer. > + * possible to both shrink and grow the packet tail. > + * Shrink done via *delta* being a negative integer. > * > * A call to this helper is susceptible to change the underlying > * packet buffer. Therefore, at load time, all checks on pointers > 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; > + } > + 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. 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)? Thanks, Daniel > if (unlikely(data_end < xdp->data + ETH_HLEN)) > return -EINVAL; > > >
On Mon, 27 Apr 2020 21:01:14 +0200 Daniel Borkmann <daniel@iogearbox.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@redhat.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.
On 4/28/20 6:37 PM, Jesper Dangaard Brouer wrote: > On Mon, 27 Apr 2020 21:01:14 +0200 > Daniel Borkmann <daniel@iogearbox.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@redhat.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. Ok, makes sense, it's probably better that way. I do have a data_meta series for a few more drivers to push out soon to make sure there's more coverage as we're using it in Cilium.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 2e29a671d67e..0e5abe991ca3 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1969,8 +1969,8 @@ union bpf_attr { * int bpf_xdp_adjust_tail(struct xdp_buff *xdp_md, int delta) * Description * Adjust (move) *xdp_md*\ **->data_end** by *delta* bytes. It is - * only possible to shrink the packet as of this writing, - * therefore *delta* must be a negative integer. + * possible to both shrink and grow the packet tail. + * Shrink done via *delta* being a negative integer. * * A call to this helper is susceptible to change the underlying * packet buffer. Therefore, at load time, all checks on pointers 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; + } + if (unlikely(xdp->frame_sz > PAGE_SIZE)) { + WARN(1, "Too BIG xdp->frame_sz = %d\n", xdp->frame_sz); + return -EINVAL; + } + if (unlikely(data_end < xdp->data + ETH_HLEN)) return -EINVAL;
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@redhat.com> --- include/uapi/linux/bpf.h | 4 ++-- net/core/filter.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-)