On Tue, May 10, 2022 at 12:18 PM Martin KaFai Lau [off-list ref] wrote:

On Tue, May 10, 2022 at 10:30:57AM -0700, Stanislav Fomichev wrote:

quoted

On Tue, May 10, 2022 at 12:13 AM Martin KaFai Lau [off-list ref] wrote:

quoted

On Mon, May 09, 2022 at 04:38:36PM -0700, Stanislav Fomichev wrote:

quoted

quoted

quoted

+unsigned int __cgroup_bpf_run_lsm_current(const void *ctx,
+                                       const struct bpf_insn *insn)
+{
+     const struct bpf_prog *shim_prog;
+     struct cgroup *cgrp;
+     int ret = 0;

From lsm_hook_defs.h, there are some default return values that are not 0.
Is it ok to always return 0 in cases like the cgroup array is empty ?

That's a good point, I haven't thought about it. You're right, it
seems like attaching to this hook for some LSMs will change the
default from some error to zero.
Let's start by prohibiting those hooks for now? I guess in theory,
when we generate a trampoline, we can put this default value as an
input arg to these new __cgroup_bpf_run_lsm_xxx helpers (in the
future)?

After looking at arch_prepare_bpf_trampoline, return 0 here should be fine.
If I read it correctly, when the shim_prog returns 0, the trampoline
will call the original kernel function which is the bpf_lsm_##NAME()
defined in bpf_lsm.c and it will then return the zero/-ve DEFAULT.

Not sure I read the same :-/ I'm assuming that for those cases we
actually end up generating fmod_ret trampoline which seems to be
unconditionally saving r0 into fp-8 ?

invoke_bpf_mod_ret() calls invoke_bpf_prog(..., true) that saves the r0.

Later, the "if (flags & BPF_TRAMP_F_CALL_ORIG)" will still
"/* call the original function */" and then stores the r0 retval
from the original function, no? or I mis-read something ?

I was under the wrong assumption this whole time that fmod_ret
programs run after the original one and the first bpf program sees the
output of the original one.
Turns out it's not the case; agreed that we already do the right
thing; thanks for pointing it out!

quoted

quoted

quoted

Another thing that seems to be related: there are a bunch of hooks
that return void, so returning EPERM from the cgroup programs won't
work as expected.
I can probably record, at verification time, whether lsm_cgroup
programs return any "non-success" return codes and prohibit attaching
these progs to the void hooks?

hmm...yeah, BPF_LSM_CGROUP can be enforced to return either 0 or 1 as
most other cgroup-progs do.

Do you have a use case that needs to return something other than -EPERM ?

We do already enforce 0/1 for cgroup progs (and we have helpers to
expose custom errno). What I want to avoid is letting users attach
programs that try to return the error for the void hooks. And it seems
like we record that return range for a particular cgroup program and
verify it at attach time, WDYT?

Make sense.  Do that in check_return_code() at load time instead of
attach time?
To be specific, meaning enforce BPF_LSM_CGROUP to 0/1 for int return type
and always 1 for void return type?

Yeah, let's try to enforce the following at load time:
- return 0 or call to bpf_set_retval should happen only for the hooks
that return int
- for the void ones, only 'return 1' should be accepted

Ah, I forgot there is a bpf_set_retval().  I assume we eventually want
to allow that for BPF_LSM_CGROUP later?  Once it is allowed,
the verifier should also reject bpf_set_retval() when the
attach_btf_id has a void return type?

Right, let me actually try to add bpf_set_retval to the set of allowed
helpers from the start, shouldn't be hard..