RISC-V ABI break: x7/t2 register is used to pass the 9th argument to a function in certain cases

[euspectre]

According to the RISC-V ABI, the first 8 integer arguments of a function are passed in registers a0-a7, the 9th and subsequent ones go on stack. It seems, clang does not always conform to that.

While investigating a crash in the Linux kernel built with clang 17.0.6, I found that x7/t2 register rather than the stack was used to pass the 9th argument, mpri, to __find_rr_leaf() function from net/ipv6/route.c.

Here is a reduced and simplified version of the preprocessed net/ipv6/route.c that demonstrates the issue:
test-arg9.c

It can be compiled as follows (the options were extracted from the kernel build):

clang --target=riscv64-linux-gnu -fintegrated-as \
  -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored \
  -Werror=unused-command-line-argument \
  -std=gnu11 -fshort-wchar -funsigned-char -fno-common -fno-PIE \
  -fno-strict-aliasing -mabi=lp64 -march=rv64imac_zihintpause \
  -mno-save-restore -mcmodel=medany \
  -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-delete-null-pointer-checks \
  -O2 \
  -fstack-protector-strong -fno-omit-frame-pointer -fno-optimize-sibling-calls \
  -ftrivial-auto-var-init=zero \
  -falign-functions=4 -fpatchable-function-entry=12 \
  -fstrict-flex-arrays=3 -fno-strict-overflow -fno-stack-check \
  -Wall -Wundef -Werror=implicit-function-declaration \
  -Werror=implicit-int -Werror=return-type -Werror=strict-prototypes \
  -Wno-format-security -Wno-trigraphs -Wno-frame-address -Wno-address-of-packed-member \
  -Wmissing-declarations -Wmissing-prototypes \
  -Wframe-larger-than=2048 -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable \
  -Wvla -Wno-pointer-sign -Wcast-function-type -Wimplicit-fallthrough \
  -Werror=date-time -Werror=incompatible-pointer-types -Wenum-conversion \
  -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-pointer-to-enum-cast \
  -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -Wno-cast-function-type-strict \
  -Wno-missing-field-initializers -Wno-type-limits -Wno-shift-negative-value -Wno-initializer-overrides \
  -Wno-sign-compare -Wno-unused-value -Wno-parentheses-equality -Wno-self-assign \
  -g -c -o test-arg9.o test-arg9.c

