netdev - Re: [PATCH v9 bpf-next 01/17] bpf: align CAP_NET_ADMIN checks with bpf

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]

Date: Sun, 5 Nov 2023 14:33:20 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Andrii Nakryiko <andrii@...nel.org>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, paul@...l-moore.com, 
	brauner@...nel.org, linux-fsdevel@...r.kernel.org, 
	linux-security-module@...r.kernel.org, keescook@...omium.org, 
	kernel-team@...a.com, sargun@...gun.me
Subject: Re: [PATCH v9 bpf-next 01/17] bpf: align CAP_NET_ADMIN checks with
 bpf_capable() approach

On Sat, Nov 4, 2023 at 3:05 AM Andrii Nakryiko <andrii@...nel.org> 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@...nel.org>
> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>

Acked-by: Yafang Shao <laoar.shao@...il.com>

> ---
>  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