[bpf,0/2] riscv, bpf: Properly sign-extend return values

Message

Björn Töpel Oct. 4, 2023, 12:07 p.m. UTC

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

The RISC-V architecture does not expose sub-registers, and hold all
32-bit values in a sign-extended format [1] [2]:

  | The compiler and calling convention maintain an invariant that all
  | 32-bit values are held in a sign-extended format in 64-bit
  | registers. Even 32-bit unsigned integers extend bit 31 into bits
  | 63 through 32. Consequently, conversion between unsigned and
  | signed 32-bit integers is a no-op, as is conversion from a signed
  | 32-bit integer to a signed 64-bit integer.

While BPF, on the other hand, exposes sub-registers, and use
zero-extension (similar to arm64/x86).

This has led to some subtle bugs, where a BPF JITted program has not
sign-extended the a0 register (return value in RISC-V land), passed
the return value up the kernel, e.g.:
    
  | int from_bpf(void);
  |
  | long foo(void)
  | {
  |    return from_bpf();
  | }

This series fixes this issue by keeping a pair of return value
registers; a0 (RISC-V ABI, sign-extended), a5 (BPF, zero-extended).

The following test_progs now pass, which were previously broken:

  | 13      bpf_cookie
  | 19      bpf_mod_race
  | 68      deny_namespace
  | 119     libbpf_get_fd_by_id_opts
  | 135     lookup_key
  | 137     lsm_cgroup
  | 284     test_lsm


Björn


Björn Töpel (2):
  riscv, bpf: Sign-extend return values
  riscv, bpf: Track both a0 (RISC-V ABI) and a5 (BPF) return values

 arch/riscv/net/bpf_jit_comp64.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)


base-commit: 9077fc228f09c9f975c498c55f5d2e882cd0da59

Comments

Daniel Borkmann Oct. 9, 2023, 1:27 p.m. UTC | #1

On 10/4/23 2:07 PM, Björn Töpel wrote:
> From: Björn Töpel <[e-mail]>
[...]
> The following test_progs now pass, which were previously broken:
> 
>    | 13      bpf_cookie
>    | 19      bpf_mod_race
>    | 68      deny_namespace
>    | 119     libbpf_get_fd_by_id_opts
>    | 135     lookup_key
>    | 137     lsm_cgroup
>    | 284     test_lsm

Thanks for the fixes, took them into bpf tree. I was wondering whether this could be
backed by specific tests, but looks like the above list already takes care of it.

Thanks,
Daniel

[e-mail] Oct. 9, 2023, 1:30 p.m. UTC | #2

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <[e-mail]>:

On Wed,  4 Oct 2023 14:07:04 +0200 you wrote:
> From: Björn Töpel <[e-mail]>
> 
> The RISC-V architecture does not expose sub-registers, and hold all
> 32-bit values in a sign-extended format [1] [2]:
> 
>   | The compiler and calling convention maintain an invariant that all
>   | 32-bit values are held in a sign-extended format in 64-bit
>   | registers. Even 32-bit unsigned integers extend bit 31 into bits
>   | 63 through 32. Consequently, conversion between unsigned and
>   | signed 32-bit integers is a no-op, as is conversion from a signed
>   | 32-bit integer to a signed 64-bit integer.
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] riscv, bpf: Sign-extend return values
    https://git.kernel.org/bpf/bpf/c/2f1b0d3d7331
  - [bpf,2/2] riscv, bpf: Track both a0 (RISC-V ABI) and a5 (BPF) return values
    https://git.kernel.org/bpf/bpf/c/7112cd26e606

You are awesome, thank you!