netdev - Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <87imiqm27d.fsf@toke.dk>
Date:   Fri, 27 Mar 2020 13:06:46 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        "David S. Miller" <davem@...emloft.net>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Andrey Ignatov <rdna@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> On Thu, Mar 26, 2020 at 01:35:13PM +0100, Toke Høiland-Jørgensen wrote:
>> 
>> Additionally, in the case where there is *not* a central management
>> daemon (i.e., what I'm implementing with libxdp), this would be the flow
>> implemented by the library without bpf_link:
>> 
>> 1. Query kernel for current BPF prog loaded on $IFACE
>> 2. Sanity-check that this program is a dispatcher program installed by
>>    libxdp
>> 3. Create a new dispatcher program with whatever changes we want to do
>>    (such as adding another component program).
>> 4. Atomically replace the old program with the new one using the netlink
>>    API in this patch series.
>
> in this model what stops another application that is not using libdispatcher to
> nuke dispatcher program ?

Nothing. But nothing is stopping it from issuing 'ip link down' either -
an application with CAP_NET_ADMIN is implicitly trusted to be
well-behaved. This patch series is just adding the kernel primitive that
enables applications to be well-behaved. I consider it an API bug-fix.

>> Whereas with bpf_link, it would be:
>> 
>> 1. Find the pinned bpf_link for $IFACE (e.g., load from
>>    /sys/fs/bpf/iface-links/$IFNAME).
>> 2. Query kernel for current BPF prog linked to $LINK
>> 3. Sanity-check that this program is a dispatcher program installed by
>>    libxdp
>> 4. Create a new dispatcher program with whatever changes we want to do
>>    (such as adding another component program).
>> 5. Atomically replace the old program with the new one using the
>>    LINK_UPDATE bpf() API.
>
> whereas here dispatcher program is only accessible to libdispatcher.
> Instance of bpffs needs to be known to libdispatcher only.
> That's the ownership I've been talking about.
>
> As discussed early we need a way for _human_ to nuke dispatcher program,
> but such api shouldn't be usable out of application/task.

As long as there is this kind of override in place, I'm not actually
fundamentally opposed to the concept of bpf_link for XDP, as an
additional mechanism. What I'm opposed to is using bpf_link as a reason
to block this series.

In fact, a way to implement the "human override" you mention, could be
to reuse the mechanism implemented in this series: If the EXPECTED_FD
passed via netlink is a bpf_link FD, that could be interpreted as an
override by the kernel.

-Toke