[bpf] riscv, bpf: Fix possible infinite tailcall when CONFIG_CFI_CLANG is enabled

Commit Message

Pu Lehui Oct. 8, 2024, 12:45 p.m. UTC

From: Pu Lehui <[e-mail]>

When CONFIG_CFI_CLANG is enabled, the number of prologue instructions
skipped by tailcall needs to include the kcfi instruction, otherwise the
TCC will be initialized every tailcall is called, which may result in
infinite tailcalls.

Fixes: e63985ecd226 ("bpf, riscv64/cfi: Support kCFI + BPF on riscv64")
Signed-off-by: Pu Lehui <[e-mail]>
---
 arch/riscv/net/bpf_jit_comp64.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Björn Töpel Oct. 9, 2024, 8:33 a.m. UTC | #1

Pu Lehui <[e-mail]> writes:

> From: Pu Lehui <[e-mail]>
>
> When CONFIG_CFI_CLANG is enabled, the number of prologue instructions
> skipped by tailcall needs to include the kcfi instruction, otherwise the
> TCC will be initialized every tailcall is called, which may result in
> infinite tailcalls.
>
> Fixes: e63985ecd226 ("bpf, riscv64/cfi: Support kCFI + BPF on riscv64")
> Signed-off-by: Pu Lehui <[e-mail]>

Thanks! Did you test this with the selftest suite? Did the tailcall
tests catch it?

Note to self is that I should run kCFI enabled tests for RISC-V.


Acked-by: Björn Töpel <[e-mail]>

Pu Lehui Oct. 9, 2024, 10:31 a.m. UTC | #2

On 2024/10/9 16:33, Björn Töpel wrote:
> Pu Lehui <[e-mail]> writes:
> 
>> From: Pu Lehui <[e-mail]>
>>
>> When CONFIG_CFI_CLANG is enabled, the number of prologue instructions
>> skipped by tailcall needs to include the kcfi instruction, otherwise the
>> TCC will be initialized every tailcall is called, which may result in
>> infinite tailcalls.
>>
>> Fixes: e63985ecd226 ("bpf, riscv64/cfi: Support kCFI + BPF on riscv64")
>> Signed-off-by: Pu Lehui <[e-mail]>
> 
> Thanks! Did you test this with the selftest suite? Did the tailcall
> tests catch it?

Oh, I discovered it through code review.

I just tried llvm compilation but it seems that my environment cannot 
compile bpf selftests. I need to find why.

But after reading the tailcalls testcase, I found that the tailcall_3 
subtest can cover this scenario as it will verify the TCC value.

> 
> Note to self is that I should run kCFI enabled tests for RISC-V.
> 
> 
> Acked-by: Björn Töpel <[e-mail]>

[e-mail] Oct. 10, 2024, 1:30 a.m. UTC | #3

Hello:

This patch was applied to bpf/bpf.git (master)
by Alexei Starovoitov <[e-mail]>:

On Tue,  8 Oct 2024 12:45:44 +0000 you wrote:
> From: Pu Lehui <[e-mail]>
> 
> When CONFIG_CFI_CLANG is enabled, the number of prologue instructions
> skipped by tailcall needs to include the kcfi instruction, otherwise the
> TCC will be initialized every tailcall is called, which may result in
> infinite tailcalls.
> 
> [...]

Here is the summary with links:
  - [bpf] riscv, bpf: Fix possible infinite tailcall when CONFIG_CFI_CLANG is enabled
    https://git.kernel.org/bpf/bpf/c/30a59cc79754

You are awesome, thank you!

Patch

diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 99f34409fb60..91bd5082c4d8 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -18,6 +18,7 @@ 
 #define RV_MAX_REG_ARGS 8
 #define RV_FENTRY_NINSNS 2
 #define RV_FENTRY_NBYTES (RV_FENTRY_NINSNS * 4)
+#define RV_KCFI_NINSNS (IS_ENABLED(CONFIG_CFI_CLANG) ? 1 : 0)
 /* imm that allows emit_imm to emit max count insns */
 #define RV_MAX_COUNT_IMM 0x7FFF7FF7FF7FF7FF
 
@@ -271,7 +272,8 @@  static void __build_epilogue(bool is_tail_call, struct rv_jit_context *ctx)
 	if (!is_tail_call)
 		emit_addiw(RV_REG_A0, RV_REG_A5, 0, ctx);
 	emit_jalr(RV_REG_ZERO, is_tail_call ? RV_REG_T3 : RV_REG_RA,
-		  is_tail_call ? (RV_FENTRY_NINSNS + 1) * 4 : 0, /* skip reserved nops and TCC init */
+		  /* kcfi, fentry and TCC init insns will be skipped on tailcall */
+		  is_tail_call ? (RV_KCFI_NINSNS + RV_FENTRY_NINSNS + 1) * 4 : 0,
 		  ctx);
 }