On 04/18/2018 02:10 PM, Jesper Dangaard Brouer wrote:
> If combining xdp_adjust_head and xdp_adjust_meta, then it is possible
> to make data_meta overlap with area used by xdp_frame. And another
> invocation of xdp_adjust_head can then clear that area, due to
> clearing of xdp_frame area.
>
> The easiest solution I found was to simply not allow
> xdp_buff->data_meta to overlap with area used by xdp_frame.
Thanks Jesper! Trying to answer both emails in one. :) More below.
> Fixes: 6dfb970d3dbd ("xdp: avoid leaking info stored in frame data on page
> reuse")
> Signed-off-by: Jesper Dangaard Brouer <bro...@redhat.com>
> ---
> net/core/filter.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 15e9b5477360..e3623e741181 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2701,6 +2701,11 @@ BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *,
> xdp, int, offset)
> data > xdp->data_end - ETH_HLEN))
> return -EINVAL;
>
> + /* Disallow data_meta to use xdp_frame area */
> + if (metalen > 0 &&
> + unlikely((data - metalen) < xdp_frame_end))
> + return -EINVAL;
> +
> /* Avoid info leak, when reusing area prev used by xdp_frame */
> if (data < xdp_frame_end) {
Effectively, when metalen > 0, then data_meta < data pointer, so above test
on new data_meta might be better, but feels like a bit of a workaround to
handle moving data pointer but disallowing moving data_meta pointer whereas
both could be handled if we wanted to go that path.
> unsigned long clearlen = xdp_frame_end - data;
> @@ -2734,6 +2739,7 @@ static const struct bpf_func_proto
> bpf_xdp_adjust_head_proto = {
>
> BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, xdp, int, offset)
> {
> + void *xdp_frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
> void *meta = xdp->data_meta + offset;
> unsigned long metalen = xdp->data - meta;
>
> @@ -2742,6 +2748,11 @@ BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *,
> xdp, int, offset)
> if (unlikely(meta < xdp->data_hard_start ||
> meta > xdp->data))
> return -EINVAL;
> +
> + /* Disallow data_meta to use xdp_frame area */
> + if (unlikely(meta < xdp_frame_end))
> + return -EINVAL;
(Ditto.)
> if (unlikely((metalen & (sizeof(__u32) - 1)) ||
> (metalen > 32)))
> return -EACCES;
The other, perhaps less invasive/complex option would be to just disallow
moving anything into previous sizeof(struct xdp_frame) area. My original
concern was that not all drivers use 256 bytes of headroom, e.g. afaik the
i40e and ixgbe have around 192 bytes of headroom available, but that should
actually still be plenty of space for encap + meta data, and potentially
with meta data use I would expect that at best custom decap would be
happening when pushing the packet up the stack. So might as well disallow
going into that region and not worry about it. Thus, reverting e9e9545e10d3
("xdp: avoid leaking info stored in frame data on page reuse") and adding
something like the below (uncompiled), should just do it:
diff --git a/net/core/filter.c b/net/core/filter.c
index 3bb0cb9..ad98ddd 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2692,8 +2692,9 @@ static unsigned long xdp_get_metalen(const struct
xdp_buff *xdp)
BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
{
+ void *frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
unsigned long metalen = xdp_get_metalen(xdp);
- void *data_start = xdp->data_hard_start + metalen;
+ void *data_start = frame_end + metalen;
void *data = xdp->data + offset;
if (unlikely(data < data_start ||
@@ -2719,13 +2720,13 @@ static const struct bpf_func_proto
bpf_xdp_adjust_head_proto = {
BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, xdp, int, offset)
{
+ void *frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
void *meta = xdp->data_meta + offset;
unsigned long metalen = xdp->data - meta;
if (xdp_data_meta_unsupported(xdp))
return -ENOTSUPP;
- if (unlikely(meta < xdp->data_hard_start ||
- meta > xdp->data))
+ if (unlikely(meta < frame_end || meta > xdp->data))
return -EINVAL;
if (unlikely((metalen & (sizeof(__u32) - 1)) ||
(metalen > 32)))
On top of that, we could even store a bool in struct xdp_rxq_info whether
the driver actually is able to participate in resp. has the XDP_REDIRECT
support and if not do something like:
void *frame_end = xdp->data_hard_start + xdp->rxq->has_redir ? sizeof(struct
xdp_frame) : 0;
But the latter is merely a small optimization. Eventually we want all native XDP
drivers to support it. Thoughts?
Thanks,
Daniel
