On 4/28/20 6:37 PM, Jesper Dangaard Brouer wrote:
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.
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.
