netdev - Re: [PATCH bpf-next v3 04/16] inet: Run SK_LOOKUP BPF program on socket lookup

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: <87k0zgj378.fsf@cloudflare.com>
Date:   Mon, 06 Jul 2020 13:24:59 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Lorenz Bauer <lmb@...udflare.com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        kernel-team <kernel-team@...udflare.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Marek Majkowski <marek@...udflare.com>
Subject: Re: [PATCH bpf-next v3 04/16] inet: Run SK_LOOKUP BPF program on socket lookup

On Thu, Jul 02, 2020 at 03:19 PM CEST, Lorenz Bauer wrote:
> On Thu, 2 Jul 2020 at 13:46, Jakub Sitnicki <jakub@...udflare.com> wrote:
>>
>> On Thu, Jul 02, 2020 at 12:27 PM CEST, Lorenz Bauer wrote:
>> > On Thu, 2 Jul 2020 at 10:24, Jakub Sitnicki <jakub@...udflare.com> wrote:
>> >>
>> >> Run a BPF program before looking up a listening socket on the receive path.
>> >> Program selects a listening socket to yield as result of socket lookup by
>> >> calling bpf_sk_assign() helper and returning BPF_REDIRECT (7) code.
>> >>
>> >> Alternatively, program can also fail the lookup by returning with
>> >> BPF_DROP (1), or let the lookup continue as usual with BPF_OK (0) on
>> >> return. Other return values are treated the same as BPF_OK.
>> >
>> > I'd prefer if other values were treated as BPF_DROP, with other semantics
>> > unchanged. Otherwise we won't be able to introduce new semantics
>> > without potentially breaking user code.
>>
>> That might be surprising or even risky. If you attach a badly written
>> program that say returns a negative value, it will drop all TCP SYNs and
>> UDP traffic.
>
> I think if you do that all bets are off anyways. No use in trying to stagger on.
> Being stricter here will actually make it easier to for a developer to ensure
> that their program is doing the right thing.
>
> My point about future extensions also still stands.

We've chatted with Lorenz off-list about pros & cons of defaulting to
drop on illegal return code from a BPF program.

On the upside, it is consistent with XDP, SK_REUSEPORT, and SK_SKB
(sockmap) program types.

TC BPF ignores illegal return values, unspecified action means no
action, so no drop. While CGROUP_INET_INGRESS and SOCKET_FILTER look
only at the lowest bit ("ret & 1"), so it is a roulette.

Then there is also the extensibility argument. If we allow traffic to
pass to regular socket lookup on illegal return code from BPF, and users
start to rely on that, then it will be hard or impossible to repurpose
an illegal return value for something else.

Downside of defaulting to drop is that you can accidentally lock
yourself out, e.g. lose SSH access, by attaching a buggy program.

Being consistent with other existing program types is what convinces me
most to set default to drop, so I'll make the change in v4 unless there
are objections.

[...]