[RFC net-next PATCH 0/2] bpf: followup avoid leaking info stored in frame data on page reuse

[RFC net-next PATCH 0/2] bpf: followup avoid leaking info stored in frame data on page reuse

[Jesper Dangaard Brouer]

This is a followup to fix commit:
 6dfb970d3dbd ("xdp: avoid leaking info stored in frame data on page reuse")

Posting as RFC, as I want Daniel to review this before it goes in, as
Daniel usually have smarter/brighter ideas of howto solve this in a
more optimal manor?

---

Jesper Dangaard Brouer (2):
      bpf: avoid clear xdp_frame area again
      bpf: disallow XDP data_meta to overlap with xdp_frame area


 net/core/filter.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

[RFC net-next PATCH 1/2] bpf: avoid clear xdp_frame area again

[Jesper Dangaard Brouer]

Avoid clearing xdp_frame area if this was already done by prevous
invocations of bpf_xdp_adjust_head.

The xdp_adjust_head helper can be called multiple times by the
bpf_prog.  If increasing the packet header size (with a negative
offset), kernel must assume bpf_prog store valuable information here,
and not clear this information.

In case of extending header into xdp_frame area the kernel clear this
area to avoid any info leaking.

The bug in the current implementation is that if existing xdp->data
pointer have already been moved into xdp_frame area, then memory is
cleared between new-data pointer and xdp_frame-end, which covers an
area that might contain information store by BPF-prog (as curr
xdp->data lays between those pointers).

Fixes: 6dfb970d3dbd ("xdp: avoid leaking info stored in frame data on page reuse")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 net/core/filter.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index a374b8560bc4..15e9b5477360 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2705,6 +2705,13 @@ BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
 	if (data < xdp_frame_end) {
 		unsigned long clearlen = xdp_frame_end - data;
 
+		/* Handle if prev call adjusted xdp->data into xdp_frame area */
+		if (unlikely(xdp->data < xdp_frame_end)) {
+			if (data < xdp->data)
+				clearlen = xdp->data - data;
+			else
+				clearlen = 0;
+		}
 		memset(data, 0, clearlen);
 	}
 

[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 <brouer@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) {
 		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;

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

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 <daniel@iogearbox.net> 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 <brouer@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)))

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 convert_to_xdp_frame()
fails and thus XDP_REDIRECT action fails.  (That will be confusing to
users to debug/troubleshoot).


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

I would _really_ like see all drivers support XDP_REDIRECT, but to be
realistic, this is happening way too slow...

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

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 <daniel@iogearbox.net> 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 <brouer@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)))
> 
> Okay, so you say just disallow using xdp_frame area in general.  It
> would be simpler.  

Yeah, lets do that.

> The advantage it that we don't run into strange situations, where the
> user/bpf_prog increased headroom too much, such that convert_to_xdp_frame()
> fails and thus XDP_REDIRECT action fails.  (That will be confusing to
> users to debug/troubleshoot).

Agree, yet another argument for doing it.