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]
Date:   Tue, 6 Dec 2022 10:09:20 -0800
From:   Yonghong Song <yhs@...a.com>
To:     Ilya Leoshkevich <iii@...ux.ibm.com>,
        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>,
        Brendan Jackman <jackmanb@...gle.com>
Subject: Re: [PATCH bpf] bpf: Proper R0 zero-extension for BPF_CALL
 instructions



On 12/6/22 9:47 AM, Yonghong Song wrote:
> 
> 
> 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.
> 
>>
>> If the above is correct, then shouldn't we rather use sizeof(void *) in
>> the mark_btf_func_reg_size() call above?
>>
>>> The opt_subreg_zext_lo32_rnd_hi32() function which is responsible for
>>> the zext patching, relies on insn_def_regno() to fetch the register
>>> to
>>> zero-extend. However, this function does not handle call instructions
>>> correctly, and opt_subreg_zext_lo32_rnd_hi32() fails the
>>> verification.
>>>
>>> Make sure that R0 is correctly resolved for (BPF_JMP | BPF_CALL)
>>> instructions.
>>>
>>> Fixes: 83a2881903f3 ("bpf: Account for BPF_FETCH in
>>> insn_has_def32()")
>>> Signed-off-by: Björn Töpel <bjorn@...osinc.com>
>>> ---
>>> I'm not super happy about the additional special case -- first
>>> cmpxchg, and now call. :-( A more elegant/generic solution is
>>> welcome!
>>> ---
>>>   kernel/bpf/verifier.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 264b3dc714cc..4f9660eafc72 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -13386,6 +13386,9 @@ static int
>>> opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
>>>                  if (!bpf_jit_needs_zext() && !is_cmpxchg_insn(&insn))
>>>                          continue;
>>> +               if (insn.code == (BPF_JMP | BPF_CALL))
>>> +                       load_reg = BPF_REG_0;

Want to double check. Do we actually have a problem here?
For example, on x64, we probably won't have this issue.

 >>>    ...
 >>>    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);
 >>>    }

The native code will return a 32-bit subreg to bpf program,
and bpf didn't do anything and return r0 to the kernel func.
In the kernel func, the kernel will take 32-bit subreg by
x86_64 convention. This applies to some other return types
like u8/s8/u16/s16/u32/s32.

Which architecture you actually see the issue?


>>> +
>>>                  if (WARN_ON(load_reg == -1)) {
>>>                          verbose(env, "verifier bug. zext_dst is set,
>>> but no reg is defined\n");
>>>                          return -EFAULT;
>>>
>>> base-commit: 01f856ae6d0ca5ad0505b79bf2d22d7ca439b2a1
>>
>> [1]
>> https://docs.kernel.org/bpf/instruction-set.html#registers-and-calling-convention

Powered by blists - more mailing lists