On Fri, May 15, 2020 at 5:52 PM Quentin Monnet <quentin@xxxxxxxxxxxxx> 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.
>
> Signed-off-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
> ---
> tools/bpf/bpftool/feature.c | 85 +++++++++++++++++++++++++++++--------
> 1 file changed, 67 insertions(+), 18 deletions(-)
>
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index 1b73e63274b5..3c3d779986c7 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -758,12 +758,32 @@ static void section_misc(const char *define_prefix, __u32 ifindex)
> print_end_section();
> }
>
> +#ifdef USE_LIBCAP
> +#define capability(c) { c, #c }
> +#endif
> +
> static int handle_perms(void)
> {
> #ifdef USE_LIBCAP
> - cap_value_t cap_list[1] = { CAP_SYS_ADMIN };
> - bool has_sys_admin_cap = false;
> + struct {
> + cap_value_t cap;
> + char name[14]; /* strlen("CAP_SYS_ADMIN") */
> + } required_caps[] = {
> + capability(CAP_SYS_ADMIN),
> +#ifdef CAP_BPF
> + /* Leave CAP_BPF in second position here: We will stop checking
> + * if the system does not know about it, since it probably just
> + * needs CAP_SYS_ADMIN to run all the probes in that case.
> + */
> + capability(CAP_BPF),
> + capability(CAP_NET_ADMIN),
> + capability(CAP_PERFMON),
> +#endif
> + };
> + bool has_admin_caps = true;
> + cap_value_t *cap_list;
> cap_flag_value_t val;
> + unsigned int i;
> int res = -1;
> cap_t caps;
>
> @@ -774,41 +794,70 @@ static int handle_perms(void)
> return -1;
> }
>
> - if (cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &val)) {
> - p_err("bug: failed to retrieve CAP_SYS_ADMIN status");
> + cap_list = malloc(sizeof(cap_value_t) * ARRAY_SIZE(required_caps));
I fail to see why you need to dynamically allocate cap_list?
cap_value_t cap_list[ARRAY_SIZE(required_caps)] wouldn't work?
> + if (!cap_list) {
> + p_err("failed to allocate cap_list: %s", strerror(errno));
> goto exit_free;
> }
> - if (val == CAP_SET)
> - has_sys_admin_cap = true;
>
> - if (!run_as_unprivileged && !has_sys_admin_cap) {
> - p_err("full feature probing requires CAP_SYS_ADMIN, run as root or use 'unprivileged'");
> - goto exit_free;
> + for (i = 0; i < ARRAY_SIZE(required_caps); i++) {
> + const char *cap_name = required_caps[i].name;
> + cap_value_t cap = required_caps[i].cap;
> +
> +#ifdef CAP_BPF
> + if (cap == CAP_BPF && !CAP_IS_SUPPORTED(cap))
> + /* System does not know about CAP_BPF, meaning
> + * that CAP_SYS_ADMIN is the only capability
> + * required. We already checked it, break.
> + */
> + break;
> +#endif
Seems more reliable to check all 4 capabilities independently (so
don't stop if !CAP_IS_SUPPORTED(cap)), and drop those that you have
set. Or there are some downsides to that?
> +
> + if (cap_get_flag(caps, cap, CAP_EFFECTIVE, &val)) {
> + p_err("bug: failed to retrieve %s status: %s", cap_name,
> + strerror(errno));
> + goto exit_free;
> + }
> +
> + if (val != CAP_SET) {
> + if (!run_as_unprivileged) {
> + p_err("missing %s, required for full feature probing; run as root or use 'unprivileged'",
> + cap_name);
> + goto exit_free;
> + }
> + has_admin_caps = false;
> + break;
> + }
> + cap_list[i] = cap;
> }
>
[...]
