netdev - Re: [PATCH bpf] bpf: Proper R0 zero-extension for BPF

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: <87o7sg1kbx.fsf@all.your.base.are.belong.to.us>
Date:   Tue, 06 Dec 2022 19:02:58 +0100
From:   Björn Töpel <bjorn@...nel.org>
To:     Yonghong Song <yhs@...a.com>, Ilya Leoshkevich <iii@...ux.ibm.com>,
        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>,
        Brendan Jackman <jackmanb@...gle.com>
Subject: Re: [PATCH bpf] bpf: Proper R0 zero-extension for BPF_CALL
 instructions

Yonghong Song <yhs@...a.com> writes:

> On 12/6/22 5:21 AM, Ilya Leoshkevich wrote:
>> On Fri, 2022-12-02 at 11:36 +0100, Björn Töpel wrote:
>>> From: Björn Töpel <bjorn@...osinc.com>
>>>
>>> A BPF call instruction can be, correctly, marked with zext_dst set to
>>> true. An example of this can be found in the BPF selftests
>>> progs/bpf_cubic.c:
>>>
>>>    ...
>>>    extern __u32 tcp_reno_undo_cwnd(struct sock *sk) __ksym;
>>>
>>>    __u32 BPF_STRUCT_OPS(bpf_cubic_undo_cwnd, struct sock *sk)
>>>    {
>>>            return tcp_reno_undo_cwnd(sk);
>>>    }
>>>    ...
>>>
>>> which compiles to:
>>>    0:  r1 = *(u64 *)(r1 + 0x0)
>>>    1:  call -0x1
>>>    2:  exit
>>>
>>> The call will be marked as zext_dst set to true, and for some
>>> backends
>>> (bpf_jit_needs_zext() returns true) expanded to:
>>>    0:  r1 = *(u64 *)(r1 + 0x0)
>>>    1:  call -0x1
>>>    2:  w0 = w0
>>>    3:  exit
>> 
>> In the verifier, the marking is done by check_kfunc_call() (added in
>> e6ac2450d6de), right? So the problem occurs only for kfuncs?
>> 
>>          /* Check return type */
>>          t = btf_type_skip_modifiers(desc_btf, func_proto->type, NULL);
>> 
>>          ...
>> 
>>          if (btf_type_is_scalar(t)) {
>>                  mark_reg_unknown(env, regs, BPF_REG_0);
>>                  mark_btf_func_reg_size(env, BPF_REG_0, t->size);
>> 
>> I tried to find some official information whether the eBPF calling
>> convention requires sign- or zero- extending return values and
>> arguments, but unfortunately [1] doesn't mention this.
>> 
>> LLVM's lib/Target/BPF/BPFCallingConv.td mentions both R* and W*
>> registers, but since assigning to W* leads to zero-extension, it seems
>> to me that this is the case.
>
> We actually follow the clang convention, the zero-extension is either
> done in caller or callee, but not both. See 
> https://reviews.llvm.org/D131598 how the convention could be changed.
>
> The following is an example.
>
> $ 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 actually used in the bpf program, the
> proper zero extension will be done. Otherwise, it is not done.
>
> This is with latest llvm16. I guess we need to check llvm whether
> we could enforce to add a w0 = w0 in bar1().
>
> Otherwise, with this patch, it will add w0 = w0 in all cases which
> is not necessary in most of practical cases.

Thanks, Yonghong! So, what would the correct fix be? We don't want the
verifier to mark the call for zext_dst in my commit message example,
since the zext will be properly done by LLVM.

Wdyt about Ilya's suggestion marking R0 as 64b? That avoids hitting my
"verifier bug", but I'm not well versed enough in verifier land to say
whether that breaks something else... I.e. is setting reg->subreg_def to
DEF_NOT_SUBREG for R0 correct?