John Fastabend wrote:
> 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.
v2 on the list now. Thanks!
@Daniel, I dropped your reviewed-by please take another look the change
should be small and functionality was the same but still its not the
same thing you reviewed ;)
@Alexei, Can you check you agree with the "v2 note:" I added at the
bottom of the commit. The gist is it doesn't really matter from
the verifiers point of view if we use umax_value or smax_value for
the msize_max_value because just below setting msize_max_value we
check tnum_is_const and value is positive. That said it reads better
now.
