netdev - Re: [PATCH RFC v2 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87v9m2nzqi.fsf@toke.dk>
Date:   Tue, 14 Apr 2020 12:11:33 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>, sameehj@...zon.com
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, zorik@...zon.com,
        akiyano@...zon.com, gtzalik@...zon.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>, brouer@...hat.com
Subject: Re: [PATCH RFC v2 29/33] xdp: allow bpf_xdp_adjust_tail() to grow packet size

Jesper Dangaard Brouer <brouer@...hat.com> writes:

> On Wed, 08 Apr 2020 13:53:01 +0200 Jesper Dangaard Brouer <brouer@...hat.com> wrote:
>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 7628b947dbc3..4d58a147eed0 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -3422,12 +3422,26 @@ 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;
>>  
> [...]
>> +	/* DANGER: ALL drivers MUST be converted to init xdp->frame_sz
>> +	 * - Adding some chicken checks below
>> +	 * - Will (likely) not be for upstream
>> +	 */
>> +	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;
>> +	}
>> +	if (unlikely(xdp->frame_sz > PAGE_SIZE)) {
>> +		WARN(1, "Too BIG xdp->frame_sz = %d\n", xdp->frame_sz);
>> +		return -EINVAL;
>> +	}
>
> Any opinions on above checks?
> Should they be removed or kept?
>
> The idea is to catch drivers that forgot to update xdp_buff->frame_sz,
> by doing some sanity checks on this uninit value.  If I correctly
> updated all XDP drivers in this patchset, then these checks should be
> unnecessary, but will this be valuable for driver developers converting
> new drivers to XDP to have these WARN checks?

Hmm, I wonder if there's a way we could have these kinds of checks
available, but disabled by default? A new macro (e.g.,
XDP_CHECK(condition)) that is only enabled when some debug option is
enabled in the kernel build, perhaps? Or just straight ifdef'ing them
out, but maybe a macro would be generally useful?

-Toke