On 8/8/22 4:25 PM, Andrii Nakryiko wrote:
On Sun, Aug 7, 2022 at 10:51 AM Yonghong Song <yhs@xxxxxx> wrote:
Tejun reported a bpf program kfunc return value mis-handling which
may cause incorrect result. The following is an example to show
the problem.
$ cat t.c
unsigned char bar();
int foo() {
if (bar() != 10) return 0; else return 1;
}
$ clang -target bpf -O2 -c t.c
$ llvm-objdump -d t.o
...
0000000000000000 <foo>:
0: 85 10 00 00 ff ff ff ff call -1
1: bf 01 00 00 00 00 00 00 r1 = r0
2: b7 00 00 00 01 00 00 00 r0 = 1
3: 15 01 01 00 0a 00 00 00 if r1 == 10 goto +1 <LBB0_2>
4: b7 00 00 00 00 00 00 00 r0 = 0
0000000000000028 <LBB0_2>:
5: 95 00 00 00 00 00 00 00 exit
$
In the above example, the return type for bar() is 'unsigned char'.
But in the disassembly code, the whole register 'r1' is used to
compare to 10 without truncating upper 56 bits.
If function bar() is implemented as a bpf function, everything
should be okay since bpf ABI will make sure the caller do
proper truncation of upper 56 bits.
But if function bar() is implemented as a non-bpf kfunc,
there could a mismatch between bar() implementation and bpf program.
For example, if the host arch is x86_64, the bar() function
may just put the return value in lower 8-bit subregister and all
upper 56 bits could contain garbage. This is not a problem
if bar() is called in x86_64 context as the caller will use
%al to get the value.
But this could be a problem if bar() is called in bpf context
and there is a mismatch expectation between bpf and native architecture.
Currently, bpf programs use the default llvm ABI ([1], function
isPromotableIntegerTypeForABI()) such that if an integer type size
is less than int type size, it is assumed proper sign or zero
extension has been done to the return value. There will be a problem
if the kfunc return value type is u8/s8/u16/s16.
Reading this I was still confused how (and whether) s32/u32 returns
are going to be handled correctly, especially on non-cpuv3 BPF object
code. So I played with this a bit and Clang does generate explicit <<
and >>/>>= shifts as expected. It might be worth it emphasizing that
for 32-bit returns Clang will generate explicit shifts?
Yes, I can reword the commit message to emphasize 32-bit return
value won't be an issue.
This patch intends to address this issue by doing proper sign or zero
extension for the kfunc return value before it is used later.
[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/TargetInfo.cpp
Reported-by: Tejun Heo <tj@xxxxxxxxxx>
Signed-off-by: Yonghong Song <yhs@xxxxxx>
---
include/linux/bpf.h | 2 ++
kernel/bpf/btf.c | 9 +++++++++
kernel/bpf/verifier.c | 35 +++++++++++++++++++++++++++++++++--
3 files changed, 44 insertions(+), 2 deletions(-)
[...]
