netdev - Re: [PATCH net-next 29/33] xdp: allow bpf_xdp_adjust

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:   Tue, 28 Apr 2020 21:36:35 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     sameehj@...zon.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
        zorik@...zon.com, akiyano@...zon.com, gtzalik@...zon.com,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        David Ahern <dsahern@...il.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        steffen.klassert@...unet.com
Subject: Re: [PATCH net-next 29/33] xdp: allow bpf_xdp_adjust_tail() to grow
 packet size

On 4/28/20 6:37 PM, Jesper Dangaard Brouer wrote:
> On Mon, 27 Apr 2020 21:01:14 +0200
> Daniel Borkmann <daniel@...earbox.net> 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@...hat.com>
>>> ---
>>>    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.