Re: [PATCH bpf-next 2/2] bpf: verifier: use check_sub_overflow() to check for subtraction overflows

[Shung-Hsi Yu <shung-hsi.yu@xxxxxxxx>]

On Mon, Jun 24, 2024 at 09:15:48AM GMT, Jiri Olsa wrote:
> On Sun, Jun 23, 2024 at 03:03:20PM +0800, Shung-Hsi Yu wrote:
> SNIP
> > @@ -13428,15 +13409,16 @@ static void scalar_min_max_sub(struct bpf_reg_state *dst_reg,
> >  	s64 smax_val = src_reg->smax_value;
> >  	u64 umin_val = src_reg->umin_value;
> >  	u64 umax_val = src_reg->umax_value;
> > +	s64 smin_cur, smax_cur;
> >  
> > -	if (signed_sub_overflows(dst_reg->smin_value, smax_val) ||
> > -	    signed_sub_overflows(dst_reg->smax_value, smin_val)) {
> > +	if (check_sub_overflow(dst_reg->smin_value, smax_val, &smin_cur) ||
> > +	    check_sub_overflow(dst_reg->smax_value, smin_val, &smax_cur)) {
> >  		/* Overflow possible, we know nothing */
> >  		dst_reg->smin_value = S64_MIN;
> >  		dst_reg->smax_value = S64_MAX;
> >  	} else {
> > -		dst_reg->smin_value -= smax_val;
> > -		dst_reg->smax_value -= smin_val;
> > +		dst_reg->smin_value = smin_cur;
> > +		dst_reg->smax_value = smax_cur;
> >  	}
> >  	if (dst_reg->umin_value < umax_val) {
> >  		/* Overflow possible, we know nothing */
> 
> could we use dst_reg->smin_* pointers directly as the sum pointer
> arguments in check_add_overflow ? ditto for the check_sub_overflow
> in the other change

Ah, yes. Didn't think of that.

	if (check_add_overflow(dst_reg->smin_value, smin_val, &dst_reg->smin_value) ||
	    check_add_overflow(dst_reg->smax_value, smax_val, &dst_reg->smax_value)) {
		dst_reg->smin_value = S64_MIN;
		dst_reg->smax_value = S64_MAX;
	}

Does look much cleaner, thanks. I'll go with this for v2 unless there's objection.

> ...