Re: [PATCH v2 2/2] selftests/x86: add generic prctl shadow stack test

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The global `_which_test` variable is never initialized, causing the signal handler to always fall back to ARCH_PRCTL.
- [Low] CET inline assembly is exposed to older toolchains due to an incorrectly moved compiler check.
- [Low] Missing fallback macro definitions for PRCTL flags break build compatibility on older systems.

Pre-existing issues:
- [High] Unchecked `fork()` failure in `test_ptrace()` broadcasts `SIGKILL` to all processes.
--

--- Patch [2]: [PATCH v2 2/2] selftests/x86: add generic prctl shadow stack test ---
commit 347a779613419462705a2f200f991f112e768094
Author: Bill Roberts <[email protected]>
Subject: selftests/x86: add generic prctl shadow stack test

This commit abstracts the x86 shadow stack selftests into a reusable
library and introduces a new test for the generic prctl() shadow stack
interface.

> diff --git a/tools/testing/selftests/x86/shadow_stack.c b/tools/testing/selftests/x86/shadow_stack.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/testing/selftests/x86/shadow_stack.c
[ ... ]
> +/*
> + * Define the ABI defines if needed, so people can run the tests
> + * without building the headers.
> + */
> +#ifndef __NR_map_shadow_stack
> +#define __NR_map_shadow_stack	453
> +
> +#define SHADOW_STACK_SET_TOKEN	(1ULL << 0)
> +
> +#define ARCH_SHSTK_ENABLE	0x5001
> +#define ARCH_SHSTK_DISABLE	0x5002
> +#define ARCH_SHSTK_LOCK		0x5003
> +#define ARCH_SHSTK_UNLOCK	0x5004
> +#define ARCH_SHSTK_STATUS	0x5005
> +
> +#define ARCH_SHSTK_SHSTK	(1ULL <<  0)
> +#define ARCH_SHSTK_WRSS		(1ULL <<  1)
> +
> +#define NT_X86_SHSTK	0x204
> +#endif

[Severity: Low]
Does this code break builds on older systems?

The test now relies on PRCTL constants like PR_SET_SHADOW_STACK_STATUS,
PR_SHADOW_STACK_ENABLE, and PR_SHADOW_STACK_WRITE. If a system's
sys/prctl.h is outdated, these will be undeclared.

Could we add fallback definitions for these constants similar to the
ARCH_SHSTK_* ones?

[ ... ]
> +#define SS_SIZE 0x200000
> +#define PAGE_SIZE 0x1000
> +
> +void write_shstk(unsigned long *addr, unsigned long val)
> +{
> +	asm volatile("wrssq %[val], (%[addr])\n"
> +		     : "=m" (addr)
> +		     : [addr] "r" (addr), [val] "r" (val));
> +}

[Severity: Low]
Could this cause build failures on systems with older toolchains?

The compiler version check for CET support was moved down to wrap only
shadow_stack_run_tests(). As a result, older compilers might fail to
build the test since they won't recognize the wrssq inline assembly in
write_shstk() and get_ssp().

[ ... ]
> +sigjmp_buf jmp_buffer;
> +enum shadow_stack_test _which_test;
> +void segv_gp_handler(int signum, siginfo_t *si, void *uc)
> +{
> +	segv_triggered = true;
> +
> +	/*
> +	 * To work with old glibc, this can't rely on siglongjmp working with
> +	 * shadow stack enabled, so disable shadow stack before siglongjmp().
> +	 */
> +	SHADOW_STACK_DISABLE(_which_test);
> +	siglongjmp(jmp_buffer, -1);
> +}

[Severity: Medium]
Is the generic prctl() fallback path ever actually tested in the signal handler?

The global _which_test variable is declared but never initialized.
When shadow_stack_run_tests() is called, it receives the test enum as
which_test, but never assigns it to _which_test.

Because _which_test remains 0 (SHADOW_STACK_TEST_ARCH_PRCTL), the
signal handler will always use the ARCH_PRCTL path instead of the new
PRCTL path, even when running the prctl test suite.

[ ... ]
> +int test_ptrace(void)
> +{
> +	unsigned long saved_ssp, ssp = 0;
> +	struct sigaction sa = {};
> +	struct iovec iov;
> +	int status;
> +	int pid;
> +
> +	iov.iov_base = &ssp;
> +	iov.iov_len = sizeof(ssp);
> +
> +	pid = fork();
> +	if (!pid) {

[Severity: High]
This is a pre-existing issue, but could this code broadcast SIGKILL to
all user processes if fork() fails?

If fork() returns -1 due to resource limits, execution will proceed with
pid = -1. When ptrace() subsequently fails, the error path is triggered:

out_kill:
	kill(pid, SIGKILL);
	return 1;

Since pid is -1, POSIX specifies that this will kill all processes the
current user has permission to terminate, potentially crashing the host
environment.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2