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