Hi Maxwell,
I am happy to test your code everytime but it would be great if you test
the code before posting it on the list. Otherwise it would take multiple
revisions for the patches to be accepted. I understand that testing this
is non-trivial because you need clang and everything and you also need
ARM64 hardware or Qemu setup. But if you enjoy kernel development you
will find all this is worth it.
> +u32 cfi_get_func_hash(void *func)
> +{
> + u32 *hashp = func - cfi_get_offset();
> + return READ_ONCE(*hashp);
The above assumes that hashp is always a valid address, and when it is
not, it crashes the kernel:
Building these patches with clang and with CONFIG_CFI_CLANG=y and then
running `sudo ./test_progs -a dummy_st_ops` crashes the kernel like:
Internal error: Oops: 0000000096000006 [#1] SMP
Modules linked in: bpf_testmod(OE) nls_ascii nls_cp437 aes_ce_blk aes_ce_cipher ghash_ce sha1_ce button sunrpc sch_fq_codel dm_mod dax configfs dmi_sysfs sha2_ce sha256_arm64
CPU: 47 PID: 5746 Comm: test_progs Tainted: G W OE 6.10.0-rc2+ #41
Hardware name: Amazon EC2 c6g.16xlarge/, BIOS 1.0 11/1/2018
pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : cfi_get_func_hash+0xc/0x1c
lr : prepare_trampoline+0xcc/0xf44
sp : ffff80008b1637e0
x29: ffff80008b163810 x28: ffff0003c9e4b0c0 x27: 0000000000000000
x26: 0000000000000010 x25: ffff0003d4ab7000 x24: 0000000000000040
x23: 0000000000000018 x22: 0000000000000037 x21: 0000000000000001
x20: 0000000000000020 x19: ffff80008b163870 x18: 0000000000000000
x17: 00000000ad6b63b6 x16: 00000000ad6b63b6 x15: ffff80008002eed4
x14: ffff80008002eff4 x13: ffff80008b160000 x12: ffff80008b164000
x11: 0000000000000082 x10: 0000000000000010 x9 : ffff80008004b724
x8 : 0000000000000110 x7 : 0000000000000000 x6 : 0000000000000001
x5 : 0000000000000110 x4 : 0000000000000001 x3 : 0000000000000000
x2 : ffff0003d4ab7000 x1 : 000000000000003f x0 : 0000000000000000
Call trace:
cfi_get_func_hash+0xc/0x1c
arch_bpf_trampoline_size+0xe8/0x158
bpf_struct_ops_prepare_trampoline+0x8
[...]
Here is my understanding of the above:
We are trying to get the cfi hash from <func_addr> - 4, but clang
doesn't emit the cfi hash for all functions, and in case the cfi hash is
not emitted, <func_addr> - 4 could be an invalid address or point to
something that is not a cfi hash.
In my original patch I had:
u32 cfi_get_func_hash(void *func)
{
u32 hash;
if (get_kernel_nofault(hash, func - cfi_get_offset()))
return 0;
return hash;
}
I think we need to keep the get_kernel_nofault() to fix this issue.
cfi_get_func_hash() in arch/x86/kernel/alternative.c also uses
get_kernel_nofault() and I think it is there for the same reason.
Thanks,
Puranjay
Attachment:
signature.asc
Description: PGP signature
