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@xxxxxxxxx>
