Received-SPF: pass (google.com: domain of linux-kernel+bounces-2438-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
References: <20231122-arm64-gcs-v7-0-201c483bd775@kernel.org>
 <20231122-arm64-gcs-v7-36-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 36/39] selftests/arm64: Add GCS signal tests
In-reply-to: <20231122-arm64-gcs-v7-36-201c483bd775@kernel.org>
Date: Sat, 16 Dec 2023 23:12:37 -0300
Message-ID: <875y0x7f1m.fsf@linaro.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain


Mark Brown <broonie@kernel.org> writes:

> diff --git a/tools/testing/selftests/arm64/signal/testcases/gcs_exception_fault.c b/tools/testing/selftests/arm64/signal/testcases/gcs_exception_fault.c
> new file mode 100644
> index 000000000000..532d533592a1
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/gcs_exception_fault.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 ARM Limited
> + */
> +
> +#include <errno.h>
> +#include <signal.h>
> +#include <unistd.h>
> +
> +#include <sys/mman.h>
> +#include <sys/prctl.h>
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +/* This should be includable from some standard header, but which? */
> +#ifndef SEGV_CPERR
> +#define SEGV_CPERR 10
> +#endif

One suggestion is include/uapi/asm-generic/siginfo.h. It already has
SEGV_MTEAERR and SEGV_MTESERR, as well as si_codes specific to other
arches.

From there, it should find its way to glibc's
sysdeps/unix/sysv/linux/bits/siginfo-consts.h.

> +static int gcs_regs(struct tdescr *td, siginfo_t *si, ucontext_t *uc)
> +{
> +	size_t offset;
> +	struct _aarch64_ctx *head = GET_BUF_RESV_HEAD(context);
> +	struct gcs_context *gcs;
> +	unsigned long expected, gcspr;
> +	int ret;
> +
> +	ret = prctl(PR_GET_SHADOW_STACK_STATUS, &expected, 0, 0, 0);
> +	if (ret != 0) {
> +		fprintf(stderr, "Unable to query GCS status\n");
> +		return 1;
> +	}
> +
> +	/* We expect a cap to be added to the GCS in the signal frame */
> +	gcspr = get_gcspr_el0();
> +	gcspr -= 8;
> +	fprintf(stderr, "Expecting GCSPR_EL0 %lx\n", gcspr);
> +
> +	if (!get_current_context(td, &context.uc, sizeof(context))) {
> +		fprintf(stderr, "Failed getting context\n");
> +		return 1;
> +	}

At this point, before any function call is made, can the test check that
*(gcspr + 8) == 0? This would detect the issue I mentioned in
patch 24 of gcs_restore_signal() not zeroing the location of the cap.

> +	fprintf(stderr, "Got context\n");
> +
> +	head = get_header(head, GCS_MAGIC, GET_BUF_RESV_SIZE(context),
> +			  &offset);
> +	if (!head) {
> +		fprintf(stderr, "No GCS context\n");
> +		return 1;
> +	}
> +
> +	gcs = (struct gcs_context *)head;
> +
> +	/* Basic size validation is done in get_current_context() */
> +
> +	if (gcs->features_enabled != expected) {
> +		fprintf(stderr, "Features enabled %llx but expected %lx\n",
> +			gcs->features_enabled, expected);
> +		return 1;
> +	}
> +
> +	if (gcs->gcspr != gcspr) {
> +		fprintf(stderr, "Got GCSPR %llx but expected %lx\n",
> +			gcs->gcspr, gcspr);
> +		return 1;
> +	}

I suggest adding a new check here to ensure that gcs->reserved == 0.

> +	fprintf(stderr, "GCS context validated\n");
> +	td->pass = 1;
> +
> +	return 0;
> +}
> +
> +struct tdescr tde = {
> +	.name = "GCS basics",
> +	.descr = "Validate a GCS signal context",
> +	.feats_required = FEAT_GCS,
> +	.timeout = 3,
> +	.run = gcs_regs,
> +};
> diff --git a/tools/testing/selftests/arm64/signal/testcases/gcs_write_fault.c b/tools/testing/selftests/arm64/signal/testcases/gcs_write_fault.c
> new file mode 100644
> index 000000000000..126b1a294a29
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/gcs_write_fault.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 ARM Limited
> + */
> +
> +#include <errno.h>
> +#include <signal.h>
> +#include <unistd.h>
> +
> +#include <sys/mman.h>
> +#include <sys/prctl.h>
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +static uint64_t *gcs_page;
> +
> +#ifndef __NR_map_shadow_stack
> +#define __NR_map_shadow_stack 452
> +#endif
> +
> +static bool alloc_gcs(struct tdescr *td)
> +{
> +	long page_size = sysconf(_SC_PAGE_SIZE);
> +
> +	gcs_page = (void *)syscall(__NR_map_shadow_stack, 0,
> +				   page_size, 0);
> +	if (gcs_page == MAP_FAILED) {
> +		fprintf(stderr, "Failed to map %ld byte GCS: %d\n",
> +			page_size, errno);

This call is failing with EINVAL for me:

# timeout set to 45
# selftests: arm64/signal: gcs_write_fault
# # GCS write fault :: Normal writes to a GCS segfault
# Registered handlers for all signals.
# Detected MINSTKSIGSZ:4720
# Required Features: [ GCS ] supported
# Incompatible Features: [] absent
# Failed to map 4096 byte GCS: 22
# FAILED Testcase initialization.
# ==>> completed. FAIL(0)
not ok 11 selftests: arm64/signal: gcs_write_fault # exit=1

> +		return false;
> +	}
> +
> +	return true;
> +}

-- 
Thiago