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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 18 Apr 2018 22:28:39 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        netdev@...r.kernel.org
Subject: Re: [RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap
 with xdp_frame area

On 04/18/2018 07:53 PM, Jesper Dangaard Brouer wrote:
> On Wed, 18 Apr 2018 18:21:21 +0200
> Daniel Borkmann <daniel@...earbox.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@...hat.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.