Received-SPF: pass (google.com: domain of linux-kernel+bounces-378-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
References: <20231122-arm64-gcs-v7-0-201c483bd775@kernel.org>
 <20231122-arm64-gcs-v7-34-201c483bd775@kernel.org>
User-agent: mu4e 1.10.8; emacs 29.1
From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon
 <will@kernel.org>, Jonathan Corbet <corbet@lwn.net>, Andrew Morton
 <akpm@linux-foundation.org>, Marc Zyngier <maz@kernel.org>, Oliver Upton
 <oliver.upton@linux.dev>, James Morse <james.morse@arm.com>, Suzuki K
 Poulose <suzuki.poulose@arm.com>, Arnd Bergmann <arnd@arndb.de>, Oleg
 Nesterov <oleg@redhat.com>, Eric Biederman <ebiederm@xmission.com>, Kees
 Cook <keescook@chromium.org>, Shuah Khan <shuah@kernel.org>, "Rick P.
 Edgecombe" <rick.p.edgecombe@intel.com>, Deepak Gupta
 <debug@rivosinc.com>, Ard Biesheuvel <ardb@kernel.org>, Szabolcs Nagy
 <Szabolcs.Nagy@arm.com>, "H.J. Lu" <hjl.tools@gmail.com>, Paul Walmsley
 <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou
 <aou@eecs.berkeley.edu>, Florian Weimer <fweimer@redhat.com>, Christian
 Brauner <brauner@kernel.org>, linux-arm-kernel@lists.infradead.org,
 linux-doc@vger.kernel.org, kvmarm@lists.linux.dev,
 linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org,
 linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v7 34/39] kselftest/arm64: Add a GCS test program built
 with the system libc
In-reply-to: <20231122-arm64-gcs-v7-34-201c483bd775@kernel.org>
Date: Thu, 14 Dec 2023 23:50:11 -0300
Message-ID: <875y1089i4.fsf@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain


Mark Brown <broonie@kernel.org> writes:

> +	/* Same thing via process_vm_readv() */
> +	local_iov.iov_base = &rval;
> +	local_iov.iov_len = sizeof(rval);
> +	remote_iov.iov_base = (void *)gcspr;
> +	remote_iov.iov_len = sizeof(rval);
> +	ret = process_vm_writev(child, &local_iov, 1, &remote_iov, 1, 0);
> +	if (ret == -1)
> +		ksft_print_msg("process_vm_readv() failed: %s (%d)\n",
> +			       strerror(errno), errno);

The comment and the error message say "process_vm_readv()", but the
function actually called is process_vm_writev(). Is this intended?

Also, process_vm_writev() is failing when I run on my Arm FVP:

# #  RUN           global.ptrace_read_write ...
# # Child: 1150
# # Child GCSPR 0xffffa210ffd8, flags 1, locked 0
# # process_vm_readv() failed: Bad address (14)
# # libc-gcs.c:271:ptrace_read_write:Expected ret (-1) == sizeof(rval) (8)
# # libc-gcs.c:272:ptrace_read_write:Expected val (281473401005692) == rval (281473402849248)
# # libc-gcs.c:293:ptrace_read_write:Expected val (281473401005692) == ptrace(PTRACE_PEEKDATA, child, (void *)gcspr, NULL) (0)
# # ptrace_read_write: Test failed at step #1
# #          FAIL  global.ptrace_read_write
# not ok 4 global.ptrace_read_write

If I swap process_vm_readv() and process_vm_writev(), then the read
succeeds but the write fails:

#  RUN           global.ptrace_read_write ...
# Child: 1996
# Child GCSPR 0xffffa7fcffd8, flags 1, locked 0
# process_vm_writev() failed: Bad address (14)
# libc-gcs.c:291:ptrace_read_write:Expected ret (-1) == sizeof(rval) (8)
# libc-gcs.c:293:ptrace_read_write:Expected val (281473500358268) == ptrace(PTRACE_PEEKDATA, child, (void *)gcspr, NULL) (0)
# ptrace_read_write: Test failed at step #1
#          FAIL  global.ptrace_read_write
not ok 4 global.ptrace_read_write

