netdev - RE: [PATCH v2 bpf-next 1/3] bpf: Support for pointers beyond pkt

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: <5fad89fb649af_2a612088e@john-XPS-13-9370.notmuch>
Date:   Thu, 12 Nov 2020 11:16:11 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        davem@...emloft.net
Cc:     daniel@...earbox.net, netdev@...r.kernel.org, bpf@...r.kernel.org,
        kernel-team@...com
Subject: RE: [PATCH v2 bpf-next 1/3] bpf: Support for pointers beyond pkt_end.

Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@...nel.org>
> 
> This patch adds the verifier support to recognize inlined branch conditions.
> The LLVM knows that the branch evaluates to the same value, but the verifier
> couldn't track it. Hence causing valid programs to be rejected.
> The potential LLVM workaround: https://reviews.llvm.org/D87428
> can have undesired side effects, since LLVM doesn't know that
> skb->data/data_end are being compared. LLVM has to introduce extra boolean
> variable and use inline_asm trick to force easier for the verifier assembly.
> 
> Instead teach the verifier to recognize that
> r1 = skb->data;
> r1 += 10;
> r2 = skb->data_end;
> if (r1 > r2) {
>   here r1 points beyond packet_end and
>   subsequent
>   if (r1 > r2) // always evaluates to "true".
> }
> 
> Tested-by: Jiri Olsa <jolsa@...hat.com>
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---
>  include/linux/bpf_verifier.h |   2 +-
>  kernel/bpf/verifier.c        | 129 +++++++++++++++++++++++++++++------
>  2 files changed, 108 insertions(+), 23 deletions(-)
> 

Thanks, we can remove another set of inline asm logic.

Acked-by: John Fastabend <john.fastabend@...il.com>
 
>  	if (pred >= 0) {
> @@ -7517,7 +7601,8 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		 */
>  		if (!__is_pointer_value(false, dst_reg))
>  			err = mark_chain_precision(env, insn->dst_reg);
> -		if (BPF_SRC(insn->code) == BPF_X && !err)
> +		if (BPF_SRC(insn->code) == BPF_X && !err &&
> +		    !__is_pointer_value(false, src_reg))

This could have been more specific with !type_is_pkt_pointer() correct? I
think its fine as is though.

>  			err = mark_chain_precision(env, insn->src_reg);
>  		if (err)
>  			return err;
> -- 
> 2.24.1
>