Alexei Starovoitov wrote:
> On Thu, Jan 23, 2020 at 11:10:42PM -0800, John Fastabend wrote:
> > @@ -3573,7 +3572,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> > * to refine return values.
> > */
> > meta->msize_smax_value = reg->smax_value;
> > - meta->msize_umax_value = reg->umax_value;
> >
> > /* The register is SCALAR_VALUE; the access check
> > * happens using its boundaries.
> > @@ -4078,9 +4076,9 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type,
> > return;
> >
> > ret_reg->smax_value = meta->msize_smax_value;
> > - ret_reg->umax_value = meta->msize_umax_value;
> > __reg_deduce_bounds(ret_reg);
> > __reg_bound_offset(ret_reg);
> > + __update_reg_bounds(ret_reg);
>
> Thanks a lot for the analysis and the fix.
> I think there is still a small problem.
> The variable is called msize_smax_value which is used to remember smax,
> but the helpers actually use umax and the rest of
> if (arg_type_is_mem_size(arg_type)) { ..}
> branch is validating [0,umax] range of memory.
> bpf_get_stack() and probe_read_str*() have 'u32 size' arguments too.
> So doing
> meta->msize_smax_value = reg->smax_value;
> isn't quite correct.
Agree.
> Also the name is misleading, since the verifier needs to remember
> the size 'signed max' doesn't have the right meaning here.
> It's just a size. It cannot be 'signed' and cannot be negative.
> How about renaming it to just msize_max_value and do
> meta->msize_max_value = reg->umax_value;
msize_max_value reads better and we can give it u64 type so the
"cannot be negative" part is clear.
> while later do:
> ret_reg->smax_value = meta->msize_max_value;
> with a comment that return value from the helpers
> is 'int' and not 'unsigned int' while input argument is 'u32 size'.
Sounds better I'll draft a v2.
