Re: [PATCH 3/4] selftests/arm64: GCS must not leak on a failed clone3

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]

On Fri, Oct 16, 2026 at 04:46:02PM -0300, Iván Ezequiel Rodriguez wrote:

> clone3(CLONE_VM|CLONE_PIDFD) with an unwritable pidfd pointer fails
> in put_user() after copy_thread() has mapped a GCS. copy_process()
> then calls exit_thread(). Without HAVE_EXIT_THREAD that free is a
> no-op and the VMA stays in the parent's mm.

> Assisted-by: LLM

This is pretty hard to follow, there's a lot of open coding, duplication
and strange structures or tests.  It makes it hard to follow what's
actually being tested and why which is going to be a maintainability
issue.

> +/* Same value as KSFT_SKIP in kselftest.h. */
> +#define KSFT_SKIP		4

Why are we not just including kselftest.h?  It doesn't have the fun
interactions with libc headers that some of the other things have.
If we are going to cut'n'paste values why only KSFT_SKIP and not any of
the other exit codes we use?

> +/* Same wrapper as gcs-locking.c: extra prctl args must be zero. */
> +#define my_syscall2(num, arg1, arg2)					\
> +({									\
> +	register long _num  __asm__ ("x8") = (num);			\
> +	register long _arg1 __asm__ ("x0") = (long)(arg1);		\
> +	register long _arg2 __asm__ ("x1") = (long)(arg2);		\
> +	register long _arg3 __asm__ ("x2") = 0;				\
> +	register long _arg4 __asm__ ("x3") = 0;				\
> +	register long _arg5 __asm__ ("x4") = 0;				\
> +									\
> +	__asm__ volatile (						\
> +		"svc #0\n"						\
> +		: "=r"(_arg1)						\
> +		: "r"(_arg1), "r"(_arg2),				\
> +		  "r"(_arg3), "r"(_arg4),				\
> +		  "r"(_arg5), "r"(_num)					\
> +		: "memory", "cc"					\
> +	);								\
> +	_arg1;								\
> +})

Factor this out.

> +static void on_fault(int sig)
> +{
> +	static const char msg[] = "[FAIL]\tparent guarded control stack faulted\n";

This isn't KTAP format.

> +		/*
> +		 * An empty GCS leaves GCSPR at the exclusive end. If
> +		 * the next VMA is contiguous that address is also the
> +		 * start of the next range; prefer the mapping that
> +		 * ends here.
> +		 */

I'm not clear what you mean by exclusive end here?

> +static int skip_enable(long rc)
> +{
> +	if (rc == -EOPNOTSUPP) {
> +		printf("[SKIP]\tkernel has no GCS\n");
> +	} else if (rc == -EINVAL) {
> +		printf("[SKIP]\tkernel built without GCS\n");
> +	} else if (rc == -EBUSY) {
> +		printf("[SKIP]\tGCS is locked\n");
> +	} else {
> +		printf("[FAIL]\tPR_SET_SHADOW_STACK_STATUS returned %ld\n", rc);
> +		return 1;
> +	}
> +	return KSFT_SKIP;
> +}

This feels rather contorted, and ends up returning a raw 1 from main()
rather than a KSFT_ exit code.

> +int main(void)
> +{
> +	struct sigaction sa;
> +	struct ss_span parent;
> +	void *stack;
> +	void *bad_pidfd;
> +	unsigned long gcspr, gcspr_mid;
> +	unsigned long gcs_mode;
> +	long rc;
> +	int pidfd;
> +	int vmas0, kb0, vmas1, kb1;
> +	int i, kept, d, cret;
> +
> +	setbuf(stdout, NULL);

There's also setvbuf().  Using kselftest.h and it's helpers would
probably be better, it takes care of disabling buffering.  I can't see
any reason to avoid that.

> +	gcspr = (unsigned long)get_gcspr();
> +	if (!gcspr) {
> +		printf("[FAIL]\tenable left GCSPR_EL0 at 0\n");
> +		_exit(1);
> +	}

Why are we using _exit(), and more of these raw 1s?

> +	gcspr = (unsigned long)get_gcspr();
> +	ss_scan(&vmas0, &kb0, gcspr, &parent, NULL, NULL);
> +	if (!parent.start) {
> +		printf("[FAIL]\tGCSPR %lx matches no shadow-stack VMA\n", gcspr);
> +		_exit(1);
> +	}

Why is this something we're validating, and how do we expect to be able
to do function calls if it is the case?

> +	if (vmas1 != vmas0 || kb1 != kb0) {
> +		printf("[FAIL]\tleaked %d shadow-stack VMA(s), %d kB\n",
> +		       vmas1 - vmas0, kb1 - kb0);
> +		_exit(1);
> +	}
> +	if (!kept) {
> +		printf("[FAIL]\tparent GCS %lx-%lx was unmapped\n",
> +		       parent.start, parent.end);
> +		_exit(2);
> +	}

Why are some failures exiting with a different exit code?

> +	if (d != 17) {
> +		printf("[FAIL]\tparent call/return returned %d, want 17\n", d);
> +		_exit(2);
> +	}

...because?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]