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:51:20 +0100
From:   Ilya Leoshkevich <iii@...ux.ibm.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>,
        Brendan Jackman <jackmanb@...gle.com>
Subject: Re: [PATCH bpf] bpf: Proper R0 zero-extension for BPF_CALL
 instructions

On Tue, 2022-12-06 at 14:49 +0100, Björn Töpel wrote:
> 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?

Whoops, you are right - that's indeed what I meant here.