| Message ID | 20200516005149.3841-1-quentin@isovalent.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] tools: bpftool: make capability check account for new BPF caps | expand |
On Fri, May 15, 2020 at 5:52 PM Quentin Monnet <quentin@isovalent.com> 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@isovalent.com> > --- > 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; > } > [...]
2020-05-18 17:07 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Fri, May 15, 2020 at 5:52 PM Quentin Monnet <quentin@isovalent.com> 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@isovalent.com> >> --- >> 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? Oh I should have thought about that, thanks! I'll fix it. >> + 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_BPF is not supported, there is simply no point in going on checking the other capabilities, since CAP_SYS_ADMIN is the only one we need to do the feature probes. So in that case I see little point in checking the others. But if I understand your concern, you're right in the sense that the current code would consider a user as "unprivileged" if they do not have all four capabilities (in the case where CAP_BPF is supported); but they may still have a subset of them and not be completely unprivileged, and in that case we would have has_admin_caps at false and skip capabilities drop. I will fix that in next version. I am not sure about the advantage of keeping track of the capabilities and building a list just for dropping only the ones we have, but I can do that if you prefer. Thanks a lot for the review! Quentin
On Mon, May 18, 2020 at 6:03 PM Quentin Monnet <quentin@isovalent.com> wrote: > > 2020-05-18 17:07 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Fri, May 15, 2020 at 5:52 PM Quentin Monnet <quentin@isovalent.com> 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@isovalent.com> > >> --- > >> 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? > > Oh I should have thought about that, thanks! I'll fix it. > > >> + 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_BPF is not supported, there is simply no point in going on > checking the other capabilities, since CAP_SYS_ADMIN is the only one we > need to do the feature probes. So in that case I see little point in > checking the others. > > But if I understand your concern, you're right in the sense that the > current code would consider a user as "unprivileged" if they do not have > all four capabilities (in the case where CAP_BPF is supported); but they > may still have a subset of them and not be completely unprivileged, and > in that case we would have has_admin_caps at false and skip capabilities > drop. > > I will fix that in next version. I am not sure about the advantage of > keeping track of the capabilities and building a list just for dropping > only the ones we have, but I can do that if you prefer. > Honestly, I don't use bpftool feature at all, so I'm not very qualified to decide. I just like tools not making too many assumptions, where not necessary. So see for yourself :) > Thanks a lot for the review! > Quentin
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)); + 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 + + 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; } - if ((run_as_unprivileged && !has_sys_admin_cap) || - (!run_as_unprivileged && has_sys_admin_cap)) { + if ((run_as_unprivileged && !has_admin_caps) || + (!run_as_unprivileged && has_admin_caps)) { /* We are all good, exit now */ res = 0; goto exit_free; } - /* if (run_as_unprivileged && has_sys_admin_cap), drop CAP_SYS_ADMIN */ + /* if (run_as_unprivileged && has_admin_caps), drop capabilities. + * cap_set_flag() returns no error when trying to dump capabilities we + * do not have, so simply attempt to drop the whole list we have. + */ - if (cap_set_flag(caps, CAP_EFFECTIVE, ARRAY_SIZE(cap_list), cap_list, - CAP_CLEAR)) { - p_err("bug: failed to clear CAP_SYS_ADMIN from capabilities"); + if (cap_set_flag(caps, CAP_EFFECTIVE, ARRAY_SIZE(required_caps), + cap_list, CAP_CLEAR)) { + p_err("bug: failed to clear capabilities: %s", strerror(errno)); goto exit_free; } if (cap_set_proc(caps)) { - p_err("failed to drop CAP_SYS_ADMIN: %s", strerror(errno)); + p_err("failed to drop capabilities: %s", strerror(errno)); goto exit_free; } res = 0; exit_free: + free(cap_list); if (cap_free(caps) && !res) { p_err("failed to clear storage object for capabilities: %s", strerror(errno)); @@ -817,7 +866,7 @@ static int handle_perms(void) return res; #else - /* Detection assumes user has sufficient privileges (CAP_SYS_ADMIN). + /* Detection assumes user has specific privileges. * We do not use libpcap so let's approximate, and restrict usage to * root user only. */ @@ -901,7 +950,7 @@ static int do_probe(int argc, char **argv) } } - /* Full feature detection requires CAP_SYS_ADMIN privilege. + /* Full feature detection requires specific privileges. * Let's approximate, and warn if user is not root. */ if (handle_perms())
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@isovalent.com> --- tools/bpf/bpftool/feature.c | 85 +++++++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 18 deletions(-)