On Sat, Nov 4, 2023 at 3:05 AM Andrii Nakryiko <andrii@xxxxxxxxxx> wrote:
>
> Within BPF syscall handling code CAP_NET_ADMIN checks stand out a bit
> compared to CAP_BPF and CAP_PERFMON checks. For the latter, CAP_BPF or
> CAP_PERFMON are checked first, but if they are not set, CAP_SYS_ADMIN
> takes over and grants whatever part of BPF syscall is required.
>
> Similar kind of checks that involve CAP_NET_ADMIN are not so consistent.
> One out of four uses does follow CAP_BPF/CAP_PERFMON model: during
> BPF_PROG_LOAD, if the type of BPF program is "network-related" either
> CAP_NET_ADMIN or CAP_SYS_ADMIN is required to proceed.
>
> But in three other cases CAP_NET_ADMIN is required even if CAP_SYS_ADMIN
> is set:
> - when creating DEVMAP/XDKMAP/CPU_MAP maps;
> - when attaching CGROUP_SKB programs;
> - when handling BPF_PROG_QUERY command.
>
> This patch is changing the latter three cases to follow BPF_PROG_LOAD
> model, that is allowing to proceed under either CAP_NET_ADMIN or
> CAP_SYS_ADMIN.
>
> This also makes it cleaner in subsequent BPF token patches to switch
> wholesomely to a generic bpf_token_capable(int cap) check, that always
> falls back to CAP_SYS_ADMIN if requested capability is missing.
>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
Acked-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> ---
> kernel/bpf/syscall.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 0ed286b8a0f0..ad4d8e433ccc 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1096,6 +1096,11 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
> return ret;
> }
>
> +static bool bpf_net_capable(void)
> +{
> + return capable(CAP_NET_ADMIN) || capable(CAP_SYS_ADMIN);
> +}
> +
> #define BPF_MAP_CREATE_LAST_FIELD map_extra
> /* called via syscall */
> static int map_create(union bpf_attr *attr)
> @@ -1199,7 +1204,7 @@ static int map_create(union bpf_attr *attr)
> case BPF_MAP_TYPE_DEVMAP:
> case BPF_MAP_TYPE_DEVMAP_HASH:
> case BPF_MAP_TYPE_XSKMAP:
> - if (!capable(CAP_NET_ADMIN))
> + if (!bpf_net_capable())
> return -EPERM;
> break;
> default:
> @@ -2599,7 +2604,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
> !bpf_capable())
> return -EPERM;
>
> - if (is_net_admin_prog_type(type) && !capable(CAP_NET_ADMIN) && !capable(CAP_SYS_ADMIN))
> + if (is_net_admin_prog_type(type) && !bpf_net_capable())
> return -EPERM;
> if (is_perfmon_prog_type(type) && !perfmon_capable())
> return -EPERM;
> @@ -3751,7 +3756,7 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog,
> case BPF_PROG_TYPE_SK_LOOKUP:
> return attach_type == prog->expected_attach_type ? 0 : -EINVAL;
> case BPF_PROG_TYPE_CGROUP_SKB:
> - if (!capable(CAP_NET_ADMIN))
> + if (!bpf_net_capable())
> /* cg-skb progs can be loaded by unpriv user.
> * check permissions at attach time.
> */
> @@ -3954,7 +3959,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
> static int bpf_prog_query(const union bpf_attr *attr,
> union bpf_attr __user *uattr)
> {
> - if (!capable(CAP_NET_ADMIN))
> + if (!bpf_net_capable())
> return -EPERM;
> if (CHECK_ATTR(BPF_PROG_QUERY))
> return -EINVAL;
> --
> 2.34.1
>
>
--
Regards
Yafang
