netdev - Re: [bpf-next PATCH 05/10] bpf: verifier, return value is an int in do_refine_retval

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 26 Mar 2020 08:52:15 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     ecree@...arflare.com, yhs@...com, daniel@...earbox.net,
        netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [bpf-next PATCH 05/10] bpf: verifier, return value is an int in
 do_refine_retval_range

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@...il.com>
> > ---
> >  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.