> +/* Put it all together, we can safely switch to and from the stack */
> +TEST_F(map_gcs, stack_switch)
> +{
> +	size_t cap_index;
> +	cap_index = (variant->stack_size / sizeof(unsigned long));
> +	unsigned long *orig_gcspr_el0, *pivot_gcspr_el0;
> +
> +	/* Skip over the stack terminator and point at the cap */
> + switch (variant->flags & (SHADOW_STACK_SET_MARKER | SHADOW_STACK_SET_TOKEN)) {
> +	case SHADOW_STACK_SET_MARKER | SHADOW_STACK_SET_TOKEN:
> +		cap_index -= 2;
> +		break;
> +	case SHADOW_STACK_SET_TOKEN:
> +		cap_index -= 1;
> +		break;
> +	case SHADOW_STACK_SET_MARKER:
> +	case 0:
> +		/* No cap, no test */
> +		return;
> +	}
> +	pivot_gcspr_el0 = &self->stack[cap_index];
> +
> +	/* Pivot to the new GCS */
> +	ksft_print_msg("Pivoting to %p from %p, target has value 0x%lx\n",
> +		       pivot_gcspr_el0, get_gcspr(),
> +		       *pivot_gcspr_el0);
> +	gcsss1(pivot_gcspr_el0);
> +	orig_gcspr_el0 = gcsss2();
> +	ksft_print_msg("Pivoted to %p from %p, target has value 0x%lx\n",
> +		       pivot_gcspr_el0, get_gcspr(),

Not sure about the intent here, but perhaps "get_gcspr()" here should be
"orig_gcspr_el0" instead? Ditto in the equivalent place at the
map_gcs.stack_overflow test below.

Also, it's strange that the tests defined after map_gcs.stack_overflow
don't run when I execute this test program. I'm doing:

$ ./run_kselftest.sh -t arm64:libc-gcs

I.e., these tests aren't being run in my FVP:

> +FIXTURE_VARIANT_ADD(map_invalid_gcs, too_small)
> +FIXTURE_VARIANT_ADD(map_invalid_gcs, unligned_1)
> +FIXTURE_VARIANT_ADD(map_invalid_gcs, unligned_2)
> +FIXTURE_VARIANT_ADD(map_invalid_gcs, unligned_3)
> +FIXTURE_VARIANT_ADD(map_invalid_gcs, unligned_4)
> +FIXTURE_VARIANT_ADD(map_invalid_gcs, unligned_5)
> +FIXTURE_VARIANT_ADD(map_invalid_gcs, unligned_6)
> +FIXTURE_VARIANT_ADD(map_invalid_gcs, unligned_7)
> +TEST_F(map_invalid_gcs, do_map)
> +FIXTURE_VARIANT_ADD(invalid_mprotect, exec)
> +FIXTURE_VARIANT_ADD(invalid_mprotect, bti)
> +FIXTURE_VARIANT_ADD(invalid_mprotect, exec_bti)
> +TEST_F(invalid_mprotect, do_map)
> +TEST_F(invalid_mprotect, do_map_read)

Finally, one last comment:

> +int main(int argc, char **argv)
> +{
> +	unsigned long gcs_mode;
> +	int ret;
> +
> +	if (!(getauxval(AT_HWCAP2) & HWCAP2_GCS))
> +		ksft_exit_skip("SKIP GCS not supported\n");
> +
> +	/* 
> +	 * Force shadow stacks on, our tests *should* be fine with or
> +	 * without libc support and with or without this having ended
> +	 * up tagged for GCS and enabled by the dynamic linker.  We
> +	 * can't use the libc prctl() function since we can't return
> +	 * from enabling the stack.  Also lock GCS if not already
> +	 * locked so we can test behaviour when it's locked.

This is probably a leftover from a previous version: the test doesn't
lock any GCS flag.

> +	 */
> +	ret = my_syscall2(__NR_prctl, PR_GET_SHADOW_STACK_STATUS, &gcs_mode);
> +	if (ret) {
> +		ksft_print_msg("Failed to read GCS state: %d\n", ret);
> +		return EXIT_FAILURE;
> +	}
> +	
> +	if (!(gcs_mode & PR_SHADOW_STACK_ENABLE)) {
> +		gcs_mode = PR_SHADOW_STACK_ENABLE;
> +		ret = my_syscall2(__NR_prctl, PR_SET_SHADOW_STACK_STATUS,
> +				  gcs_mode);
> +		if (ret) {
> +			ksft_print_msg("Failed to configure GCS: %d\n", ret);
> +			return EXIT_FAILURE;
> +		}
> +	}
> +
> +	/* Avoid returning in case libc doesn't understand GCS */
> +	exit(test_harness_run(argc, argv));
> +}


-- 
Thiago