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/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;
