netdev - Re: [PATCH bpf-next V13 4/7] bpf: add BPF-helper for MTU checking

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-next>] [day] [month] [year] [list]

Date:   Sat, 30 Jan 2021 01:08:17 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        maze@...gle.com, lmb@...udflare.com, shaun@...era.io,
        Lorenzo Bianconi <lorenzo@...nel.org>, marek@...udflare.com,
        Jakub Kicinski <kuba@...nel.org>, eyal.birger@...il.com,
        colrack@...il.com
Subject: Re: [PATCH bpf-next V13 4/7] bpf: add BPF-helper for MTU checking

On 1/29/21 4:50 PM, John Fastabend wrote:
> Jesper Dangaard Brouer wrote:
>> On Thu, 28 Jan 2021 22:51:23 -0800
>> John Fastabend <john.fastabend@...il.com> wrote:
>>> Jesper Dangaard Brouer wrote:
>>>> This BPF-helper bpf_check_mtu() works for both XDP and TC-BPF programs.
>>>>
>>>> The SKB object is complex and the skb->len value (accessible from
>>>> BPF-prog) also include the length of any extra GRO/GSO segments, but
>>>> without taking into account that these GRO/GSO segments get added
>>>> transport (L4) and network (L3) headers before being transmitted. Thus,
>>>> this BPF-helper is created such that the BPF-programmer don't need to
>>>> handle these details in the BPF-prog.
>>>>
>>>> The API is designed to help the BPF-programmer, that want to do packet
>>>> context size changes, which involves other helpers. These other helpers
>>>> usually does a delta size adjustment. This helper also support a delta
>>>> size (len_diff), which allow BPF-programmer to reuse arguments needed by
>>>> these other helpers, and perform the MTU check prior to doing any actual
>>>> size adjustment of the packet context.
>>>>
>>>> It is on purpose, that we allow the len adjustment to become a negative
>>>> result, that will pass the MTU check. This might seem weird, but it's not
>>>> this helpers responsibility to "catch" wrong len_diff adjustments. Other
>>>> helpers will take care of these checks, if BPF-programmer chooses to do
>>>> actual size adjustment.
>>
>> The nitpick below about len adjust can become negative, is on purpose
>> and why is described in above.
> 
> following up on a nitpick :)
> 
> What is the use case to allow users to push a negative len_diff with
> abs(len_diff) > skb_diff and not throw an error. I would understand if it
> was a pain to catch the case, but below is fairly straightforward. Of
> course if user really tries to truncate the packet like this later it
> will also throw an error, but still missing why we don't throw an error
> here.
> 
> Anyways its undefined if len_diff is truely bogus. Its not really a
> problem I guess because garbage in (bogus len_diff) garbage out is OK I
> think.

What's the rationale to not sanity check for it? I just double checked
the UAPI helper description comment ... at minimum this behavior would
need to be documented there to avoid confusion.

> For the patch,
> 
> Acked-by: John Fastabend <john.fastabend@...il.com>
>