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] [day] [month] [year] [list]

Date:   Wed, 07 Dec 2022 08:10:49 +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:

>>>>>> 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.
>> 
>> The "problem" is that I hit this:
>> 		if (WARN_ON(load_reg == -1)) {
>> 			verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
>> 			return -EFAULT;
>> 		}
>> 
>> This path is only taken for archs which have bpf_jit_needs_zext() ==
>> true. In my case it's riscv64, but it should hit i386, sparc, s390, ppc,
>> mips, and arm.
>> 
>> My reading of this thread has been that "marking the call has
>> zext_dst=true, is incorrect", i.e. that LLVM will insert the correct
>> zext instructions.
>
> Your interpretation is correct. Yes, for func return values, the
> llvm will insert correct zext/sext instructions if the return
> value is used. Otherwise, if the return value simply passes
> through, the caller call site should handle that properly.
>
> So, yes changing t->size to sizeof(u64) in below code in
> check_kfunc_call() should work. But the fix sounds like a hack
> and we might have some side effect during verification, now
> or future.
>
> Maybe we could check BPF_PSEUDO_KFUNC_CALL in appropriate place to 
> prevent zext.

Thanks for all the input! I'll digest it, and get back with a v2.