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]

Date:   Tue, 06 Dec 2022 14:49:22 +0100
From:   Björn Töpel <bjorn@...nel.org>
To:     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

Ilya Leoshkevich <iii@...ux.ibm.com> writes:

> 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?

I've only seen it for kfuncs, yes.

>
>         /* 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.
>
> If the above is correct, then shouldn't we rather use sizeof(void *) in
> the mark_btf_func_reg_size() call above?

Hmm, or rather sizeof(u64) if I'm reading you correctly?


Thanks for having a look!
Björn