Re: [PATCH bpf-next 04/13] bpf: add register bounds sanity checks and sanitization

Andrii Nakryiko
2023-11-03T21:11:55Z

On Fri, Nov 3, 2023 at 10:56 AM Eduard Zingerman <eddyz87@gmail.com> 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@kernel.org>
> > ---
> > 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 …