clang: version 17.0.6 (https://github.com/llvm/llvm-project.git 6009708)
Target: riscv64-unknown-linux-gnu

A disassembly of test-arg9.o:
test-arg9.disasm

Here are the portions of __find_rr_leaf() (callee) and fib6_table_lookup() (caller) where t2 is used to pass the 9th argument to the function:

fib6_table_lookup():
...
00000000000000c4 <.LBB0_9>:
  c4:	00ccab83          	lw	s7,12(s9)
  c8:	f8043423          	sd	zero,-120(s0)
  cc:	f9b42223          	sw	s11,-124(s0)
  d0:	f8840713          	addi	a4,s0,-120
  d4:	f8340893          	addi	a7,s0,-125
  d8:	f8440393          	addi	t2,s0,-124	// <<< here
  dc:	8566                	mv	a0,s9
  de:	4581                	li	a1,0
  e0:	865e                	mv	a2,s7
  e2:	86da                	mv	a3,s6
  e4:	87ce                	mv	a5,s3
  e6:	884a                	mv	a6,s2
  e8:	00000097          	auipc	ra,0x0
			e8: R_RISCV_CALL_PLT	__find_rr_leaf
			e8: R_RISCV_RELAX	*ABS*
  ec:	000080e7          	jalr	ra # e8 <.LBB0_9+0x24>
...

__find_rr_leaf():
...
 1e2:	891e                	mv	s2,t2
...
00000000000002b2 <.LBB1_16>:
 2b2:	00092503          	lw	a0,0(s2)	// <<< "if (m > *mpri)" in find_match()
 2b6:	01455063          	bge	a0,s4,2b6 <.LBB1_16+0x4>
			2b6: R_RISCV_BRANCH	.LBB1_4
 2ba:	0d05                	addi	s10,s10,1
 2bc:	001d3513          	seqz	a0,s10
 2c0:	f4843583          	ld	a1,-184(s0)
 2c4:	00a58023          	sb	a0,0(a1)
 2c8:	01492023          	sw	s4,0(s2)	// <<< "*mpri = m;" in find_match()
 2cc:	a001                	j	2cc <.LBB1_16+0x1a>
			2cc: R_RISCV_RVC_JUMP	.LBB1_3
...

This looks like a RISC-V ABI break.

Ftrace, for example, relies on the fact that the temporary registers like t0-t2 can be safely clobbered at the beginning of the function. So, if dynamic Ftrace is enabled for __find_rr_leaf() in the kernel, t2 could contain anything when the function tries dereferencing its 9th argument, mpri, hence the kernel crashes.

I do not think this issue has security implications, so, I am reporting it here.

[topperc]

I wonder if the calling convention was changed to fastcc by the IR optimizer?

[euspectre]

@topperc :

I wonder if the calling convention was changed to fastcc by the IR optimizer?

If it is legal to use fastcc here, then is there any compiler option or something to disable such behaviour? I mean, to force even such functions to use the calling convention that RISC-V ABI describes rather than fastcc.

[llvmbot]

@llvm/issue-subscribers-backend-risc-v

Author: Eugene A. Shatokhin (euspectre)

According to the RISC-V ABI, the first 8 integer arguments of a function are passed in registers a0-a7, the 9th and subsequent ones go on stack. It seems, clang does not always conform to that.

While investigating a crash in the Linux kernel built with clang 17.0.6, I found that x7/t2 register rather than the stack was used to pass the 9th argument, mpri, to __find_rr_leaf() function from net/ipv6/route.c.

Here is a reduced and simplified version of the preprocessed net/ipv6/route.c that demonstrates the issue:
test-arg9.c

It can be compiled as follows (the options were extracted from the kernel build):

clang --target=riscv64-linux-gnu -fintegrated-as \
  -Werror=unknown-warning-option -Werror=ignored-optimization-argument -Werror=option-ignored \
  -Werror=unused-command-line-argument \
  -std=gnu11 -fshort-wchar -funsigned-char -fno-common -fno-PIE \
  -fno-strict-aliasing -mabi=lp64 -march=rv64imac_zihintpause \
  -mno-save-restore -mcmodel=medany \
  -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-delete-null-pointer-checks \
  -O2 \
  -fstack-protector-strong -fno-omit-frame-pointer -fno-optimize-sibling-calls \
  -ftrivial-auto-var-init=zero \
  -falign-functions=4 -fpatchable-function-entry=12 \
  -fstrict-flex-arrays=3 -fno-strict-overflow -fno-stack-check \
  -Wall -Wundef -Werror=implicit-function-declaration \
  -Werror=implicit-int -Werror=return-type -Werror=strict-prototypes \
  -Wno-format-security -Wno-trigraphs -Wno-frame-address -Wno-address-of-packed-member \
  -Wmissing-declarations -Wmissing-prototypes \
  -Wframe-larger-than=2048 -Wno-gnu -Wno-unused-but-set-variable -Wno-unused-const-variable \
  -Wvla -Wno-pointer-sign -Wcast-function-type -Wimplicit-fallthrough \
  -Werror=date-time -Werror=incompatible-pointer-types -Wenum-conversion \
  -Wno-unused-but-set-variable -Wno-unused-const-variable -Wno-pointer-to-enum-cast \
  -Wno-tautological-constant-out-of-range-compare -Wno-unaligned-access -Wno-cast-function-type-strict \
  -Wno-missing-field-initializers -Wno-type-limits -Wno-shift-negative-value -Wno-initializer-overrides \
  -Wno-sign-compare -Wno-unused-value -Wno-parentheses-equality -Wno-self-assign \
  -g -c -o test-arg9.o test-arg9.c

clang: version 17.0.6 (https://github.com/llvm/llvm-project.git 6009708)
Target: riscv64-unknown-linux-gnu

A disassembly of test-arg9.o:
test-arg9.disasm

Here are the portions of __find_rr_leaf() (callee) and fib6_table_lookup() (caller) where t2 is used to pass the 9th argument to the function:

fib6_table_lookup():
...
00000000000000c4 <.LBB0_9>:
  c4:	00ccab83          	lw	s7,12(s9)
  c8:	f8043423          	sd	zero,-120(s0)
  cc:	f9b42223          	sw	s11,-124(s0)
  d0:	f8840713          	addi	a4,s0,-120
  d4:	f8340893          	addi	a7,s0,-125
  d8:	f8440393          	addi	t2,s0,-124	// <<< here
  dc:	8566                	mv	a0,s9
  de:	4581                	li	a1,0
  e0:	865e                	mv	a2,s7
  e2:	86da                	mv	a3,s6
  e4:	87ce                	mv	a5,s3
  e6:	884a                	mv	a6,s2
  e8:	00000097          	auipc	ra,0x0
			e8: R_RISCV_CALL_PLT	__find_rr_leaf
			e8: R_RISCV_RELAX	*ABS*
  ec:	000080e7          	jalr	ra # e8 <.LBB0_9+0x24>
...

__find_rr_leaf():
...
 1e2:	891e                	mv	s2,t2
...
00000000000002b2 <.LBB1_16>:
 2b2:	00092503          	lw	a0,0(s2)	// <<< "if (m > *mpri)" in find_match()
 2b6:	01455063          	bge	a0,s4,2b6 <.LBB1_16+0x4>
			2b6: R_RISCV_BRANCH	.LBB1_4
 2ba:	0d05                	addi	s10,s10,1
 2bc:	001d3513          	seqz	a0,s10
 2c0:	f4843583          	ld	a1,-184(s0)
 2c4:	00a58023          	sb	a0,0(a1)
 2c8:	01492023          	sw	s4,0(s2)	// <<< "*mpri = m;" in find_match()
 2cc:	a001                	j	2cc <.LBB1_16+0x1a>
			2cc: R_RISCV_RVC_JUMP	.LBB1_3
...

This looks like a RISC-V ABI break.

Ftrace, for example, relies on the fact that the temporary registers like t0-t2 can be safely clobbered at the beginning of the function. So, if dynamic Ftrace is enabled for __find_rr_leaf() in the kernel, t2 could contain anything when the function tries dereferencing its 9th argument, mpri, hence the kernel crashes.

I do not think this issue has security implications, so, I am reporting it here.

[euspectre]

Looks like others encountered this issue too, with exactly the same code in the Linux kernel: https://lore.kernel.org/lkml/20220509034951.trbbxhmgedocoxqm@M910t/T/

It does not seem that any solution was proposed in that thread though.

By the way, is fastcall convention for RISC-V standardized somehow? I have been unable to find any spec for it so far.

If the compiler indeed behaves correctly and switching to fastcall is allowed for the static functions, a workaround is still needed. An ideal would probably be an option to disable fastcall convention.

Saving / restoring all registers in Ftrace trampolines might be costly, disabling tracing for the functions with 9+ arguments - too error-prone.

[topperc]

CC @jrtc27 @asb @ShivaChen

[jrtc27]

Doesn't fastcc do this for other architectures too? Why isn't this an issue on other architectures? (I know for Arm at least there are IP0 and IP1 designated in their PCS which presumably are avoided for fastcc and used by ftrace?)

[euspectre]

Doesn't fastcc do this for other architectures too? Why isn't this an issue on other architectures? (I know for Arm at least there are IP0 and IP1 designated in their PCS which presumably are avoided for fastcc and used by ftrace?)

Good question.
I am no expert in ARM, but as far as I can see in arm64/kernel/entry-ftrace.S, Ftrace relies on the fact that it can safely clobber at least x9 and x10 (or even all registers x9-x17).

Android kernels are usually built with clang, some of them likely support Ftrace - and it seems to work somehow, for a long time.

[topperc]

We discussed this a little in the LLVM RISC-V call this morning.

From a quick inspection of AArchCallingConv.td it doesn't look it does anything different for fastcc. Nor does x86-64.

x86-32 does implement a fastcc. The arguments are in memory in the standard ABI. So fastcc is a probably good optimization there.

AArch64, RISC-V, and X86-64 already pass arguments in registers in the standard ABI. I wonder how much additional performance the RISC-V fastcc provides.

[euspectre]

AArch64, RISC-V, and X86-64 already pass arguments in registers in the standard ABI. I wonder how much additional performance the RISC-V fastcc provides.

As far as Linux kernel is concerned - not much, I guess. Not sure about other software though.

The calling convention in the RISC-V ABI requires that 8 registers, a0-a7, are used to pass the first 8 integer/pointer arguments, the remaining ones go on stack.

fastcc seems to only differ from that for the functions with 9 or more arguments. There are not very many such functions in the kernel and these are unlikely to be in the hot code paths often.

[jyknight]

This is usually the intended/expected behavior. For internal calls, where the compiler controls both the caller and callee, the calling convention is irrelevant so it's perfectly allowable (and desirable) to use whatever registers it likes between those caller/callee.

But, this case involves the -fpatchable-function-entry=N flag/attribute. This tells the compiler to leave space at the beginnings of functions so that an external entity (the kernel) can overwrite them with its own instructions. And in this case, I think it's valid to expect that the calling convention is not changed via optimizations from what the user specified, since now the compiler isn't just calling its own generated code, but rather is calling this user-provided function-entry assembly.

Thus, I think the correct fix here is for LLVM to disable all calling-convention-mutating optimizations on functions with the patchable-function-entry IR attribute (on all architectures, not just RISCV).