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

Eduard Zingerman
2023-11-03T21:39:12Z

On Fri, 2023-11-03 at 14:11 -0700, Andrii Nakryiko wrote:
[...]
> > 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.

Oh, right, sorry.

> 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.

tbh I think that compartmentalizing this check worth a little bit of
churn, but ok, not that important.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>