netdev - Re: [PATCH bpf v2] bpf: Do not zero-extend kfunc return values

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: <886b6647-fe65-39d7-dd78-86a5f5d2acc6@meta.com>
Date:   Wed, 7 Dec 2022 08:47:14 -0800
From:   Yonghong Song <yhs@...a.com>
To:     Björn Töpel <bjorn@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>, bpf@...r.kernel.org,
        netdev@...r.kernel.org
Cc:     Björn Töpel <bjorn@...osinc.com>,
        Ilya Leoshkevich <iii@...ux.ibm.com>,
        Brendan Jackman <jackmanb@...gle.com>,
        Yang Jihong <yangjihong1@...wei.com>
Subject: Re: [PATCH bpf v2] bpf: Do not zero-extend kfunc return values



On 12/7/22 2:35 AM, Björn Töpel wrote:
> From: Björn Töpel <bjorn@...osinc.com>
> 
> In BPF all global functions, and BPF helpers return a 64-bit
> value. For kfunc calls, this is not the case, and they can return
> e.g. 32-bit values.
> 
> The return register R0 for kfuncs calls can therefore be marked as
> subreg_def != DEF_NOT_SUBREG. In general, if a register is marked with
> subreg_def != DEF_NOT_SUBREG, some archs (where bpf_jit_needs_zext()
> returns true) require the verifier to insert explicit zero-extension
> instructions.
> 
> For kfuncs calls, however, the caller should do sign/zero extension
> for return values. In other words, the compiler is responsible to
> insert proper instructions, not the verifier.
> 
> An example, provided by Yonghong Song:
> 
> $ cat t.c
> extern unsigned foo(void);
> unsigned bar1(void) {
>       return foo();
> }
> unsigned bar2(void) {
>       if (foo()) return 10; else return 20;
> }
> 
> $ clang -target bpf -mcpu=v3 -O2 -c t.c && llvm-objdump -d t.o
> t.o:    file format elf64-bpf
> 
> Disassembly of section .text:
> 
> 0000000000000000 <bar1>:
> 	0:       85 10 00 00 ff ff ff ff call -0x1
> 	1:       95 00 00 00 00 00 00 00 exit
> 
> 0000000000000010 <bar2>:
> 	2:       85 10 00 00 ff ff ff ff call -0x1
> 	3:       bc 01 00 00 00 00 00 00 w1 = w0
> 	4:       b4 00 00 00 14 00 00 00 w0 = 0x14
> 	5:       16 01 01 00 00 00 00 00 if w1 == 0x0 goto +0x1 <LBB1_2>
> 	6:       b4 00 00 00 0a 00 00 00 w0 = 0xa
> 
> 0000000000000038 <LBB1_2>:
> 	7:       95 00 00 00 00 00 00 00 exit
> 
> If the return value of 'foo()' is used in the BPF program, the proper
> zero-extension will be done.
> 
> Currently, the verifier correctly marks, say, a 32-bit return value as
> subreg_def != DEF_NOT_SUBREG, but will fail performing the actual
> zero-extension, due to a verifier bug in
> opt_subreg_zext_lo32_rnd_hi32(). load_reg is not properly set to R0,
> and the following path will be taken:
> 
> 		if (WARN_ON(load_reg == -1)) {
> 			verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
> 			return -EFAULT;
> 		}
> 
> A longer discussion from v1 can be found in the link below.
> 
> Correct the verifier by avoiding doing explicit zero-extension of R0
> for kfunc calls. Note that R0 will still be marked as a sub-register
> for return values smaller than 64-bit.
> 
> Fixes: 83a2881903f3 ("bpf: Account for BPF_FETCH in insn_has_def32()")
> Link: https://lore.kernel.org/bpf/20221202103620.1915679-1-bjorn@kernel.org/
> Suggested-by: Yonghong Song <yhs@...a.com>
> Signed-off-by: Björn Töpel <bjorn@...osinc.com>

Acked-by: Yonghong Song <yhs@...com>