On Tue, 2025-06-24 at 14:55 -0700, Alexei Starovoitov wrote:
> On Tue, Jun 24, 2025 at 12:10 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> >
> > Originally prog_tests/verifier.c was developed to run tests ported
> > from test_verifier binary. test_verifier runs tests with CAP_SYS_ADMIN
> > dropped, hence this behaviour was copied in prog_tests/verifier.c.
> > BPF_OBJ_GET_NEXT_ID BPF syscall command fails w/o CAP_SYS_ADMIN and
> > this prevents libbpf from loading module BTFs.
>
> You need this only because of 'bpf_kfunc_trusted_num_test' access
> in patch 4?
Yes.
> Can you use kernel kfunc instead?
Should be able to.
> This needs more thought.
> s/RUN/RUN_FULL_CAPS/ just because of kfunc in the bpf_testmod
> doesn't look like a good long term approach.
>
> I thought we agreed to relax BPF_OBJ_GET_NEXT_ID to allow for CAP_BPF.
> Probably even unpriv can do it.
> Just knowing a set of prog, map, bpf IDs is not a security threat.
>
> BPF_BTF_GET_FD_BY_ID can also be allowed for unpriv,
> since one can do it already from /sys/kernel/btf/
Makes sense to me.
> > This commit adds an optout from capability drop.
> >
> > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > ---
> > .../testing/selftests/bpf/prog_tests/verifier.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > index c9da06741104..cedb86d8f717 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > @@ -115,14 +115,16 @@ struct test_val {
> > __maybe_unused
> > static void run_tests_aux(const char *skel_name,
> > skel_elf_bytes_fn elf_bytes_factory,
> > - pre_execution_cb pre_execution_cb)
> > + pre_execution_cb pre_execution_cb,
> > + bool drop_sysadmin)
>
> I have an allergic reaction to bool arguments.
>
> > run_tests_aux("verifier_array_access",
> > verifier_array_access__elf_bytes,
> > - init_array_access_maps);
> > + init_array_access_maps,
> > + true);
>
> This is not readable without looking at the argument name.
I'll drop this change in v2.
