Re: [RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap with xdp_frame area

[Daniel Borkmann]

On 04/18/2018 07:53 PM, Jesper Dangaard Brouer wrote:
> On Wed, 18 Apr 2018 18:21:21 +0200
> Daniel Borkmann  wrote:
> 
>> 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 
>>> ---
>>>  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)))
> 
> Okay, so you say just disallow using xdp_frame area in general.  It
> would be simpler.  

Yeah, lets do that.

> The advantage 

Re: [RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap with xdp_frame area

[Jesper Dangaard Brouer]

On Wed, 18 Apr 2018 18:21:21 +0200
Daniel Borkmann  wrote:

> 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 
> > ---
> >  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)))

Okay, so you say just disallow using xdp_frame area in general.  It
would be simpler.  

The advantage it that we don't run into strange situations, where the
user/bpf_prog increased headroom too much, such that 

Re: [RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap with xdp_frame area

[Daniel Borkmann]

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

[RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap with xdp_frame area

[Jesper Dangaard Brouer]

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.

Fixes: 6dfb970d3dbd ("xdp: avoid leaking info stored in frame data on page 
reuse")
Signed-off-by: Jesper Dangaard Brouer 
---
 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) {
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;
+
if (unlikely((metalen & (sizeof(__u32) - 1)) ||
 (metalen > 32)))
return -EACCES;