Re: [PATCH bpf-next v2] tools: bpftool: make capability check account for new BPF caps

From: Daniel Borkmann <daniel@iogearbox.net>
To: Quentin Monnet <quentin@isovalent.com>,
	Alexei Starovoitov <ast@kernel.org>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>
Subject: Re: [PATCH bpf-next v2] tools: bpftool: make capability check account for new BPF caps
Date: Tue, 26 May 2020 00:20:44 +0200	[thread overview]
Message-ID: <e9dbf916-1c22-29f4-77ef-2a587eca6f2a@iogearbox.net> (raw)
In-Reply-To: <20200523010247.20654-1-quentin@isovalent.com>

On 5/23/20 3:02 AM, Quentin Monnet wrote:
> Following the introduction of CAP_BPF, and the switch from CAP_SYS_ADMIN
> to other capabilities for various BPF features, update the capability
> checks (and potentially, drops) in bpftool for feature probes. Because
> bpftool and/or the system might not know of CAP_BPF yet, some caution is
> necessary:
> 
> - If compiled and run on a system with CAP_BPF, check CAP_BPF,
>    CAP_SYS_ADMIN, CAP_PERFMON, CAP_NET_ADMIN.
> 
> - Guard against CAP_BPF being undefined, to allow compiling bpftool from
>    latest sources on older systems. If the system where feature probes
>    are run does not know of CAP_BPF, stop checking after CAP_SYS_ADMIN,
>    as this should be the only capability required for all the BPF
>    probing.
> 
> - If compiled from latest sources on a system without CAP_BPF, but later
>    executed on a newer system with CAP_BPF knowledge, then we only test
>    CAP_SYS_ADMIN. Some probes may fail if the bpftool process has
>    CAP_SYS_ADMIN but misses the other capabilities. The alternative would
>    be to redefine the value for CAP_BPF in bpftool, but this does not
>    look clean, and the case sounds relatively rare anyway.
> 
> Note that libcap offers a cap_to_name() function to retrieve the name of
> a given capability (e.g. "cap_sys_admin"). We do not use it because
> deriving the names from the macros looks simpler than using
> cap_to_name() (doing a strdup() on the string) + cap_free() + handling
> the case of failed allocations, when we just want to use the name of the
> capability in an error message.
> 
> The checks when compiling without libcap (i.e. root versus non-root) are
> unchanged.
> 
> v2:
> - Do not allocate cap_list dynamically.
> - Drop BPF-related capabilities when running with "unprivileged", even
>    if we didn't have the full set in the first place (in v1, we would
>    skip dropping them in that case).
> - Keep track of what capabilities we have, print the names of the
>    missing ones for privileged probing.
> - Attempt to drop only the capabilities we actually have.
> - Rename a couple variables.
> 
> Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Applied, thanks!