Alexei Starovoitov wrote:
> On Tue, Mar 24, 2020 at 10:39:16AM -0700, John Fastabend wrote:
> > Mark 32-bit subreg region with max value because do_refine_retval_range()
> > catches functions with int return type (We will assume here that int is
> > a 32-bit type). Marking 64-bit region could be dangerous if upper bits
> > are not zero which could be possible.
> >
> > Two reasons to pull this out of original patch. First it makes the original
> > fix impossible to backport. And second I've not seen this as being problematic
> > in practice unlike the other case.
> >
> > Fixes: 849fa50662fbc ("bpf/verifier: refine retval R0 state for bpf_get_stack helper")
> > Signed-off-by: John Fastabend <john.fastabend@xxxxxxxxx>
> > ---
> > kernel/bpf/verifier.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 6372fa4..3731109 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4328,7 +4328,7 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type,
> > func_id != BPF_FUNC_probe_read_str))
> > return;
> >
> > - ret_reg->smax_value = meta->msize_max_value;
> > + ret_reg->s32_max_value = meta->msize_max_value;
>
> I think this is not correct.
> These two special helpers are invoked via BPF_CALL_x() which has u64 return value.
> So despite having 'int' return in bpf_helper_defs.h the upper 32-bit will be correct.
> I think this patch should do:
> ret_reg->smax_value = meta->msize_max_value;
> ret_reg->s32_max_value = meta->msize_max_value;
OK, I missed the u64 in BPF_CALL_x(). Setting both smax and s32_max
looks correct. My logic above is wrong so I'll fix that. Thanks.
