Re: [PATCH bpf-next 0/7] stricter register ID checking in regsafe()

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2022-12-13 at 16:34 -0800, Andrii Nakryiko wrote:
> On Fri, Dec 9, 2022 at 5:58 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > This patch-set consists of a series of bug fixes for register ID
> > tracking in verifier.c:states_equal()/regsafe() functions:
> >  - for registers of type PTR_TO_MAP_{KEY,VALUE}, PTR_TO_PACKET[_META]
> >    the regsafe() should call check_ids() even if registers are
> >    byte-to-byte equal;
> >  - states_equal() must maintain idmap that covers all function frames
> >    in the state because functions like mark_ptr_or_null_regs() operate
> >    on all registers in the state;
> >  - regsafe() must compare spin lock ids for PTR_TO_MAP_VALUE registers.
> > 
> > The last point covers issue reported by Kumar Kartikeya Dwivedi in [1],
> > I borrowed the test commit from there.
> > Note, that there is also an issue with register id tracking for
> > scalars described here [2], it would be addressed separately.
> > 
> 
> Awesome set of patches, thanks for working on this! I left a few
> comments and suggestions, please take a look, and if they do make
> sense, consider sending follow up patches.
> 
> Let's really try to use asm() next time for selftests, though.
> 
> It would be awesome to somehow automatically move test_verifier's
> tests to this test_progs-based embedded assembly way, but that
> probably takes some Python hackery (awesome project for some curious
> soul, for sure).
> 
> Anyways, back to the point I wanted to make. Given you've clearly
> thought about all the ID checks a lot, consider checking refsafe()
> (not regsafe()!) as well. I think we should do check_ids() there as
> well. And you did all the preliminary work with making idmap
> persistent across all frames. Just something to improve (and looks
> straightforward, unlike many other things you've dealt with recently
> ;).

Makes sense, I'll work on it.

> 
> Anyways, great work, thanks!

Thank you.

> 
> > [1] https://lore.kernel.org/bpf/20221111202719.982118-1-memxor@gmail.com/
> > [2] https://lore.kernel.org/bpf/20221128163442.280187-2-eddyz87@gmail.com/
> > 
> > Eduard Zingerman (6):
> >   bpf: regsafe() must not skip check_ids()
> >   selftests/bpf: test cases for regsafe() bug skipping check_id()
> >   bpf: states_equal() must build idmap for all function frames
> >   selftests/bpf: verify states_equal() maintains idmap across all frames
> >   bpf: use check_ids() for active_lock comparison
> >   selftests/bpf: test case for relaxed prunning of active_lock.id
> > 
> > Kumar Kartikeya Dwivedi (1):
> >   selftests/bpf: Add pruning test case for bpf_spin_lock
> > 
> >  include/linux/bpf_verifier.h                  |   4 +-
> >  kernel/bpf/verifier.c                         |  48 ++++----
> >  tools/testing/selftests/bpf/verifier/calls.c  |  82 +++++++++++++
> >  .../bpf/verifier/direct_packet_access.c       |  54 +++++++++
> >  .../selftests/bpf/verifier/spin_lock.c        | 114 ++++++++++++++++++
> >  .../selftests/bpf/verifier/value_or_null.c    |  49 ++++++++
> >  6 files changed, 324 insertions(+), 27 deletions(-)
> > 
> > --
> > 2.34.1
> >