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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20200326062301.fvomwkz5grg3b5qb@ast-mbp>
Date:   Wed, 25 Mar 2020 23:23:01 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     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

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;