On Thu, 28 Nov 2024 at 03:01, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Thu, 2024-11-28 at 02:57 +0100, Kumar Kartikeya Dwivedi wrote:
> > On Thu, 28 Nov 2024 at 02:50, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > >
> > > On Wed, 2024-11-27 at 13:20 -0800, Kumar Kartikeya Dwivedi wrote:
> > > > Ensure that when CAP_PERFMON is dropped, and the verifier sees
> > > > allow_ptr_leaks as false, we are not permitted to read from a
> > > > STACK_INVALID slot. Without the fix, the test will report unexpected
> > > > success in loading.
> > > >
> > > > Since we need to control the capabilities when loading this test to only
> > > > retain CAP_BPF, refactor support added to do the same for
> > > > test_verifier_mtu and reuse it for this selftest to avoid copy-paste.
> > > >
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > > ---
> > > > .../selftests/bpf/prog_tests/verifier.c | 41 ++++++++++++++++---
> > > > .../bpf/progs/verifier_stack_noperfmon.c | 21 ++++++++++
> > > > 2 files changed, 56 insertions(+), 6 deletions(-)
> > > > create mode 100644 tools/testing/selftests/bpf/progs/verifier_stack_noperfmon.c
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > index d9f65adb456b..aaf4324e8ef0 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> > > > @@ -63,6 +63,7 @@
> > > > #include "verifier_prevent_map_lookup.skel.h"
> > > > #include "verifier_private_stack.skel.h"
> > > > #include "verifier_raw_stack.skel.h"
> > > > +#include "verifier_stack_noperfmon.skel.h"
> > > > #include "verifier_raw_tp_writable.skel.h"
> > > > #include "verifier_reg_equal.skel.h"
> > > > #include "verifier_ref_tracking.skel.h"
> > > > @@ -226,22 +227,50 @@ void test_verifier_xdp_direct_packet_access(void) { RUN(verifier_xdp_direct_pack
> > > > void test_verifier_bits_iter(void) { RUN(verifier_bits_iter); }
> > > > void test_verifier_lsm(void) { RUN(verifier_lsm); }
> > > >
> > > > -void test_verifier_mtu(void)
> > > > +static int test_verifier_disable_caps(__u64 *caps)
> > >
> > > The original thread [0] discusses __caps_unpriv macro.
> > > I'd prefer such macro over these changes to prog_tests/verifier.c,
> > > were there any technical problems with code suggested in [0]?
> > >
> > > [0] https://lore.kernel.org/bpf/a1e48f5d9ae133e19adc6adf27e19d585e06bab4.camel@xxxxxxxxx/#t
> > >
> >
> > I think that patch worked as well, but I got to look at this now after
> > all these months, and concluded that
> > what Daniel did in
> > https://lore.kernel.org/bpf/20241021152809.33343-5-daniel@xxxxxxxxxxxxx
> > was also
> > acceptable and preferred.
> >
> > I can add your patch to this set and respin, or post a follow-up converting
> > test_verifier_mtu to it as well. Whatever is preferred.
>
> Patch #1 would need a respin because the comment for mark_stack_slot_misc() needs fixing.
> If you agree with adding __caps_unpriv, could you please make it a part of v3?
Should I leave STACK_INVALID as-is regardless of privilege, which
would basically remove the need to check allow_ptr_leaks in this
function? As you said, pruning considers them equivalent if
allow_uninit_stack and stack_read logic allows loading from invalid
slots, so upgrading to STACK_MISC isn't necessary.
>
> [...]
>
