On Fri, Nov 3, 2023 at 10:56 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Thu, 2023-11-02 at 17:08 -0700, Andrii Nakryiko wrote: > > Add simple sanity checks that validate well-formed ranges (min <= max) > > across u64, s64, u32, and s32 ranges. Also for cases when the value is > > constant (either 64-bit or 32-bit), we validate that ranges and tnums > > are in agreement. > > > > These bounds checks are performed at the end of BPF_ALU/BPF_ALU64 > > operations, on conditional jumps, and for LDX instructions (where subreg > > zero/sign extension is probably the most important to check). This > > covers most of the interesting cases. > > > > Also, we validate the sanity of the return register when manually > > adjusting it for some special helpers. > > > > By default, sanity violation will trigger a warning in verifier log and > > resetting register bounds to "unbounded" ones. But to aid development > > and debugging, BPF_F_TEST_SANITY_STRICT flag is added, which will > > trigger hard failure of verification with -EFAULT on register bounds > > violations. This allows selftests to catch such issues. veristat will > > also gain a CLI option to enable this behavior. > > This is a useful check but I'm not sure about placement. > It might be useful to guard calls to coerce_subreg_to_size_sx() as well. Those are covered as part of the ALU/ALU64 check. My initial idea was to add it into reg_bounds_sync() and make reg_bounds_sync() return int (right now it's void). But discussing with Alexei we came to the conclusion that it would be a bit too much code churn for little gain. This coerce_subreg...() stuff, it's also void, so we'd need to propagate errors out of it as well. In the end I think I'm covering basically all relevant cases (ALU, LDX, RETVAL, COND_JUMP). > Maybe insert it as a part of the main do_check() loop but filter > by instruction class (and also force on stack_pop)? That would be a) a bit wasteful, and b) I'd need to re-interpret BPF_X vs BPF_K and all the other idiosyncrasies of instruction encoding. So it doesn't seem like a good idea. > > > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > --- > > include/linux/bpf_verifier.h | 1 + > > include/uapi/linux/bpf.h | 3 + > > kernel/bpf/syscall.c | 3 +- > > kernel/bpf/verifier.c | 117 ++++++++++++++++++++++++++------- > > tools/include/uapi/linux/bpf.h | 3 + > > 5 files changed, 101 insertions(+), 26 deletions(-) > > trimming is good [...]
