Re: [PATCH v3 bpf-next 2/3] bpf: implement CAP_BPF

On Sep 4, 2019, at 8:15 PM, Alexei Starovoitov [off-list ref] wrote:

On Thu, Sep 05, 2019 at 02:51:51AM +0000, Song Liu wrote:

quoted

quoted

On Sep 4, 2019, at 6:32 PM, Alexei Starovoitov [off-list ref] wrote:

On Thu, Sep 05, 2019 at 12:34:36AM +0000, Song Liu wrote:

quoted

quoted

On Sep 4, 2019, at 11:43 AM, Alexei Starovoitov [off-list ref] wrote:

Implement permissions as stated in uapi/linux/capability.h

Signed-off-by: Alexei Starovoitov <ast@kernel.org>

[...]

quoted

@@ -1648,11 +1648,11 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)

	is_gpl = license_is_gpl_compatible(license);

	if (attr->insn_cnt == 0 ||
-	    attr->insn_cnt > (capable(CAP_SYS_ADMIN) ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
+	    attr->insn_cnt > (capable_bpf() ? BPF_COMPLEXITY_LIMIT_INSNS : BPF_MAXINSNS))
		return -E2BIG;
	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
	    type != BPF_PROG_TYPE_CGROUP_SKB &&
-	    !capable(CAP_SYS_ADMIN))
+	    !capable_bpf())
		return -EPERM;

Do we allow load BPF_PROG_TYPE_SOCKET_FILTER and BPF_PROG_TYPE_CGROUP_SKB
without CAP_BPF? If so, maybe highlight in the header?

of course. there is no change in behavior.
'highlight in the header'?
you mean in commit log?
I think it's a bit weird to describe things in commit that patch
is _not_ changing vs things that patch does actually change.
This type of comment would be great in a doc though.
The doc will be coming separately in the follow up assuming
the whole thing lands. I'll remember to note that bit.

I meant capability.h:

+ * CAP_BPF allows the following BPF operations:
+ * - Loading all types of BPF programs

But CAP_BPF is not required to load all types of programs. 

yes, but above statement is still correct, right?

And right below it says:
* CAP_BPF allows the following BPF operations:
* - Loading all types of BPF programs
* - Creating all types of BPF maps except:
*    - stackmap that needs CAP_TRACING
*    - devmap that needs CAP_NET_ADMIN
*    - cpumap that needs CAP_SYS_ADMIN
which is also correct, but CAP_BPF is not required
for array, hash, prog_array, percpu, map-in-map ...
except their lru variants...
and except if they contain bpf_spin_lock...
and if they need BTF it currently can be loaded with cap_sys_admin only...

If we say something about socket_filter, cg_skb progs in capability.h
we should clarify maps as well, but then it will become too big for .h
The comments in capability.h already look too long to me.
All that info and a lot more belongs in the doc.

Agreed. We cannot put all these details in capability.h. Doc/wikipages 
would be better fit for these information. 

Thanks,
Song