LinuxLists.cc - [PATCH 0/4] kselftests/arm64: add PAuth tests

[permalink] [raw]

Subject: [PATCH 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

Kernel documentation states that it will change PAuth keys on exec() calls.

Verify that all keys are correctly switched to new ones.

Cc: Shuah Khan <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Signed-off-by: Boyan Karatotev <[email protected]>
---
tools/testing/selftests/arm64/pauth/Makefile | 4 +
.../selftests/arm64/pauth/exec_target.c | 35 +++++
tools/testing/selftests/arm64/pauth/helper.h | 10 ++
tools/testing/selftests/arm64/pauth/pac.c | 148 ++++++++++++++++++
4 files changed, 197 insertions(+)
create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c

diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
index a017d1c8dd58..2e237b21ccf6 100644
--- a/tools/testing/selftests/arm64/pauth/Makefile
+++ b/tools/testing/selftests/arm64/pauth/Makefile
@@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret

TEST_GEN_PROGS := pac
TEST_GEN_FILES := pac_corruptor.o helper.o
+TEST_GEN_PROGS_EXTENDED := exec_target

include ../../lib.mk

@@ -20,6 +21,9 @@ $(OUTPUT)/helper.o: helper.c
# greater, gcc emits pac* instructions which are not in HINT NOP space,
# preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
# run on earlier targets and print a meaningful error messages
+$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
+ $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
+
$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a

diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
new file mode 100644
index 000000000000..07addef5a1d7
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/exec_target.c
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/auxv.h>
+
+#include "helper.h"
+
+
+int main(void)
+{
+ struct signatures signed_vals;
+ unsigned long hwcaps;
+ size_t val;
+
+ fread(&val, sizeof(size_t), 1, stdin);
+
+ /* don't try to execute illegal (unimplemented) instructions) caller
+ * should have checked this and keep worker simple
+ */
+ hwcaps = getauxval(AT_HWCAP);
+
+ if (hwcaps & HWCAP_PACA) {
+ signed_vals.keyia = keyia_sign(val);
+ signed_vals.keyib = keyib_sign(val);
+ signed_vals.keyda = keyda_sign(val);
+ signed_vals.keydb = keydb_sign(val);
+ }
+ signed_vals.keyg = (hwcaps & HWCAP_PACG) ? keyg_sign(val) : 0;
+
+ fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
+
+ return 0;
+}
diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
index b3cf709e249d..fceaa1e4824a 100644
--- a/tools/testing/selftests/arm64/pauth/helper.h
+++ b/tools/testing/selftests/arm64/pauth/helper.h
@@ -6,6 +6,16 @@

#include <stdlib.h>

+#define NKEYS 5
+
+
+struct signatures {
+ size_t keyia;
+ size_t keyib;
+ size_t keyda;
+ size_t keydb;
+ size_t keyg;
+};

void pac_corruptor(void);

diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
index cdbffa8bf61e..16dea47b11c7 100644
--- a/tools/testing/selftests/arm64/pauth/pac.c
+++ b/tools/testing/selftests/arm64/pauth/pac.c
@@ -2,6 +2,8 @@
// Copyright (C) 2020 ARM Limited

#include <sys/auxv.h>
+#include <sys/types.h>
+#include <sys/wait.h>
#include <signal.h>

#include "../../kselftest_harness.h"
@@ -33,6 +35,117 @@ do { \
} while (0)

+void sign_specific(struct signatures *sign, size_t val)
+{
+ sign->keyia = keyia_sign(val);
+ sign->keyib = keyib_sign(val);
+ sign->keyda = keyda_sign(val);
+ sign->keydb = keydb_sign(val);
+}
+
+void sign_all(struct signatures *sign, size_t val)
+{
+ sign->keyia = keyia_sign(val);
+ sign->keyib = keyib_sign(val);
+ sign->keyda = keyda_sign(val);
+ sign->keydb = keydb_sign(val);
+ sign->keyg = keyg_sign(val);
+}
+
+int are_same(struct signatures *old, struct signatures *new, int nkeys)
+{
+ int res = 0;
+
+ res |= old->keyia == new->keyia;
+ res |= old->keyib == new->keyib;
+ res |= old->keyda == new->keyda;
+ res |= old->keydb == new->keydb;
+ if (nkeys == NKEYS)
+ res |= old->keyg == new->keyg;
+
+ return res;
+}
+
+int exec_sign_all(struct signatures *signed_vals, size_t val)
+{
+ int new_stdin[2];
+ int new_stdout[2];
+ int status;
+ ssize_t ret;
+ pid_t pid;
+
+ ret = pipe(new_stdin);
+ if (ret == -1) {
+ perror("pipe returned error");
+ return -1;
+ }
+
+ ret = pipe(new_stdout);
+ if (ret == -1) {
+ perror("pipe returned error");
+ return -1;
+ }
+
+ pid = fork();
+ // child
+ if (pid == 0) {
+ dup2(new_stdin[0], STDIN_FILENO);
+ if (ret == -1) {
+ perror("dup2 returned error");
+ exit(1);
+ }
+
+ dup2(new_stdout[1], STDOUT_FILENO);
+ if (ret == -1) {
+ perror("dup2 returned error");
+ exit(1);
+ }
+
+ close(new_stdin[0]);
+ close(new_stdin[1]);
+ close(new_stdout[0]);
+ close(new_stdout[1]);
+
+ ret = execl("exec_target", "exec_target", (char *) NULL);
+ if (ret == -1) {
+ perror("exec returned error");
+ exit(1);
+ }
+ }
+
+ close(new_stdin[0]);
+ close(new_stdout[1]);
+
+ ret = write(new_stdin[1], &val, sizeof(size_t));
+ if (ret == -1) {
+ perror("write returned error");
+ return -1;
+ }
+
+ /*
+ * wait for the worker to finish, so that read() reads all data
+ * will also context switch with worker so that this function can be used
+ * for context switch tests
+ */
+ waitpid(pid, &status, 0);
+ if (WIFEXITED(status) == 0) {
+ fprintf(stderr, "worker exited unexpectedly\n");
+ return -1;
+ }
+ if (WEXITSTATUS(status) != 0) {
+ fprintf(stderr, "worker exited with error\n");
+ return -1;
+ }
+
+ ret = read(new_stdout[0], signed_vals, sizeof(struct signatures));
+ if (ret == -1) {
+ perror("read returned error");
+ return -1;
+ }
+
+ return 0;
+}
+
/* check that a corrupted PAC results in SIGSEGV */
TEST_SIGNAL(corrupt_pac, SIGSEGV)
{
@@ -79,5 +192,40 @@ TEST(pac_instructions_not_nop_generic)
ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
}

+/*
+ * fork() does not change keys. Only exec() does so call a worker program.
+ * Its only job is to sign a value and report back the resutls
+ */
+TEST(exec_unique_keys)
+{
+ struct signatures new_keys;
+ struct signatures old_keys;
+ int ret;
+ int different = 0;
+ int nkeys = NKEYS;
+ unsigned long hwcaps = getauxval(AT_HWCAP);
+
+ /* generic and data key instructions are not in NOP space. This prevents a SIGILL */
+ ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
+ if (!(hwcaps & HWCAP_PACG)) {
+ TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
+ nkeys = NKEYS - 1;
+ }
+
+ for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
+ ret = exec_sign_all(&new_keys, i);
+ ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
+
+ if (nkeys == NKEYS)
+ sign_all(&old_keys, i);
+ else
+ sign_specific(&old_keys, i);
+
+ different |= !are_same(&old_keys, &new_keys, nkeys);
+ }
+
+ ASSERT_EQ(1, different) TH_LOG("exec() did not change keys");
+}
+
TEST_HARNESS_MAIN

--
2.17.1

2020-08-28 13:26:02

[permalink] [raw]

Subject: [PATCH 1/4] kselftests/arm64: add a basic Pointer Authentication test

PAuth signs and verifies return addresses on the stack. It does so by
inserting a Pointer Authentication code (PAC) into some of the unused top
bits of an address. This is achieved by adding paciasp/autiasp instructions
at the beginning and end of a function.

This feature is partially backwards compatible with earlier versions of the
ARM architecture. To coerce the compiler into emitting fully backwards
compatible code the main file is compiled to target an earlier ARM version.
This allows the tests to check for the feature and print meaningful error
messages instead of crashing.

Add a test to verify that corrupting the return address results in a
SIGSEGV on return.

Cc: Shuah Khan <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Signed-off-by: Boyan Karatotev <[email protected]>
---
tools/testing/selftests/arm64/Makefile | 2 +-
.../testing/selftests/arm64/pauth/.gitignore | 1 +
tools/testing/selftests/arm64/pauth/Makefile | 22 ++++++++++++
tools/testing/selftests/arm64/pauth/helper.h | 10 ++++++
tools/testing/selftests/arm64/pauth/pac.c | 32 +++++++++++++++++
.../selftests/arm64/pauth/pac_corruptor.S | 36 +++++++++++++++++++
6 files changed, 102 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/arm64/pauth/.gitignore
create mode 100644 tools/testing/selftests/arm64/pauth/Makefile
create mode 100644 tools/testing/selftests/arm64/pauth/helper.h
create mode 100644 tools/testing/selftests/arm64/pauth/pac.c
create mode 100644 tools/testing/selftests/arm64/pauth/pac_corruptor.S

diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
index 93b567d23c8b..525506fd97b9 100644
--- a/tools/testing/selftests/arm64/Makefile
+++ b/tools/testing/selftests/arm64/Makefile
@@ -4,7 +4,7 @@
ARCH ?= $(shell uname -m 2>/dev/null || echo not)

ifneq (,$(filter $(ARCH),aarch64 arm64))
-ARM64_SUBTARGETS ?= tags signal
+ARM64_SUBTARGETS ?= tags signal pauth
else
ARM64_SUBTARGETS :=
endif
diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
new file mode 100644
index 000000000000..b557c916720a
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/.gitignore
@@ -0,0 +1 @@
+pac
diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
new file mode 100644
index 000000000000..785c775e5e41
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/Makefile
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2020 ARM Limited
+
+CFLAGS += -mbranch-protection=pac-ret
+
+TEST_GEN_PROGS := pac
+TEST_GEN_FILES := pac_corruptor.o
+
+include ../../lib.mk
+
+# pac* and aut* instructions are not available on architectures berfore
+# ARMv8.3. Therefore target ARMv8.3 wherever they are used directly
+$(OUTPUT)/pac_corruptor.o: pac_corruptor.S
+ $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
+
+# when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
+# greater, gcc emits pac* instructions which are not in HINT NOP space,
+# preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
+# run on earlier targets and print a meaningful error messages
+$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
+ $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
+
diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
new file mode 100644
index 000000000000..f777f88acf0a
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/helper.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2020 ARM Limited */
+
+#ifndef _HELPER_H_
+#define _HELPER_H_
+
+void pac_corruptor(void);
+
+#endif
+
diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
new file mode 100644
index 000000000000..ed445050f621
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/pac.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#include <sys/auxv.h>
+#include <signal.h>
+
+#include "../../kselftest_harness.h"
+#include "helper.h"
+
+/*
+ * Tests are ARMv8.3 compliant. They make no provisions for features present in
+ * future version of the arm architecture
+ */
+
+#define ASSERT_PAUTH_ENABLED() \
+do { \
+ unsigned long hwcaps = getauxval(AT_HWCAP); \
+ /* data key instructions are not in NOP space. This prevents a SIGILL */ \
+ ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
+} while (0)
+
+
+/* check that a corrupted PAC results in SIGSEGV */
+TEST_SIGNAL(corrupt_pac, SIGSEGV)
+{
+ ASSERT_PAUTH_ENABLED();
+
+ pac_corruptor();
+}
+
+TEST_HARNESS_MAIN
+
diff --git a/tools/testing/selftests/arm64/pauth/pac_corruptor.S b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
new file mode 100644
index 000000000000..6a34ec23a034
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2020 ARM Limited */
+
+.global pac_corruptor
+
+.text
+/*
+ * Corrupting a single bit of the PAC ensures the authentication will fail. It
+ * also guarantees no possible collision. TCR_EL1.TBI0 is set by default so no
+ * top byte PAC is tested
+ */
+ pac_corruptor:
+ paciasp
+
+ /* make stack frame */
+ sub sp, sp, #16
+ stp x29, lr, [sp]
+ mov x29, sp
+
+ /* prepare mask for bit to be corrupted (bit 54) */
+ mov x1, xzr
+ add x1, x1, #1
+ lsl x1, x1, #54
+
+ /* get saved lr, corrupt selected bit, put it back */
+ ldr x0, [sp, #8]
+ eor x0, x0, x1
+ str x0, [sp, #8]
+
+ /* remove stack frame */
+ ldp x29, lr, [sp]
+ add sp, sp, #16
+
+ autiasp
+ ret
+
--
2.17.1

2020-08-28 13:26:41

[permalink] [raw]

Subject: [PATCH 2/4] kselftests/arm64: add nop checks for PAuth tests

PAuth adds sign/verify controls to enable and disable groups of
instructions in hardware for compatibility with libraries that do not
implement PAuth. The kernel always enables them if it detects PAuth.

Add a test that checks that each group of instructions is enabled, if the
kernel reports PAuth as detected.

Note: For groups, for the purpose of this patch, we intend instructions
that use a certain key.

Cc: Shuah Khan <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Signed-off-by: Boyan Karatotev <[email protected]>
---
.../testing/selftests/arm64/pauth/.gitignore | 1 +
tools/testing/selftests/arm64/pauth/Makefile | 7 ++-
tools/testing/selftests/arm64/pauth/helper.c | 41 +++++++++++++++
tools/testing/selftests/arm64/pauth/helper.h | 10 ++++
tools/testing/selftests/arm64/pauth/pac.c | 51 +++++++++++++++++++
5 files changed, 108 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/arm64/pauth/helper.c

diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
index b557c916720a..155137d92722 100644
--- a/tools/testing/selftests/arm64/pauth/.gitignore
+++ b/tools/testing/selftests/arm64/pauth/.gitignore
@@ -1 +1,2 @@
+exec_target
pac
diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
index 785c775e5e41..a017d1c8dd58 100644
--- a/tools/testing/selftests/arm64/pauth/Makefile
+++ b/tools/testing/selftests/arm64/pauth/Makefile
@@ -4,7 +4,7 @@
CFLAGS += -mbranch-protection=pac-ret

TEST_GEN_PROGS := pac
-TEST_GEN_FILES := pac_corruptor.o
+TEST_GEN_FILES := pac_corruptor.o helper.o

include ../../lib.mk

@@ -13,10 +13,13 @@ include ../../lib.mk
$(OUTPUT)/pac_corruptor.o: pac_corruptor.S
$(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a

+$(OUTPUT)/helper.o: helper.c
+ $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
+
# when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
# greater, gcc emits pac* instructions which are not in HINT NOP space,
# preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
# run on earlier targets and print a meaningful error messages
-$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
+$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a

diff --git a/tools/testing/selftests/arm64/pauth/helper.c b/tools/testing/selftests/arm64/pauth/helper.c
new file mode 100644
index 000000000000..8619afb16124
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/helper.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#include "helper.h"
+
+
+size_t keyia_sign(size_t ptr)
+{
+ asm volatile("paciza %0" : "+r" (ptr));
+ return ptr;
+}
+
+size_t keyib_sign(size_t ptr)
+{
+ asm volatile("pacizb %0" : "+r" (ptr));
+ return ptr;
+}
+
+size_t keyda_sign(size_t ptr)
+{
+ asm volatile("pacdza %0" : "+r" (ptr));
+ return ptr;
+}
+
+size_t keydb_sign(size_t ptr)
+{
+ asm volatile("pacdzb %0" : "+r" (ptr));
+ return ptr;
+}
+
+size_t keyg_sign(size_t ptr)
+{
+ /* output is encoded in the upper 32 bits */
+ size_t dest = 0;
+ size_t modifier = 0;
+
+ asm volatile("pacga %0, %1, %2" : "=r" (dest) : "r" (ptr), "r" (modifier));
+
+ return dest;
+}
+
diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
index f777f88acf0a..b3cf709e249d 100644
--- a/tools/testing/selftests/arm64/pauth/helper.h
+++ b/tools/testing/selftests/arm64/pauth/helper.h
@@ -4,7 +4,17 @@
#ifndef _HELPER_H_
#define _HELPER_H_

+#include <stdlib.h>
+
+
void pac_corruptor(void);

+/* PAuth sign a value with key ia and modifier value 0 */
+size_t keyia_sign(size_t val);
+size_t keyib_sign(size_t val);
+size_t keyda_sign(size_t val);
+size_t keydb_sign(size_t val);
+size_t keyg_sign(size_t val);
+
#endif

diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
index ed445050f621..cdbffa8bf61e 100644
--- a/tools/testing/selftests/arm64/pauth/pac.c
+++ b/tools/testing/selftests/arm64/pauth/pac.c
@@ -12,12 +12,25 @@
* future version of the arm architecture
*/

+#define PAC_COLLISION_ATTEMPTS 10
+/*
+ * The kernel sets TBID by default. So bits 55 and above should remain
+ * untouched no matter what.
+ * The VA space size is 48 bits. Bigger is opt-in.
+ */
+#define PAC_MASK (~0xff80ffffffffffff)
#define ASSERT_PAUTH_ENABLED() \
do { \
unsigned long hwcaps = getauxval(AT_HWCAP); \
/* data key instructions are not in NOP space. This prevents a SIGILL */ \
ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
} while (0)
+#define ASSERT_GENERIC_PAUTH_ENABLED() \
+do { \
+ unsigned long hwcaps = getauxval(AT_HWCAP); \
+ /* generic key instructions are not in NOP space. This prevents a SIGILL */ \
+ ASSERT_NE(0, hwcaps & HWCAP_PACG) TH_LOG("Generic PAUTH not enabled"); \
+} while (0)

/* check that a corrupted PAC results in SIGSEGV */
@@ -28,5 +41,43 @@ TEST_SIGNAL(corrupt_pac, SIGSEGV)
pac_corruptor();
}

+/*
+ * There are no separate pac* and aut* controls so checking only the pac*
+ * instructions is sufficient
+ */
+TEST(pac_instructions_not_nop)
+{
+ size_t keyia = 0;
+ size_t keyib = 0;
+ size_t keyda = 0;
+ size_t keydb = 0;
+
+ ASSERT_PAUTH_ENABLED();
+
+ for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
+ keyia |= keyia_sign(i) & PAC_MASK;
+ keyib |= keyib_sign(i) & PAC_MASK;
+ keyda |= keyda_sign(i) & PAC_MASK;
+ keydb |= keydb_sign(i) & PAC_MASK;
+ }
+
+ ASSERT_NE(0, keyia) TH_LOG("keyia instructions did nothing");
+ ASSERT_NE(0, keyib) TH_LOG("keyib instructions did nothing");
+ ASSERT_NE(0, keyda) TH_LOG("keyda instructions did nothing");
+ ASSERT_NE(0, keydb) TH_LOG("keydb instructions did nothing");
+}
+
+TEST(pac_instructions_not_nop_generic)
+{
+ size_t keyg = 0;
+
+ ASSERT_GENERIC_PAUTH_ENABLED();
+
+ for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++)
+ keyg |= keyg_sign(i) & PAC_MASK;
+
+ ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
+}
+
TEST_HARNESS_MAIN

--
2.17.1

2020-08-28 13:27:30

[permalink] [raw]

Subject: [PATCH 4/4] kselftests/arm64: add PAuth tests for single threaded consistency and key uniqueness

PAuth adds 5 different keys that can be used to sign addresses.

Add a test that verifies that the kernel initializes them uniquely and
preserves them across context switches.

Cc: Shuah Khan <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Signed-off-by: Boyan Karatotev <[email protected]>
---
tools/testing/selftests/arm64/pauth/pac.c | 116 ++++++++++++++++++++++
1 file changed, 116 insertions(+)

diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
index 16dea47b11c7..718f49adc275 100644
--- a/tools/testing/selftests/arm64/pauth/pac.c
+++ b/tools/testing/selftests/arm64/pauth/pac.c
@@ -1,10 +1,13 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (C) 2020 ARM Limited

+#define _GNU_SOURCE
+
#include <sys/auxv.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <signal.h>
+#include <sched.h>

#include "../../kselftest_harness.h"
#include "helper.h"
@@ -21,6 +24,7 @@
* The VA space size is 48 bits. Bigger is opt-in.
*/
#define PAC_MASK (~0xff80ffffffffffff)
+#define ARBITRARY_VALUE (0x1234)
#define ASSERT_PAUTH_ENABLED() \
do { \
unsigned long hwcaps = getauxval(AT_HWCAP); \
@@ -66,13 +70,36 @@ int are_same(struct signatures *old, struct signatures *new, int nkeys)
return res;
}

+int are_unique(struct signatures *sign, int nkeys)
+{
+ size_t vals[nkeys];
+
+ vals[0] = sign->keyia & PAC_MASK;
+ vals[1] = sign->keyib & PAC_MASK;
+ vals[2] = sign->keyda & PAC_MASK;
+ vals[3] = sign->keydb & PAC_MASK;
+
+ if (nkeys >= 4)
+ vals[4] = sign->keyg & PAC_MASK;
+
+ for (int i = 0; i < nkeys - 1; i++) {
+ for (int j = i + 1; j < nkeys; j++) {
+ if (vals[i] == vals[j])
+ return 0;
+ }
+ }
+ return 1;
+}
+
int exec_sign_all(struct signatures *signed_vals, size_t val)
{
int new_stdin[2];
int new_stdout[2];
int status;
+ int i;
ssize_t ret;
pid_t pid;
+ cpu_set_t mask;

ret = pipe(new_stdin);
if (ret == -1) {
@@ -86,6 +113,20 @@ int exec_sign_all(struct signatures *signed_vals, size_t val)
return -1;
}

+ /*
+ * pin this process and all its children to a single CPU, so it can also
+ * guarantee a context switch with its child
+ */
+ sched_getaffinity(0, sizeof(mask), &mask);
+
+ for (i = 0; i < sizeof(cpu_set_t); i++)
+ if (CPU_ISSET(i, &mask))
+ break;
+
+ CPU_ZERO(&mask);
+ CPU_SET(i, &mask);
+ sched_setaffinity(0, sizeof(mask), &mask);
+
pid = fork();
// child
if (pid == 0) {
@@ -192,6 +233,38 @@ TEST(pac_instructions_not_nop_generic)
ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
}

+TEST(single_thread_unique_keys)
+{
+ int unique = 0;
+ int nkeys = NKEYS;
+ struct signatures signed_vals;
+ unsigned long hwcaps = getauxval(AT_HWCAP);
+
+ /* generic and data key instructions are not in NOP space. This prevents a SIGILL */
+ ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
+ if (!(hwcaps & HWCAP_PACG)) {
+ TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
+ nkeys = NKEYS - 1;
+ }
+
+ /*
+ * The PAC field is up to 7 bits. Even with unique keys there is about
+ * 5% chance for a collision. This chance rapidly increases the fewer
+ * bits there are, a comparison of the keys directly will be more
+ * reliable All signed values need to be unique at least once out of n
+ * attempts to be certain that the keys are unique
+ */
+ for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
+ if (nkeys == NKEYS)
+ sign_all(&signed_vals, i);
+ else
+ sign_specific(&signed_vals, i);
+ unique |= are_unique(&signed_vals, nkeys);
+ }
+
+ ASSERT_EQ(1, unique) TH_LOG("keys clashed every time");
+}
+
/*
* fork() does not change keys. Only exec() does so call a worker program.
* Its only job is to sign a value and report back the resutls
@@ -227,5 +300,48 @@ TEST(exec_unique_keys)
ASSERT_EQ(1, different) TH_LOG("exec() did not change keys");
}

+TEST(context_switch_keep_keys)
+{
+ int ret;
+ struct signatures trash;
+ struct signatures before;
+ struct signatures after;
+
+ ASSERT_PAUTH_ENABLED();
+
+ sign_specific(&before, ARBITRARY_VALUE);
+
+ /* will context switch with a process with different keys at least once */
+ ret = exec_sign_all(&trash, ARBITRARY_VALUE);
+ ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
+
+ sign_specific(&after, ARBITRARY_VALUE);
+
+ ASSERT_EQ(before.keyia, after.keyia) TH_LOG("keyia changed after context switching");
+ ASSERT_EQ(before.keyib, after.keyib) TH_LOG("keyib changed after context switching");
+ ASSERT_EQ(before.keyda, after.keyda) TH_LOG("keyda changed after context switching");
+ ASSERT_EQ(before.keydb, after.keydb) TH_LOG("keydb changed after context switching");
+}
+
+TEST(context_switch_keep_keys_generic)
+{
+ int ret;
+ struct signatures trash;
+ size_t before;
+ size_t after;
+
+ ASSERT_GENERIC_PAUTH_ENABLED();
+
+ before = keyg_sign(ARBITRARY_VALUE);
+
+ /* will context switch with a process with different keys at least once */
+ ret = exec_sign_all(&trash, ARBITRARY_VALUE);
+ ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
+
+ after = keyg_sign(ARBITRARY_VALUE);
+
+ ASSERT_EQ(before, after) TH_LOG("keyg changed after context switching");
+}
+
TEST_HARNESS_MAIN

--
2.17.1

2020-08-28 14:27:44

[permalink] [raw]

Subject: Re: [PATCH 1/4] kselftests/arm64: add a basic Pointer Authentication test

On 8/28/20 2:16 PM, Boyan Karatotev wrote:
> PAuth signs and verifies return addresses on the stack. It does so by
> inserting a Pointer Authentication code (PAC) into some of the unused top
> bits of an address. This is achieved by adding paciasp/autiasp instructions
> at the beginning and end of a function.
>
> This feature is partially backwards compatible with earlier versions of the
> ARM architecture. To coerce the compiler into emitting fully backwards
> compatible code the main file is compiled to target an earlier ARM version.
> This allows the tests to check for the feature and print meaningful error
> messages instead of crashing.
>
> Add a test to verify that corrupting the return address results in a
> SIGSEGV on return.
>

Reviewed-by: Vincenzo Frascino <[email protected]>

> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>
> ---
> tools/testing/selftests/arm64/Makefile | 2 +-
> .../testing/selftests/arm64/pauth/.gitignore | 1 +
> tools/testing/selftests/arm64/pauth/Makefile | 22 ++++++++++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++++++
> tools/testing/selftests/arm64/pauth/pac.c | 32 +++++++++++++++++
> .../selftests/arm64/pauth/pac_corruptor.S | 36 +++++++++++++++++++
> 6 files changed, 102 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/arm64/pauth/.gitignore
> create mode 100644 tools/testing/selftests/arm64/pauth/Makefile
> create mode 100644 tools/testing/selftests/arm64/pauth/helper.h
> create mode 100644 tools/testing/selftests/arm64/pauth/pac.c
> create mode 100644 tools/testing/selftests/arm64/pauth/pac_corruptor.S
>
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> index 93b567d23c8b..525506fd97b9 100644
> --- a/tools/testing/selftests/arm64/Makefile
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -4,7 +4,7 @@
> ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>
> ifneq (,$(filter $(ARCH),aarch64 arm64))
> -ARM64_SUBTARGETS ?= tags signal
> +ARM64_SUBTARGETS ?= tags signal pauth
> else
> ARM64_SUBTARGETS :=
> endif
> diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
> new file mode 100644
> index 000000000000..b557c916720a
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/.gitignore
> @@ -0,0 +1 @@
> +pac
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> new file mode 100644
> index 000000000000..785c775e5e41
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2020 ARM Limited
> +
> +CFLAGS += -mbranch-protection=pac-ret
> +
> +TEST_GEN_PROGS := pac
> +TEST_GEN_FILES := pac_corruptor.o
> +
> +include ../../lib.mk
> +
> +# pac* and aut* instructions are not available on architectures berfore
> +# ARMv8.3. Therefore target ARMv8.3 wherever they are used directly
> +$(OUTPUT)/pac_corruptor.o: pac_corruptor.S
> + $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
> +
> +# when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
> +# greater, gcc emits pac* instructions which are not in HINT NOP space,
> +# preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
> +# run on earlier targets and print a meaningful error messages
> +$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
> + $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
> +
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> new file mode 100644
> index 000000000000..f777f88acf0a
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2020 ARM Limited */
> +
> +#ifndef _HELPER_H_
> +#define _HELPER_H_
> +
> +void pac_corruptor(void);
> +
> +#endif
> +
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> new file mode 100644
> index 000000000000..ed445050f621
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include <sys/auxv.h>
> +#include <signal.h>
> +
> +#include "../../kselftest_harness.h"
> +#include "helper.h"
> +
> +/*
> + * Tests are ARMv8.3 compliant. They make no provisions for features present in
> + * future version of the arm architecture
> + */
> +
> +#define ASSERT_PAUTH_ENABLED() \
> +do { \
> + unsigned long hwcaps = getauxval(AT_HWCAP); \
> + /* data key instructions are not in NOP space. This prevents a SIGILL */ \
> + ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
> +} while (0)
> +
> +
> +/* check that a corrupted PAC results in SIGSEGV */
> +TEST_SIGNAL(corrupt_pac, SIGSEGV)
> +{
> + ASSERT_PAUTH_ENABLED();
> +
> + pac_corruptor();
> +}
> +
> +TEST_HARNESS_MAIN
> +
> diff --git a/tools/testing/selftests/arm64/pauth/pac_corruptor.S b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
> new file mode 100644
> index 000000000000..6a34ec23a034
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2020 ARM Limited */
> +
> +.global pac_corruptor
> +
> +.text
> +/*
> + * Corrupting a single bit of the PAC ensures the authentication will fail. It
> + * also guarantees no possible collision. TCR_EL1.TBI0 is set by default so no
> + * top byte PAC is tested
> + */
> + pac_corruptor:
> + paciasp
> +
> + /* make stack frame */
> + sub sp, sp, #16
> + stp x29, lr, [sp]
> + mov x29, sp
> +
> + /* prepare mask for bit to be corrupted (bit 54) */
> + mov x1, xzr
> + add x1, x1, #1
> + lsl x1, x1, #54
> +
> + /* get saved lr, corrupt selected bit, put it back */
> + ldr x0, [sp, #8]
> + eor x0, x0, x1
> + str x0, [sp, #8]
> +
> + /* remove stack frame */
> + ldp x29, lr, [sp]
> + add sp, sp, #16
> +
> + autiasp
> + ret
> +
>

--
Regards,
Vincenzo

2020-08-28 14:30:58

[permalink] [raw]

Subject: Re: [PATCH 2/4] kselftests/arm64: add nop checks for PAuth tests

On 8/28/20 2:16 PM, Boyan Karatotev wrote:
> PAuth adds sign/verify controls to enable and disable groups of
> instructions in hardware for compatibility with libraries that do not
> implement PAuth. The kernel always enables them if it detects PAuth.
>
> Add a test that checks that each group of instructions is enabled, if the
> kernel reports PAuth as detected.
>
> Note: For groups, for the purpose of this patch, we intend instructions
> that use a certain key.
>

Reviewed-by: Vincenzo Frascino <[email protected]>

> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>
> ---
> .../testing/selftests/arm64/pauth/.gitignore | 1 +
> tools/testing/selftests/arm64/pauth/Makefile | 7 ++-
> tools/testing/selftests/arm64/pauth/helper.c | 41 +++++++++++++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++++
> tools/testing/selftests/arm64/pauth/pac.c | 51 +++++++++++++++++++
> 5 files changed, 108 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/arm64/pauth/helper.c
>
> diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
> index b557c916720a..155137d92722 100644
> --- a/tools/testing/selftests/arm64/pauth/.gitignore
> +++ b/tools/testing/selftests/arm64/pauth/.gitignore
> @@ -1 +1,2 @@
> +exec_target
> pac
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index 785c775e5e41..a017d1c8dd58 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -4,7 +4,7 @@
> CFLAGS += -mbranch-protection=pac-ret
>
> TEST_GEN_PROGS := pac
> -TEST_GEN_FILES := pac_corruptor.o
> +TEST_GEN_FILES := pac_corruptor.o helper.o
>
> include ../../lib.mk
>
> @@ -13,10 +13,13 @@ include ../../lib.mk
> $(OUTPUT)/pac_corruptor.o: pac_corruptor.S
> $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
>
> +$(OUTPUT)/helper.o: helper.c
> + $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
> +
> # when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
> # greater, gcc emits pac* instructions which are not in HINT NOP space,
> # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
> # run on earlier targets and print a meaningful error messages
> -$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
> +$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
> $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>
> diff --git a/tools/testing/selftests/arm64/pauth/helper.c b/tools/testing/selftests/arm64/pauth/helper.c
> new file mode 100644
> index 000000000000..8619afb16124
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/helper.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include "helper.h"
> +
> +
> +size_t keyia_sign(size_t ptr)
> +{
> + asm volatile("paciza %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keyib_sign(size_t ptr)
> +{
> + asm volatile("pacizb %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keyda_sign(size_t ptr)
> +{
> + asm volatile("pacdza %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keydb_sign(size_t ptr)
> +{
> + asm volatile("pacdzb %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keyg_sign(size_t ptr)
> +{
> + /* output is encoded in the upper 32 bits */
> + size_t dest = 0;
> + size_t modifier = 0;
> +
> + asm volatile("pacga %0, %1, %2" : "=r" (dest) : "r" (ptr), "r" (modifier));
> +
> + return dest;
> +}
> +
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> index f777f88acf0a..b3cf709e249d 100644
> --- a/tools/testing/selftests/arm64/pauth/helper.h
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -4,7 +4,17 @@
> #ifndef _HELPER_H_
> #define _HELPER_H_
>
> +#include <stdlib.h>
> +
> +
> void pac_corruptor(void);
>
> +/* PAuth sign a value with key ia and modifier value 0 */
> +size_t keyia_sign(size_t val);
> +size_t keyib_sign(size_t val);
> +size_t keyda_sign(size_t val);
> +size_t keydb_sign(size_t val);
> +size_t keyg_sign(size_t val);
> +
> #endif
>
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index ed445050f621..cdbffa8bf61e 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -12,12 +12,25 @@
> * future version of the arm architecture
> */
>
> +#define PAC_COLLISION_ATTEMPTS 10
> +/*
> + * The kernel sets TBID by default. So bits 55 and above should remain
> + * untouched no matter what.
> + * The VA space size is 48 bits. Bigger is opt-in.
> + */
> +#define PAC_MASK (~0xff80ffffffffffff)
> #define ASSERT_PAUTH_ENABLED() \
> do { \
> unsigned long hwcaps = getauxval(AT_HWCAP); \
> /* data key instructions are not in NOP space. This prevents a SIGILL */ \
> ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
> } while (0)
> +#define ASSERT_GENERIC_PAUTH_ENABLED() \
> +do { \
> + unsigned long hwcaps = getauxval(AT_HWCAP); \
> + /* generic key instructions are not in NOP space. This prevents a SIGILL */ \
> + ASSERT_NE(0, hwcaps & HWCAP_PACG) TH_LOG("Generic PAUTH not enabled"); \
> +} while (0)
>
>
> /* check that a corrupted PAC results in SIGSEGV */
> @@ -28,5 +41,43 @@ TEST_SIGNAL(corrupt_pac, SIGSEGV)
> pac_corruptor();
> }
>
> +/*
> + * There are no separate pac* and aut* controls so checking only the pac*
> + * instructions is sufficient
> + */
> +TEST(pac_instructions_not_nop)
> +{
> + size_t keyia = 0;
> + size_t keyib = 0;
> + size_t keyda = 0;
> + size_t keydb = 0;
> +
> + ASSERT_PAUTH_ENABLED();
> +
> + for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
> + keyia |= keyia_sign(i) & PAC_MASK;
> + keyib |= keyib_sign(i) & PAC_MASK;
> + keyda |= keyda_sign(i) & PAC_MASK;
> + keydb |= keydb_sign(i) & PAC_MASK;
> + }
> +
> + ASSERT_NE(0, keyia) TH_LOG("keyia instructions did nothing");
> + ASSERT_NE(0, keyib) TH_LOG("keyib instructions did nothing");
> + ASSERT_NE(0, keyda) TH_LOG("keyda instructions did nothing");
> + ASSERT_NE(0, keydb) TH_LOG("keydb instructions did nothing");
> +}
> +
> +TEST(pac_instructions_not_nop_generic)
> +{
> + size_t keyg = 0;
> +
> + ASSERT_GENERIC_PAUTH_ENABLED();
> +
> + for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++)
> + keyg |= keyg_sign(i) & PAC_MASK;
> +
> + ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
> +}
> +
> TEST_HARNESS_MAIN
>
>

--
Regards,
Vincenzo

2020-08-28 14:33:00

[permalink] [raw]

Subject: Re: [PATCH 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

On 8/28/20 2:16 PM, Boyan Karatotev wrote:
> Kernel documentation states that it will change PAuth keys on exec() calls.
>
> Verify that all keys are correctly switched to new ones.
>

Reviewed-by: Vincenzo Frascino <[email protected]>

> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>
> ---
> tools/testing/selftests/arm64/pauth/Makefile | 4 +
> .../selftests/arm64/pauth/exec_target.c | 35 +++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++
> tools/testing/selftests/arm64/pauth/pac.c | 148 ++++++++++++++++++
> 4 files changed, 197 insertions(+)
> create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
>
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index a017d1c8dd58..2e237b21ccf6 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>
> TEST_GEN_PROGS := pac
> TEST_GEN_FILES := pac_corruptor.o helper.o
> +TEST_GEN_PROGS_EXTENDED := exec_target
>
> include ../../lib.mk
>
> @@ -20,6 +21,9 @@ $(OUTPUT)/helper.o: helper.c
> # greater, gcc emits pac* instructions which are not in HINT NOP space,
> # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
> # run on earlier targets and print a meaningful error messages
> +$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
> + $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
> +
> $(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
> $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>
> diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
> new file mode 100644
> index 000000000000..07addef5a1d7
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/exec_target.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/auxv.h>
> +
> +#include "helper.h"
> +
> +
> +int main(void)
> +{
> + struct signatures signed_vals;
> + unsigned long hwcaps;
> + size_t val;
> +
> + fread(&val, sizeof(size_t), 1, stdin);
> +
> + /* don't try to execute illegal (unimplemented) instructions) caller
> + * should have checked this and keep worker simple
> + */
> + hwcaps = getauxval(AT_HWCAP);
> +
> + if (hwcaps & HWCAP_PACA) {
> + signed_vals.keyia = keyia_sign(val);
> + signed_vals.keyib = keyib_sign(val);
> + signed_vals.keyda = keyda_sign(val);
> + signed_vals.keydb = keydb_sign(val);
> + }
> + signed_vals.keyg = (hwcaps & HWCAP_PACG) ? keyg_sign(val) : 0;
> +
> + fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
> +
> + return 0;
> +}
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> index b3cf709e249d..fceaa1e4824a 100644
> --- a/tools/testing/selftests/arm64/pauth/helper.h
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -6,6 +6,16 @@
>
> #include <stdlib.h>
>
> +#define NKEYS 5
> +
> +
> +struct signatures {
> + size_t keyia;
> + size_t keyib;
> + size_t keyda;
> + size_t keydb;
> + size_t keyg;
> +};
>
> void pac_corruptor(void);
>
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index cdbffa8bf61e..16dea47b11c7 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -2,6 +2,8 @@
> // Copyright (C) 2020 ARM Limited
>
> #include <sys/auxv.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> #include <signal.h>
>
> #include "../../kselftest_harness.h"
> @@ -33,6 +35,117 @@ do { \
> } while (0)
>
>
> +void sign_specific(struct signatures *sign, size_t val)
> +{
> + sign->keyia = keyia_sign(val);
> + sign->keyib = keyib_sign(val);
> + sign->keyda = keyda_sign(val);
> + sign->keydb = keydb_sign(val);
> +}
> +
> +void sign_all(struct signatures *sign, size_t val)
> +{
> + sign->keyia = keyia_sign(val);
> + sign->keyib = keyib_sign(val);
> + sign->keyda = keyda_sign(val);
> + sign->keydb = keydb_sign(val);
> + sign->keyg = keyg_sign(val);
> +}
> +
> +int are_same(struct signatures *old, struct signatures *new, int nkeys)
> +{
> + int res = 0;
> +
> + res |= old->keyia == new->keyia;
> + res |= old->keyib == new->keyib;
> + res |= old->keyda == new->keyda;
> + res |= old->keydb == new->keydb;
> + if (nkeys == NKEYS)
> + res |= old->keyg == new->keyg;
> +
> + return res;
> +}
> +
> +int exec_sign_all(struct signatures *signed_vals, size_t val)
> +{
> + int new_stdin[2];
> + int new_stdout[2];
> + int status;
> + ssize_t ret;
> + pid_t pid;
> +
> + ret = pipe(new_stdin);
> + if (ret == -1) {
> + perror("pipe returned error");
> + return -1;
> + }
> +
> + ret = pipe(new_stdout);
> + if (ret == -1) {
> + perror("pipe returned error");
> + return -1;
> + }
> +
> + pid = fork();
> + // child
> + if (pid == 0) {
> + dup2(new_stdin[0], STDIN_FILENO);
> + if (ret == -1) {
> + perror("dup2 returned error");
> + exit(1);
> + }
> +
> + dup2(new_stdout[1], STDOUT_FILENO);
> + if (ret == -1) {
> + perror("dup2 returned error");
> + exit(1);
> + }
> +
> + close(new_stdin[0]);
> + close(new_stdin[1]);
> + close(new_stdout[0]);
> + close(new_stdout[1]);
> +
> + ret = execl("exec_target", "exec_target", (char *) NULL);
> + if (ret == -1) {
> + perror("exec returned error");
> + exit(1);
> + }
> + }
> +
> + close(new_stdin[0]);
> + close(new_stdout[1]);
> +
> + ret = write(new_stdin[1], &val, sizeof(size_t));
> + if (ret == -1) {
> + perror("write returned error");
> + return -1;
> + }
> +
> + /*
> + * wait for the worker to finish, so that read() reads all data
> + * will also context switch with worker so that this function can be used
> + * for context switch tests
> + */
> + waitpid(pid, &status, 0);
> + if (WIFEXITED(status) == 0) {
> + fprintf(stderr, "worker exited unexpectedly\n");
> + return -1;
> + }
> + if (WEXITSTATUS(status) != 0) {
> + fprintf(stderr, "worker exited with error\n");
> + return -1;
> + }
> +
> + ret = read(new_stdout[0], signed_vals, sizeof(struct signatures));
> + if (ret == -1) {
> + perror("read returned error");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> /* check that a corrupted PAC results in SIGSEGV */
> TEST_SIGNAL(corrupt_pac, SIGSEGV)
> {
> @@ -79,5 +192,40 @@ TEST(pac_instructions_not_nop_generic)
> ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
> }
>
> +/*
> + * fork() does not change keys. Only exec() does so call a worker program.
> + * Its only job is to sign a value and report back the resutls
> + */
> +TEST(exec_unique_keys)
> +{
> + struct signatures new_keys;
> + struct signatures old_keys;
> + int ret;
> + int different = 0;
> + int nkeys = NKEYS;
> + unsigned long hwcaps = getauxval(AT_HWCAP);
> +
> + /* generic and data key instructions are not in NOP space. This prevents a SIGILL */
> + ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
> + if (!(hwcaps & HWCAP_PACG)) {
> + TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
> + nkeys = NKEYS - 1;
> + }
> +
> + for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
> + ret = exec_sign_all(&new_keys, i);
> + ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
> +
> + if (nkeys == NKEYS)
> + sign_all(&old_keys, i);
> + else
> + sign_specific(&old_keys, i);
> +
> + different |= !are_same(&old_keys, &new_keys, nkeys);
> + }
> +
> + ASSERT_EQ(1, different) TH_LOG("exec() did not change keys");
> +}
> +
> TEST_HARNESS_MAIN
>
>

--
Regards,
Vincenzo

2020-08-28 14:37:50

[permalink] [raw]

Subject: Re: [PATCH 4/4] kselftests/arm64: add PAuth tests for single threaded consistency and key uniqueness

On 8/28/20 2:16 PM, Boyan Karatotev wrote:
> PAuth adds 5 different keys that can be used to sign addresses.
>
> Add a test that verifies that the kernel initializes them uniquely and
> preserves them across context switches.
>

Reviewed-by: Vincenzo Frascino <[email protected]>

> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>
> ---
> tools/testing/selftests/arm64/pauth/pac.c | 116 ++++++++++++++++++++++
> 1 file changed, 116 insertions(+)
>
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index 16dea47b11c7..718f49adc275 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -1,10 +1,13 @@
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (C) 2020 ARM Limited
>
> +#define _GNU_SOURCE
> +
> #include <sys/auxv.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <signal.h>
> +#include <sched.h>
>
> #include "../../kselftest_harness.h"
> #include "helper.h"
> @@ -21,6 +24,7 @@
> * The VA space size is 48 bits. Bigger is opt-in.
> */
> #define PAC_MASK (~0xff80ffffffffffff)
> +#define ARBITRARY_VALUE (0x1234)
> #define ASSERT_PAUTH_ENABLED() \
> do { \
> unsigned long hwcaps = getauxval(AT_HWCAP); \
> @@ -66,13 +70,36 @@ int are_same(struct signatures *old, struct signatures *new, int nkeys)
> return res;
> }
>
> +int are_unique(struct signatures *sign, int nkeys)
> +{
> + size_t vals[nkeys];
> +
> + vals[0] = sign->keyia & PAC_MASK;
> + vals[1] = sign->keyib & PAC_MASK;
> + vals[2] = sign->keyda & PAC_MASK;
> + vals[3] = sign->keydb & PAC_MASK;
> +
> + if (nkeys >= 4)
> + vals[4] = sign->keyg & PAC_MASK;
> +
> + for (int i = 0; i < nkeys - 1; i++) {
> + for (int j = i + 1; j < nkeys; j++) {
> + if (vals[i] == vals[j])
> + return 0;
> + }
> + }
> + return 1;
> +}
> +
> int exec_sign_all(struct signatures *signed_vals, size_t val)
> {
> int new_stdin[2];
> int new_stdout[2];
> int status;
> + int i;
> ssize_t ret;
> pid_t pid;
> + cpu_set_t mask;
>
> ret = pipe(new_stdin);
> if (ret == -1) {
> @@ -86,6 +113,20 @@ int exec_sign_all(struct signatures *signed_vals, size_t val)
> return -1;
> }
>
> + /*
> + * pin this process and all its children to a single CPU, so it can also
> + * guarantee a context switch with its child
> + */
> + sched_getaffinity(0, sizeof(mask), &mask);
> +
> + for (i = 0; i < sizeof(cpu_set_t); i++)
> + if (CPU_ISSET(i, &mask))
> + break;
> +
> + CPU_ZERO(&mask);
> + CPU_SET(i, &mask);
> + sched_setaffinity(0, sizeof(mask), &mask);
> +
> pid = fork();
> // child
> if (pid == 0) {
> @@ -192,6 +233,38 @@ TEST(pac_instructions_not_nop_generic)
> ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
> }
>
> +TEST(single_thread_unique_keys)
> +{
> + int unique = 0;
> + int nkeys = NKEYS;
> + struct signatures signed_vals;
> + unsigned long hwcaps = getauxval(AT_HWCAP);
> +
> + /* generic and data key instructions are not in NOP space. This prevents a SIGILL */
> + ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
> + if (!(hwcaps & HWCAP_PACG)) {
> + TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
> + nkeys = NKEYS - 1;
> + }
> +
> + /*
> + * The PAC field is up to 7 bits. Even with unique keys there is about
> + * 5% chance for a collision. This chance rapidly increases the fewer
> + * bits there are, a comparison of the keys directly will be more
> + * reliable All signed values need to be unique at least once out of n
> + * attempts to be certain that the keys are unique
> + */
> + for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
> + if (nkeys == NKEYS)
> + sign_all(&signed_vals, i);
> + else
> + sign_specific(&signed_vals, i);
> + unique |= are_unique(&signed_vals, nkeys);
> + }
> +
> + ASSERT_EQ(1, unique) TH_LOG("keys clashed every time");
> +}
> +
> /*
> * fork() does not change keys. Only exec() does so call a worker program.
> * Its only job is to sign a value and report back the resutls
> @@ -227,5 +300,48 @@ TEST(exec_unique_keys)
> ASSERT_EQ(1, different) TH_LOG("exec() did not change keys");
> }
>
> +TEST(context_switch_keep_keys)
> +{
> + int ret;
> + struct signatures trash;
> + struct signatures before;
> + struct signatures after;
> +
> + ASSERT_PAUTH_ENABLED();
> +
> + sign_specific(&before, ARBITRARY_VALUE);
> +
> + /* will context switch with a process with different keys at least once */
> + ret = exec_sign_all(&trash, ARBITRARY_VALUE);
> + ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
> +
> + sign_specific(&after, ARBITRARY_VALUE);
> +
> + ASSERT_EQ(before.keyia, after.keyia) TH_LOG("keyia changed after context switching");
> + ASSERT_EQ(before.keyib, after.keyib) TH_LOG("keyib changed after context switching");
> + ASSERT_EQ(before.keyda, after.keyda) TH_LOG("keyda changed after context switching");
> + ASSERT_EQ(before.keydb, after.keydb) TH_LOG("keydb changed after context switching");
> +}
> +
> +TEST(context_switch_keep_keys_generic)
> +{
> + int ret;
> + struct signatures trash;
> + size_t before;
> + size_t after;
> +
> + ASSERT_GENERIC_PAUTH_ENABLED();
> +
> + before = keyg_sign(ARBITRARY_VALUE);
> +
> + /* will context switch with a process with different keys at least once */
> + ret = exec_sign_all(&trash, ARBITRARY_VALUE);
> + ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
> +
> + after = keyg_sign(ARBITRARY_VALUE);
> +
> + ASSERT_EQ(before, after) TH_LOG("keyg changed after context switching");
> +}
> +
> TEST_HARNESS_MAIN
>
>

--
Regards,
Vincenzo

2020-08-31 08:06:50

[permalink] [raw]

Subject: Re: [PATCH 1/4] kselftests/arm64: add a basic Pointer Authentication test

Hi Boyan,

On 8/28/20 6:46 PM, Boyan Karatotev wrote:
> PAuth signs and verifies return addresses on the stack. It does so by
> inserting a Pointer Authentication code (PAC) into some of the unused top
> bits of an address. This is achieved by adding paciasp/autiasp instructions
> at the beginning and end of a function.
>
> This feature is partially backwards compatible with earlier versions of the
> ARM architecture. To coerce the compiler into emitting fully backwards
> compatible code the main file is compiled to target an earlier ARM version.
> This allows the tests to check for the feature and print meaningful error
> messages instead of crashing.
>
> Add a test to verify that corrupting the return address results in a
> SIGSEGV on return.
>
> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>
> ---
> tools/testing/selftests/arm64/Makefile | 2 +-
> .../testing/selftests/arm64/pauth/.gitignore | 1 +
> tools/testing/selftests/arm64/pauth/Makefile | 22 ++++++++++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++++++
> tools/testing/selftests/arm64/pauth/pac.c | 32 +++++++++++++++++
> .../selftests/arm64/pauth/pac_corruptor.S | 36 +++++++++++++++++++
> 6 files changed, 102 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/arm64/pauth/.gitignore
> create mode 100644 tools/testing/selftests/arm64/pauth/Makefile
> create mode 100644 tools/testing/selftests/arm64/pauth/helper.h
> create mode 100644 tools/testing/selftests/arm64/pauth/pac.c
> create mode 100644 tools/testing/selftests/arm64/pauth/pac_corruptor.S
>
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> index 93b567d23c8b..525506fd97b9 100644
> --- a/tools/testing/selftests/arm64/Makefile
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -4,7 +4,7 @@
> ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>
> ifneq (,$(filter $(ARCH),aarch64 arm64))
> -ARM64_SUBTARGETS ?= tags signal
> +ARM64_SUBTARGETS ?= tags signal pauth
> else
> ARM64_SUBTARGETS :=
> endif
> diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
> new file mode 100644
> index 000000000000..b557c916720a
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/.gitignore
> @@ -0,0 +1 @@
> +pac
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> new file mode 100644
> index 000000000000..785c775e5e41
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2020 ARM Limited
> +
> +CFLAGS += -mbranch-protection=pac-ret

There is no CFLAGS validation present which may give error with non
supported compiler.

Can you add a check something like,

+#check if the compiler works well
+pauth_cc_support := $(shell if ($(CC) $(CFLAGS) -march=armv8.3-a -E -x
c /dev/null -o /dev/null 2>&1) then echo "1"; fi)
+
+ifeq ($(pauth_cc_support),1)
TEST_GEN_PROGS := pac
TEST_GEN_FILES := pac_corruptor.o
+endif

include ../../lib.mk

+ifeq ($(pauth_cc_support),1)
# pac* and aut* instructions are not available on architectures berfore
# ARMv8.3. Therefore target ARMv8.3 wherever they are used directly
$(OUTPUT)/pac_corruptor.o: pac_corruptor.S
@@ -19,4 +25,4 @@ $(OUTPUT)/pac_corruptor.o: pac_corruptor.S
# run on earlier targets and print a meaningful error messages
$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
-
+endif

> +
> +TEST_GEN_PROGS := pac
> +TEST_GEN_FILES := pac_corruptor.o
> +
> +include ../../lib.mk
> +
> +# pac* and aut* instructions are not available on architectures berfore
> +# ARMv8.3. Therefore target ARMv8.3 wherever they are used directly
> +$(OUTPUT)/pac_corruptor.o: pac_corruptor.S
> + $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
> +
> +# when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
> +# greater, gcc emits pac* instructions which are not in HINT NOP space,
> +# preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
> +# run on earlier targets and print a meaningful error messages
> +$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
> + $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
> +
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> new file mode 100644
> index 000000000000..f777f88acf0a
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2020 ARM Limited */
> +
> +#ifndef _HELPER_H_
> +#define _HELPER_H_
> +
> +void pac_corruptor(void);
> +
> +#endif
> +

Please delete extra line at end of file here and other places too.

Other changes look fine for me. After the suggested changes please free
to add,
Reviewed-by: Amit Daniel Kachhap <[email protected]>

Thanks,
Amit Daniel

> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> new file mode 100644
> index 000000000000..ed445050f621
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include <sys/auxv.h>
> +#include <signal.h>
> +
>

2020-08-31 08:10:44

[permalink] [raw]

Subject: Re: [PATCH 2/4] kselftests/arm64: add nop checks for PAuth tests

On 8/28/20 6:46 PM, Boyan Karatotev wrote:
> PAuth adds sign/verify controls to enable and disable groups of
> instructions in hardware for compatibility with libraries that do not
> implement PAuth. The kernel always enables them if it detects PAuth.
>
> Add a test that checks that each group of instructions is enabled, if the
> kernel reports PAuth as detected.
>
> Note: For groups, for the purpose of this patch, we intend instructions
> that use a certain key.
>
> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>

The changes look fine so please free to add,
Reviewed-by: Amit Daniel Kachhap <[email protected]>

> ---
> .../testing/selftests/arm64/pauth/.gitignore | 1 +
> tools/testing/selftests/arm64/pauth/Makefile | 7 ++-
> tools/testing/selftests/arm64/pauth/helper.c | 41 +++++++++++++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++++
> tools/testing/selftests/arm64/pauth/pac.c | 51 +++++++++++++++++++
> 5 files changed, 108 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/arm64/pauth/helper.c
>
> diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
> index b557c916720a..155137d92722 100644
> --- a/tools/testing/selftests/arm64/pauth/.gitignore
> +++ b/tools/testing/selftests/arm64/pauth/.gitignore
> @@ -1 +1,2 @@
> +exec_target
> pac
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index 785c775e5e41..a017d1c8dd58 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -4,7 +4,7 @@
> CFLAGS += -mbranch-protection=pac-ret
>
> TEST_GEN_PROGS := pac
> -TEST_GEN_FILES := pac_corruptor.o
> +TEST_GEN_FILES := pac_corruptor.o helper.o
>
> include ../../lib.mk
>
> @@ -13,10 +13,13 @@ include ../../lib.mk
> $(OUTPUT)/pac_corruptor.o: pac_corruptor.S
> $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
>
> +$(OUTPUT)/helper.o: helper.c
> + $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
> +
> # when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
> # greater, gcc emits pac* instructions which are not in HINT NOP space,
> # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
> # run on earlier targets and print a meaningful error messages
> -$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
> +$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
> $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>
> diff --git a/tools/testing/selftests/arm64/pauth/helper.c b/tools/testing/selftests/arm64/pauth/helper.c
> new file mode 100644
> index 000000000000..8619afb16124
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/helper.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include "helper.h"
> +
> +
> +size_t keyia_sign(size_t ptr)
> +{
> + asm volatile("paciza %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keyib_sign(size_t ptr)
> +{
> + asm volatile("pacizb %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keyda_sign(size_t ptr)
> +{
> + asm volatile("pacdza %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keydb_sign(size_t ptr)
> +{
> + asm volatile("pacdzb %0" : "+r" (ptr));
> + return ptr;
> +}
> +
> +size_t keyg_sign(size_t ptr)
> +{
> + /* output is encoded in the upper 32 bits */
> + size_t dest = 0;
> + size_t modifier = 0;
> +
> + asm volatile("pacga %0, %1, %2" : "=r" (dest) : "r" (ptr), "r" (modifier));
> +
> + return dest;
> +}
> +
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> index f777f88acf0a..b3cf709e249d 100644
> --- a/tools/testing/selftests/arm64/pauth/helper.h
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -4,7 +4,17 @@
> #ifndef _HELPER_H_
> #define _HELPER_H_
>
> +#include <stdlib.h>
> +
> +
> void pac_corruptor(void);
>
> +/* PAuth sign a value with key ia and modifier value 0 */
> +size_t keyia_sign(size_t val);
> +size_t keyib_sign(size_t val);
> +size_t keyda_sign(size_t val);
> +size_t keydb_sign(size_t val);
> +size_t keyg_sign(size_t val);
> +
> #endif
>
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index ed445050f621..cdbffa8bf61e 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -12,12 +12,25 @@
> * future version of the arm architecture
> */
>
> +#define PAC_COLLISION_ATTEMPTS 10
> +/*
> + * The kernel sets TBID by default. So bits 55 and above should remain
> + * untouched no matter what.
> + * The VA space size is 48 bits. Bigger is opt-in.
> + */
> +#define PAC_MASK (~0xff80ffffffffffff)
> #define ASSERT_PAUTH_ENABLED() \
> do { \
> unsigned long hwcaps = getauxval(AT_HWCAP); \
> /* data key instructions are not in NOP space. This prevents a SIGILL */ \
> ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
> } while (0)
> +#define ASSERT_GENERIC_PAUTH_ENABLED() \
> +do { \
> + unsigned long hwcaps = getauxval(AT_HWCAP); \
> + /* generic key instructions are not in NOP space. This prevents a SIGILL */ \
> + ASSERT_NE(0, hwcaps & HWCAP_PACG) TH_LOG("Generic PAUTH not enabled"); \
> +} while (0)
>
>
> /* check that a corrupted PAC results in SIGSEGV */
> @@ -28,5 +41,43 @@ TEST_SIGNAL(corrupt_pac, SIGSEGV)
> pac_corruptor();
> }
>
> +/*
> + * There are no separate pac* and aut* controls so checking only the pac*
> + * instructions is sufficient
> + */
> +TEST(pac_instructions_not_nop)
> +{
> + size_t keyia = 0;
> + size_t keyib = 0;
> + size_t keyda = 0;
> + size_t keydb = 0;
> +
> + ASSERT_PAUTH_ENABLED();
> +
> + for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
> + keyia |= keyia_sign(i) & PAC_MASK;
> + keyib |= keyib_sign(i) & PAC_MASK;
> + keyda |= keyda_sign(i) & PAC_MASK;
> + keydb |= keydb_sign(i) & PAC_MASK;
> + }
> +
> + ASSERT_NE(0, keyia) TH_LOG("keyia instructions did nothing");
> + ASSERT_NE(0, keyib) TH_LOG("keyib instructions did nothing");
> + ASSERT_NE(0, keyda) TH_LOG("keyda instructions did nothing");
> + ASSERT_NE(0, keydb) TH_LOG("keydb instructions did nothing");
> +}
> +
> +TEST(pac_instructions_not_nop_generic)
> +{
> + size_t keyg = 0;
> +
> + ASSERT_GENERIC_PAUTH_ENABLED();
> +
> + for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++)
> + keyg |= keyg_sign(i) & PAC_MASK;
> +
> + ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
> +}
> +
> TEST_HARNESS_MAIN
>
>

2020-08-31 08:16:03

[permalink] [raw]

Subject: Re: [PATCH 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

On 8/28/20 6:46 PM, Boyan Karatotev wrote:
> Kernel documentation states that it will change PAuth keys on exec() calls.
>
> Verify that all keys are correctly switched to new ones.
>
> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>

The changes look fine so,
Reviewed-by: Amit Daniel Kachhap <[email protected]>

> ---
> tools/testing/selftests/arm64/pauth/Makefile | 4 +
> .../selftests/arm64/pauth/exec_target.c | 35 +++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++
> tools/testing/selftests/arm64/pauth/pac.c | 148 ++++++++++++++++++
> 4 files changed, 197 insertions(+)
> create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
>
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index a017d1c8dd58..2e237b21ccf6 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>

2020-08-31 08:24:19

[permalink] [raw]

Subject: Re: [PATCH 4/4] kselftests/arm64: add PAuth tests for single threaded consistency and key uniqueness

Hi,

On 8/28/20 6:46 PM, Boyan Karatotev wrote:
> PAuth adds 5 different keys that can be used to sign addresses.
>
> Add a test that verifies that the kernel initializes them uniquely and
> preserves them across context switches.
>
> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>

> ---
> tools/testing/selftests/arm64/pauth/pac.c | 116 ++++++++++++++++++++++
> 1 file changed, 116 insertions(+)
>
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index 16dea47b11c7..718f49adc275 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -1,10 +1,13 @@
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (C) 2020 ARM Limited
>
> +#define _GNU_SOURCE
> +
> #include <sys/auxv.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <signal.h>
> +#include <sched.h>
>
> #include "../../kselftest_harness.h"
> #include "helper.h"
> @@ -21,6 +24,7 @@
> * The VA space size is 48 bits. Bigger is opt-in.
> */
> #define PAC_MASK (~0xff80ffffffffffff)
> +#define ARBITRARY_VALUE (0x1234)
> #define ASSERT_PAUTH_ENABLED() \
> do { \
> unsigned long hwcaps = getauxval(AT_HWCAP); \
> @@ -66,13 +70,36 @@ int are_same(struct signatures *old, struct signatures *new, int nkeys)
> return res;
> }
>
> +int are_unique(struct signatures *sign, int nkeys)
> +{
> + size_t vals[nkeys];
> +
> + vals[0] = sign->keyia & PAC_MASK;
> + vals[1] = sign->keyib & PAC_MASK;
> + vals[2] = sign->keyda & PAC_MASK;
> + vals[3] = sign->keydb & PAC_MASK;
> +
> + if (nkeys >= 4)
> + vals[4] = sign->keyg & PAC_MASK;
> +
> + for (int i = 0; i < nkeys - 1; i++) {
> + for (int j = i + 1; j < nkeys; j++) {
> + if (vals[i] == vals[j])
> + return 0;
> + }
> + }
> + return 1;
> +}
> +
> int exec_sign_all(struct signatures *signed_vals, size_t val)
> {
> int new_stdin[2];
> int new_stdout[2];
> int status;
> + int i;
> ssize_t ret;
> pid_t pid;
> + cpu_set_t mask;
>
> ret = pipe(new_stdin);
> if (ret == -1) {
> @@ -86,6 +113,20 @@ int exec_sign_all(struct signatures *signed_vals, size_t val)
> return -1;
> }
>
> + /*
> + * pin this process and all its children to a single CPU, so it can also
> + * guarantee a context switch with its child
> + */
> + sched_getaffinity(0, sizeof(mask), &mask);
> +
> + for (i = 0; i < sizeof(cpu_set_t); i++)
> + if (CPU_ISSET(i, &mask))
> + break;
> +
> + CPU_ZERO(&mask);
> + CPU_SET(i, &mask);
> + sched_setaffinity(0, sizeof(mask), &mask);
> +
> pid = fork();
> // child
> if (pid == 0) {
> @@ -192,6 +233,38 @@ TEST(pac_instructions_not_nop_generic)
> ASSERT_NE(0, keyg) TH_LOG("keyg instructions did nothing");
> }
>
> +TEST(single_thread_unique_keys)
> +{
> + int unique = 0;
> + int nkeys = NKEYS;
> + struct signatures signed_vals;
> + unsigned long hwcaps = getauxval(AT_HWCAP);
> +
> + /* generic and data key instructions are not in NOP space. This prevents a SIGILL */
> + ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
> + if (!(hwcaps & HWCAP_PACG)) {
> + TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
> + nkeys = NKEYS - 1;
> + }
> +
> + /*
> + * The PAC field is up to 7 bits. Even with unique keys there is about
> + * 5% chance for a collision. This chance rapidly increases the fewer
> + * bits there are, a comparison of the keys directly will be more

Can you please reframe the above sentence as it is not clear.
The other changes look fine to me so please free to add,
Reviewed-by: Amit Daniel Kachhap <[email protected]>

//Amit

> + * reliable All signed values need to be unique at least once out of n
> + * attempts to be certain that the keys are unique
> + */
> + for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
> + if (nkeys == NKEYS)
> + sign_all(&signed_vals, i);
> + else
> + sign_specific(&signed_vals, i);
> + unique |= are_unique(&signed_vals, nkeys);
> + }
> +
> + ASSERT_EQ(1, unique) TH_LOG("keys clashed every time");
> +}
> +
> /*
> * fork() does not change keys. Only exec() does so call a worker program.
> * Its only job is to sign a value and report back the resutls
> @@ -227,5 +300,48 @@ TEST(exec_unique_keys)
> ASSERT_EQ(1, different) TH_LOG("exec() did not change keys");
> }
>
> +TEST(context_switch_keep_keys)
> +{
> + int ret;
> + struct signatures trash;
> + struct signatures before;
> + struct signatures after;
> +
> + ASSERT_PAUTH_ENABLED();
> +
> + sign_specific(&before, ARBITRARY_VALUE);
> +
> + /* will context switch with a process with different keys at least once */
> + ret = exec_sign_all(&trash, ARBITRARY_VALUE);
> + ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
> +
> + sign_specific(&after, ARBITRARY_VALUE);
> +
> + ASSERT_EQ(before.keyia, after.keyia) TH_LOG("keyia changed after context switching");
> + ASSERT_EQ(before.keyib, after.keyib) TH_LOG("keyib changed after context switching");
> + ASSERT_EQ(before.keyda, after.keyda) TH_LOG("keyda changed after context switching");
> + ASSERT_EQ(before.keydb, after.keydb) TH_LOG("keydb changed after context switching");
> +}
> +
> +TEST(context_switch_keep_keys_generic)
> +{
> + int ret;
> + struct signatures trash;
> + size_t before;
> + size_t after;
> +
> + ASSERT_GENERIC_PAUTH_ENABLED();
> +
> + before = keyg_sign(ARBITRARY_VALUE);
> +
> + /* will context switch with a process with different keys at least once */
> + ret = exec_sign_all(&trash, ARBITRARY_VALUE);
> + ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
> +
> + after = keyg_sign(ARBITRARY_VALUE);
> +
> + ASSERT_EQ(before, after) TH_LOG("keyg changed after context switching");
> +}
> +
> TEST_HARNESS_MAIN
>
>

2020-09-02 16:50:55

[permalink] [raw]

Subject: Re: [PATCH 0/4] kselftests/arm64: add PAuth tests

On Fri, Aug 28, 2020 at 02:16:02PM +0100, Boyan Karatotev wrote:
> Pointer Authentication (PAuth) is a security feature introduced in ARMv8.3.
> It introduces instructions to sign addresses and later check for potential
> corruption using a second modifier value and one of a set of keys. The
> signature, in the form of the Pointer Authentication Code (PAC), is stored
> in some of the top unused bits of the virtual address (e.g. [54: 49] if
> TBID0 is enabled and TnSZ is set to use a 48 bit VA space). A set of
> controls are present to enable/disable groups of instructions (which use
> certain keys) for compatibility with libraries that do not utilize the
> feature. PAuth is used to verify the integrity of return addresses on the
> stack with less memory than the stack canary.
>
> This patchset adds kselftests to verify the kernel's configuration of the
> feature and its runtime behaviour. There are 7 tests which verify that:
> * an authentication failure leads to a SIGSEGV
> * the data/instruction instruction groups are enabled
> * the generic instructions are enabled
> * all 5 keys are unique for a single thread
> * exec() changes all keys to new unique ones
> * context switching preserves the 4 data/instruction keys
> * context switching preserves the generic keys
>
> The tests have been verified to work on qemu without a working PAUTH
> Implementation and on ARM's FVP with a full or partial PAuth
> implementation.
>
> Note: This patchset is only verified for ARMv8.3 and there will be some
> changes required for ARMv8.6. More details can be found here [1]. Once
> ARMv8.6 PAuth is merged the first test in this series will required to be
> updated.

Nit: is it worth running checkpatch over this series?

Although this is not kernel code, there are a number of formatting
weirdnesses and surplus blank lines etc. that checkpatch would probably
warn about.

[...]

Cheers
---Dave

2020-09-02 16:52:10

[permalink] [raw]

Subject: Re: [PATCH 1/4] kselftests/arm64: add a basic Pointer Authentication test

On Fri, Aug 28, 2020 at 02:16:03PM +0100, Boyan Karatotev wrote:
> PAuth signs and verifies return addresses on the stack. It does so by
> inserting a Pointer Authentication code (PAC) into some of the unused top
> bits of an address. This is achieved by adding paciasp/autiasp instructions
> at the beginning and end of a function.
>
> This feature is partially backwards compatible with earlier versions of the
> ARM architecture. To coerce the compiler into emitting fully backwards
> compatible code the main file is compiled to target an earlier ARM version.
> This allows the tests to check for the feature and print meaningful error
> messages instead of crashing.
>
> Add a test to verify that corrupting the return address results in a
> SIGSEGV on return.
>
> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>
> ---
> tools/testing/selftests/arm64/Makefile | 2 +-
> .../testing/selftests/arm64/pauth/.gitignore | 1 +
> tools/testing/selftests/arm64/pauth/Makefile | 22 ++++++++++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++++++
> tools/testing/selftests/arm64/pauth/pac.c | 32 +++++++++++++++++
> .../selftests/arm64/pauth/pac_corruptor.S | 36 +++++++++++++++++++
> 6 files changed, 102 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/arm64/pauth/.gitignore
> create mode 100644 tools/testing/selftests/arm64/pauth/Makefile
> create mode 100644 tools/testing/selftests/arm64/pauth/helper.h
> create mode 100644 tools/testing/selftests/arm64/pauth/pac.c
> create mode 100644 tools/testing/selftests/arm64/pauth/pac_corruptor.S
>
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> index 93b567d23c8b..525506fd97b9 100644
> --- a/tools/testing/selftests/arm64/Makefile
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -4,7 +4,7 @@
> ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>
> ifneq (,$(filter $(ARCH),aarch64 arm64))
> -ARM64_SUBTARGETS ?= tags signal
> +ARM64_SUBTARGETS ?= tags signal pauth
> else
> ARM64_SUBTARGETS :=
> endif
> diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
> new file mode 100644
> index 000000000000..b557c916720a
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/.gitignore
> @@ -0,0 +1 @@
> +pac
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> new file mode 100644
> index 000000000000..785c775e5e41
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2020 ARM Limited
> +
> +CFLAGS += -mbranch-protection=pac-ret
> +
> +TEST_GEN_PROGS := pac
> +TEST_GEN_FILES := pac_corruptor.o
> +
> +include ../../lib.mk
> +
> +# pac* and aut* instructions are not available on architectures berfore
> +# ARMv8.3. Therefore target ARMv8.3 wherever they are used directly
> +$(OUTPUT)/pac_corruptor.o: pac_corruptor.S
> + $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
> +
> +# when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
> +# greater, gcc emits pac* instructions which are not in HINT NOP space,
> +# preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
> +# run on earlier targets and print a meaningful error messages
> +$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
> + $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
> +
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> new file mode 100644
> index 000000000000..f777f88acf0a
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2020 ARM Limited */
> +
> +#ifndef _HELPER_H_
> +#define _HELPER_H_
> +
> +void pac_corruptor(void);
> +
> +#endif
> +
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> new file mode 100644
> index 000000000000..ed445050f621
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include <sys/auxv.h>
> +#include <signal.h>
> +
> +#include "../../kselftest_harness.h"
> +#include "helper.h"
> +
> +/*
> + * Tests are ARMv8.3 compliant. They make no provisions for features present in
> + * future version of the arm architecture
> + */
> +
> +#define ASSERT_PAUTH_ENABLED() \
> +do { \
> + unsigned long hwcaps = getauxval(AT_HWCAP); \
> + /* data key instructions are not in NOP space. This prevents a SIGILL */ \

> + ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
> +} while (0)
> +
> +
> +/* check that a corrupted PAC results in SIGSEGV */
> +TEST_SIGNAL(corrupt_pac, SIGSEGV)
> +{
> + ASSERT_PAUTH_ENABLED();
> +
> + pac_corruptor();
> +}
> +
> +TEST_HARNESS_MAIN
> +
> diff --git a/tools/testing/selftests/arm64/pauth/pac_corruptor.S b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
> new file mode 100644
> index 000000000000..6a34ec23a034
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2020 ARM Limited */
> +
> +.global pac_corruptor
> +
> +.text
> +/*
> + * Corrupting a single bit of the PAC ensures the authentication will fail. It
> + * also guarantees no possible collision. TCR_EL1.TBI0 is set by default so no
> + * top byte PAC is tested
> + */
> + pac_corruptor:
> + paciasp
> +
> + /* make stack frame */
> + sub sp, sp, #16
> + stp x29, lr, [sp]

Nit: if respinning, you can optimise a few sequences of this sort, e.g.

stp x29, lr, [sp, #-16]!

> + mov x29, sp
> +
> + /* prepare mask for bit to be corrupted (bit 54) */
> + mov x1, xzr
> + add x1, x1, #1
> + lsl x1, x1, #54

Nit:

mov x1, #1 << 54

but anyway, the logic operations can encode most simple bitmasks
directly as immediate operands, so you can skip this and just do

> +
> + /* get saved lr, corrupt selected bit, put it back */
> + ldr x0, [sp, #8]
> + eor x0, x0, x1

eor x0, x0, #1 << 54

> + str x0, [sp, #8]
> +
> + /* remove stack frame */
> + ldp x29, lr, [sp]
> + add sp, sp, #16

ldp x29, lr, [sp], #16

[...]

Actually, since there are no leaf nested function calls and no trap is
expected until the function returns (so backtracing in the middle of
this function is unlikely to be needed), could we optimise this whole
thing down to the following?

pac_corruptor:
paciasp
eor lr, lr, #1 << 53
autiasp
ret

Cheers
---Dave

2020-09-02 17:02:58

[permalink] [raw]

Subject: Re: [PATCH 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
> Kernel documentation states that it will change PAuth keys on exec() calls.
>
> Verify that all keys are correctly switched to new ones.
>
> Cc: Shuah Khan <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Signed-off-by: Boyan Karatotev <[email protected]>
> ---
> tools/testing/selftests/arm64/pauth/Makefile | 4 +
> .../selftests/arm64/pauth/exec_target.c | 35 +++++
> tools/testing/selftests/arm64/pauth/helper.h | 10 ++
> tools/testing/selftests/arm64/pauth/pac.c | 148 ++++++++++++++++++
> 4 files changed, 197 insertions(+)
> create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
>
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index a017d1c8dd58..2e237b21ccf6 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>
> TEST_GEN_PROGS := pac
> TEST_GEN_FILES := pac_corruptor.o helper.o
> +TEST_GEN_PROGS_EXTENDED := exec_target
>
> include ../../lib.mk
>
> @@ -20,6 +21,9 @@ $(OUTPUT)/helper.o: helper.c
> # greater, gcc emits pac* instructions which are not in HINT NOP space,
> # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
> # run on earlier targets and print a meaningful error messages
> +$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
> + $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
> +
> $(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
> $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>
> diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
> new file mode 100644
> index 000000000000..07addef5a1d7
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/exec_target.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/auxv.h>
> +
> +#include "helper.h"
> +
> +
> +int main(void)
> +{
> + struct signatures signed_vals;
> + unsigned long hwcaps;
> + size_t val;
> +
> + fread(&val, sizeof(size_t), 1, stdin);
> +
> + /* don't try to execute illegal (unimplemented) instructions) caller
> + * should have checked this and keep worker simple
> + */
> + hwcaps = getauxval(AT_HWCAP);
> +
> + if (hwcaps & HWCAP_PACA) {
> + signed_vals.keyia = keyia_sign(val);
> + signed_vals.keyib = keyib_sign(val);
> + signed_vals.keyda = keyda_sign(val);
> + signed_vals.keydb = keydb_sign(val);
> + }
> + signed_vals.keyg = (hwcaps & HWCAP_PACG) ? keyg_sign(val) : 0;
> +
> + fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
> +
> + return 0;
> +}
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> index b3cf709e249d..fceaa1e4824a 100644
> --- a/tools/testing/selftests/arm64/pauth/helper.h
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -6,6 +6,16 @@
>
> #include <stdlib.h>
>
> +#define NKEYS 5
> +
> +
> +struct signatures {
> + size_t keyia;
> + size_t keyib;
> + size_t keyda;
> + size_t keydb;
> + size_t keyg;
> +};
>
> void pac_corruptor(void);
>
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index cdbffa8bf61e..16dea47b11c7 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -2,6 +2,8 @@
> // Copyright (C) 2020 ARM Limited
>
> #include <sys/auxv.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> #include <signal.h>
>
> #include "../../kselftest_harness.h"
> @@ -33,6 +35,117 @@ do { \
> } while (0)
>
>
> +void sign_specific(struct signatures *sign, size_t val)
> +{
> + sign->keyia = keyia_sign(val);
> + sign->keyib = keyib_sign(val);
> + sign->keyda = keyda_sign(val);
> + sign->keydb = keydb_sign(val);
> +}
> +
> +void sign_all(struct signatures *sign, size_t val)
> +{
> + sign->keyia = keyia_sign(val);
> + sign->keyib = keyib_sign(val);
> + sign->keyda = keyda_sign(val);
> + sign->keydb = keydb_sign(val);
> + sign->keyg = keyg_sign(val);
> +}
> +
> +int are_same(struct signatures *old, struct signatures *new, int nkeys)
> +{
> + int res = 0;
> +
> + res |= old->keyia == new->keyia;
> + res |= old->keyib == new->keyib;
> + res |= old->keyda == new->keyda;
> + res |= old->keydb == new->keydb;
> + if (nkeys == NKEYS)
> + res |= old->keyg == new->keyg;
> +
> + return res;
> +}
> +
> +int exec_sign_all(struct signatures *signed_vals, size_t val)
> +{

Could popen(3) be used here?

Fork-and-exec is notoriously fiddly, so it's preferable to use a library
function to do it where applicable.

[...]

Cheers
---Dave

2020-09-03 09:47:52

[permalink] [raw]

Subject: Re: [PATCH 0/4] kselftests/arm64: add PAuth tests

On 02/09/2020 17:48, Dave Martin wrote:
> On Fri, Aug 28, 2020 at 02:16:02PM +0100, Boyan Karatotev wrote:
>> Pointer Authentication (PAuth) is a security feature introduced in ARMv8.3.
>> It introduces instructions to sign addresses and later check for potential
>> corruption using a second modifier value and one of a set of keys. The
>> signature, in the form of the Pointer Authentication Code (PAC), is stored
>> in some of the top unused bits of the virtual address (e.g. [54: 49] if
>> TBID0 is enabled and TnSZ is set to use a 48 bit VA space). A set of
>> controls are present to enable/disable groups of instructions (which use
>> certain keys) for compatibility with libraries that do not utilize the
>> feature. PAuth is used to verify the integrity of return addresses on the
>> stack with less memory than the stack canary.
>>
>> This patchset adds kselftests to verify the kernel's configuration of the
>> feature and its runtime behaviour. There are 7 tests which verify that:
>> * an authentication failure leads to a SIGSEGV
>> * the data/instruction instruction groups are enabled
>> * the generic instructions are enabled
>> * all 5 keys are unique for a single thread
>> * exec() changes all keys to new unique ones
>> * context switching preserves the 4 data/instruction keys
>> * context switching preserves the generic keys
>>
>> The tests have been verified to work on qemu without a working PAUTH
>> Implementation and on ARM's FVP with a full or partial PAuth
>> implementation.
>>
>> Note: This patchset is only verified for ARMv8.3 and there will be some
>> changes required for ARMv8.6. More details can be found here [1]. Once
>> ARMv8.6 PAuth is merged the first test in this series will required to be
>> updated.
>
> Nit: is it worth running checkpatch over this series?
>
> Although this is not kernel code, there are a number of formatting
> weirdnesses and surplus blank lines etc. that checkpatch would probably
> warn about.
>
I ran it through checkpatch and it came out clean except for some
MAINTAINERS warnings. I see that when I add --strict it does complain
about multiple blank lines which I can fix for the next version. Are
there any other flags I should be running checkpatch with?
> [...]
>
> Cheers
> ---Dave
>

--
Regards,
Boyan

2020-09-03 10:13:16

[permalink] [raw]

Subject: Re: [PATCH 1/4] kselftests/arm64: add a basic Pointer Authentication test

On 02/09/2020 17:49, Dave Martin wrote:
> On Fri, Aug 28, 2020 at 02:16:03PM +0100, Boyan Karatotev wrote:
>> PAuth signs and verifies return addresses on the stack. It does so by
>> inserting a Pointer Authentication code (PAC) into some of the unused top
>> bits of an address. This is achieved by adding paciasp/autiasp instructions
>> at the beginning and end of a function.
>>
>> This feature is partially backwards compatible with earlier versions of the
>> ARM architecture. To coerce the compiler into emitting fully backwards
>> compatible code the main file is compiled to target an earlier ARM version.
>> This allows the tests to check for the feature and print meaningful error
>> messages instead of crashing.
>>
>> Add a test to verify that corrupting the return address results in a
>> SIGSEGV on return.
>>
>> Cc: Shuah Khan <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Signed-off-by: Boyan Karatotev <[email protected]>
>> ---
>> tools/testing/selftests/arm64/Makefile | 2 +-
>> .../testing/selftests/arm64/pauth/.gitignore | 1 +
>> tools/testing/selftests/arm64/pauth/Makefile | 22 ++++++++++++
>> tools/testing/selftests/arm64/pauth/helper.h | 10 ++++++
>> tools/testing/selftests/arm64/pauth/pac.c | 32 +++++++++++++++++
>> .../selftests/arm64/pauth/pac_corruptor.S | 36 +++++++++++++++++++
>> 6 files changed, 102 insertions(+), 1 deletion(-)
>> create mode 100644 tools/testing/selftests/arm64/pauth/.gitignore
>> create mode 100644 tools/testing/selftests/arm64/pauth/Makefile
>> create mode 100644 tools/testing/selftests/arm64/pauth/helper.h
>> create mode 100644 tools/testing/selftests/arm64/pauth/pac.c
>> create mode 100644 tools/testing/selftests/arm64/pauth/pac_corruptor.S
>>
>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>> index 93b567d23c8b..525506fd97b9 100644
>> --- a/tools/testing/selftests/arm64/Makefile
>> +++ b/tools/testing/selftests/arm64/Makefile
>> @@ -4,7 +4,7 @@
>> ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>
>> ifneq (,$(filter $(ARCH),aarch64 arm64))
>> -ARM64_SUBTARGETS ?= tags signal
>> +ARM64_SUBTARGETS ?= tags signal pauth
>> else
>> ARM64_SUBTARGETS :=
>> endif
>> diff --git a/tools/testing/selftests/arm64/pauth/.gitignore b/tools/testing/selftests/arm64/pauth/.gitignore
>> new file mode 100644
>> index 000000000000..b557c916720a
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/pauth/.gitignore
>> @@ -0,0 +1 @@
>> +pac
>> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
>> new file mode 100644
>> index 000000000000..785c775e5e41
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/pauth/Makefile
>> @@ -0,0 +1,22 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2020 ARM Limited
>> +
>> +CFLAGS += -mbranch-protection=pac-ret
>> +
>> +TEST_GEN_PROGS := pac
>> +TEST_GEN_FILES := pac_corruptor.o
>> +
>> +include ../../lib.mk
>> +
>> +# pac* and aut* instructions are not available on architectures berfore
>> +# ARMv8.3. Therefore target ARMv8.3 wherever they are used directly
>> +$(OUTPUT)/pac_corruptor.o: pac_corruptor.S
>> + $(CC) -c $^ -o $@ $(CFLAGS) -march=armv8.3-a
>> +
>> +# when -mbranch-protection is enabled and the target architecture is ARMv8.3 or
>> +# greater, gcc emits pac* instructions which are not in HINT NOP space,
>> +# preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
>> +# run on earlier targets and print a meaningful error messages
>> +$(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o
>> + $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>> +
>> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
>> new file mode 100644
>> index 000000000000..f777f88acf0a
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/pauth/helper.h
>> @@ -0,0 +1,10 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2020 ARM Limited */
>> +
>> +#ifndef _HELPER_H_
>> +#define _HELPER_H_
>> +
>> +void pac_corruptor(void);
>> +
>> +#endif
>> +
>> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
>> new file mode 100644
>> index 000000000000..ed445050f621
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/pauth/pac.c
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2020 ARM Limited
>> +
>> +#include <sys/auxv.h>
>> +#include <signal.h>
>> +
>> +#include "../../kselftest_harness.h"
>> +#include "helper.h"
>> +
>> +/*
>> + * Tests are ARMv8.3 compliant. They make no provisions for features present in
>> + * future version of the arm architecture
>> + */
>> +
>> +#define ASSERT_PAUTH_ENABLED() \
>> +do { \
>> + unsigned long hwcaps = getauxval(AT_HWCAP); \
>> + /* data key instructions are not in NOP space. This prevents a SIGILL */ \
>
>
>> + ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled"); \
>> +} while (0)
>> +
>> +
>> +/* check that a corrupted PAC results in SIGSEGV */
>> +TEST_SIGNAL(corrupt_pac, SIGSEGV)
>> +{
>> + ASSERT_PAUTH_ENABLED();
>> +
>> + pac_corruptor();
>> +}
>> +
>> +TEST_HARNESS_MAIN
>> +
>> diff --git a/tools/testing/selftests/arm64/pauth/pac_corruptor.S b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
>> new file mode 100644
>> index 000000000000..6a34ec23a034
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2020 ARM Limited */
>> +
>> +.global pac_corruptor
>> +
>> +.text
>> +/*
>> + * Corrupting a single bit of the PAC ensures the authentication will fail. It
>> + * also guarantees no possible collision. TCR_EL1.TBI0 is set by default so no
>> + * top byte PAC is tested
>> + */
>> + pac_corruptor:
>> + paciasp
>> +
>> + /* make stack frame */
>> + sub sp, sp, #16
>> + stp x29, lr, [sp]
>
> Nit: if respinning, you can optimise a few sequences of this sort, e.g.
>
> stp x29, lr, [sp, #-16]!
>
>> + mov x29, sp
>> +
>> + /* prepare mask for bit to be corrupted (bit 54) */
>> + mov x1, xzr
>> + add x1, x1, #1
>> + lsl x1, x1, #54
>
> Nit:
>
> mov x1, #1 << 54
Thank you for this, didn't know I could do it this way.
>
> but anyway, the logic operations can encode most simple bitmasks
> directly as immediate operands, so you can skip this and just do
>
>> +
>> + /* get saved lr, corrupt selected bit, put it back */
>> + ldr x0, [sp, #8]
>> + eor x0, x0, x1
>
> eor x0, x0, #1 << 54
>
>> + str x0, [sp, #8]
>> +
>> + /* remove stack frame */
>> + ldp x29, lr, [sp]
>> + add sp, sp, #16
>
> ldp x29, lr, [sp], #16
>
> [...]
>
> Actually, since there are no leaf nested function calls and no trap is
> expected until the function returns (so backtracing in the middle of
> this function is unlikely to be needed), could we optimise this whole
> thing down to the following?
>
I suppose you're right. The intent was to emulate a c function but there
really is no point in doing all this extra work. Will change it.
> pac_corruptor:
> paciasp
> eor lr, lr, #1 << 53
> autiasp
> ret
>
> Cheers
> ---Dave
>

--
Regards,
Boyan

2020-09-03 10:22:23

[permalink] [raw]

Subject: Re: [PATCH 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

On 02/09/2020 18:00, Dave Martin wrote:
> On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
>> Kernel documentation states that it will change PAuth keys on exec() calls.
>>
>> Verify that all keys are correctly switched to new ones.
>>
>> Cc: Shuah Khan <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Signed-off-by: Boyan Karatotev <[email protected]>
>> ---
>> tools/testing/selftests/arm64/pauth/Makefile | 4 +
>> .../selftests/arm64/pauth/exec_target.c | 35 +++++
>> tools/testing/selftests/arm64/pauth/helper.h | 10 ++
>> tools/testing/selftests/arm64/pauth/pac.c | 148 ++++++++++++++++++
>> 4 files changed, 197 insertions(+)
>> create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
>>
>> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
>> index a017d1c8dd58..2e237b21ccf6 100644
>> --- a/tools/testing/selftests/arm64/pauth/Makefile
>> +++ b/tools/testing/selftests/arm64/pauth/Makefile
>> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>>
>> TEST_GEN_PROGS := pac
>> TEST_GEN_FILES := pac_corruptor.o helper.o
>> +TEST_GEN_PROGS_EXTENDED := exec_target
>>
>> include ../../lib.mk
>>
>> @@ -20,6 +21,9 @@ $(OUTPUT)/helper.o: helper.c
>> # greater, gcc emits pac* instructions which are not in HINT NOP space,
>> # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
>> # run on earlier targets and print a meaningful error messages
>> +$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
>> + $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>> +
>> $(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
>> $(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>>
>> diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
>> new file mode 100644
>> index 000000000000..07addef5a1d7
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/pauth/exec_target.c
>> @@ -0,0 +1,35 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2020 ARM Limited
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <sys/auxv.h>
>> +
>> +#include "helper.h"
>> +
>> +
>> +int main(void)
>> +{
>> + struct signatures signed_vals;
>> + unsigned long hwcaps;
>> + size_t val;
>> +
>> + fread(&val, sizeof(size_t), 1, stdin);
>> +
>> + /* don't try to execute illegal (unimplemented) instructions) caller
>> + * should have checked this and keep worker simple
>> + */
>> + hwcaps = getauxval(AT_HWCAP);
>> +
>> + if (hwcaps & HWCAP_PACA) {
>> + signed_vals.keyia = keyia_sign(val);
>> + signed_vals.keyib = keyib_sign(val);
>> + signed_vals.keyda = keyda_sign(val);
>> + signed_vals.keydb = keydb_sign(val);
>> + }
>> + signed_vals.keyg = (hwcaps & HWCAP_PACG) ? keyg_sign(val) : 0;
>> +
>> + fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
>> +
>> + return 0;
>> +}
>> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
>> index b3cf709e249d..fceaa1e4824a 100644
>> --- a/tools/testing/selftests/arm64/pauth/helper.h
>> +++ b/tools/testing/selftests/arm64/pauth/helper.h
>> @@ -6,6 +6,16 @@
>>
>> #include <stdlib.h>
>>
>> +#define NKEYS 5
>> +
>> +
>> +struct signatures {
>> + size_t keyia;
>> + size_t keyib;
>> + size_t keyda;
>> + size_t keydb;
>> + size_t keyg;
>> +};
>>
>> void pac_corruptor(void);
>>
>> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
>> index cdbffa8bf61e..16dea47b11c7 100644
>> --- a/tools/testing/selftests/arm64/pauth/pac.c
>> +++ b/tools/testing/selftests/arm64/pauth/pac.c
>> @@ -2,6 +2,8 @@
>> // Copyright (C) 2020 ARM Limited
>>
>> #include <sys/auxv.h>
>> +#include <sys/types.h>
>> +#include <sys/wait.h>
>> #include <signal.h>
>>
>> #include "../../kselftest_harness.h"
>> @@ -33,6 +35,117 @@ do { \
>> } while (0)
>>
>>
>> +void sign_specific(struct signatures *sign, size_t val)
>> +{
>> + sign->keyia = keyia_sign(val);
>> + sign->keyib = keyib_sign(val);
>> + sign->keyda = keyda_sign(val);
>> + sign->keydb = keydb_sign(val);
>> +}
>> +
>> +void sign_all(struct signatures *sign, size_t val)
>> +{
>> + sign->keyia = keyia_sign(val);
>> + sign->keyib = keyib_sign(val);
>> + sign->keyda = keyda_sign(val);
>> + sign->keydb = keydb_sign(val);
>> + sign->keyg = keyg_sign(val);
>> +}
>> +
>> +int are_same(struct signatures *old, struct signatures *new, int nkeys)
>> +{
>> + int res = 0;
>> +
>> + res |= old->keyia == new->keyia;
>> + res |= old->keyib == new->keyib;
>> + res |= old->keyda == new->keyda;
>> + res |= old->keydb == new->keydb;
>> + if (nkeys == NKEYS)
>> + res |= old->keyg == new->keyg;
>> +
>> + return res;
>> +}
>> +
>> +int exec_sign_all(struct signatures *signed_vals, size_t val)
>> +{
>
> Could popen(3) be used here?
>
> Fork-and-exec is notoriously fiddly, so it's preferable to use a library
> function to do it where applicable.I would love to, but the worker needs a bidirectional channel and popen
only gives a unidirectional stream.
>
> [...]
>
> Cheers
> ---Dave
>

--
Regards,
Boyan

2020-09-07 10:27:06

[permalink] [raw]

Subject: Re: [PATCH 1/4] kselftests/arm64: add a basic Pointer Authentication test

On Thu, Sep 03, 2020 at 11:12:02AM +0100, Boyan Karatotev wrote:
> On 02/09/2020 17:49, Dave Martin wrote:
> > On Fri, Aug 28, 2020 at 02:16:03PM +0100, Boyan Karatotev wrote:
> >> PAuth signs and verifies return addresses on the stack. It does so by
> >> inserting a Pointer Authentication code (PAC) into some of the unused top
> >> bits of an address. This is achieved by adding paciasp/autiasp instructions
> >> at the beginning and end of a function.
> >>
> >> This feature is partially backwards compatible with earlier versions of the
> >> ARM architecture. To coerce the compiler into emitting fully backwards
> >> compatible code the main file is compiled to target an earlier ARM version.
> >> This allows the tests to check for the feature and print meaningful error
> >> messages instead of crashing.
> >>
> >> Add a test to verify that corrupting the return address results in a
> >> SIGSEGV on return.
> >>
> >> Cc: Shuah Khan <[email protected]>
> >> Cc: Catalin Marinas <[email protected]>
> >> Cc: Will Deacon <[email protected]>
> >> Signed-off-by: Boyan Karatotev <[email protected]>
> >> ---

[...]

> >> diff --git a/tools/testing/selftests/arm64/pauth/pac_corruptor.S b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
> >> new file mode 100644
> >> index 000000000000..6a34ec23a034
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
> >> @@ -0,0 +1,36 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* Copyright (C) 2020 ARM Limited */
> >> +
> >> +.global pac_corruptor
> >> +
> >> +.text
> >> +/*
> >> + * Corrupting a single bit of the PAC ensures the authentication will fail. It
> >> + * also guarantees no possible collision. TCR_EL1.TBI0 is set by default so no
> >> + * top byte PAC is tested
> >> + */
> >> + pac_corruptor:
> >> + paciasp
> >> +
> >> + /* make stack frame */
> >> + sub sp, sp, #16
> >> + stp x29, lr, [sp]
> >
> > Nit: if respinning, you can optimise a few sequences of this sort, e.g.
> >
> > stp x29, lr, [sp, #-16]!
> >
> >> + mov x29, sp
> >> +
> >> + /* prepare mask for bit to be corrupted (bit 54) */
> >> + mov x1, xzr
> >> + add x1, x1, #1
> >> + lsl x1, x1, #54
> >
> > Nit:
> >
> > mov x1, #1 << 54
> Thank you for this, didn't know I could do it this way.
> >
> > but anyway, the logic operations can encode most simple bitmasks
> > directly as immediate operands, so you can skip this and just do
> >
> >> +
> >> + /* get saved lr, corrupt selected bit, put it back */
> >> + ldr x0, [sp, #8]
> >> + eor x0, x0, x1
> >
> > eor x0, x0, #1 << 54
> >
> >> + str x0, [sp, #8]
> >> +
> >> + /* remove stack frame */
> >> + ldp x29, lr, [sp]
> >> + add sp, sp, #16
> >
> > ldp x29, lr, [sp], #16
> >
> > [...]
> >
> > Actually, since there are no leaf nested function calls and no trap is
> > expected until the function returns (so backtracing in the middle of
> > this function is unlikely to be needed), could we optimise this whole
> > thing down to the following?
> >
> I suppose you're right. The intent was to emulate a c function but there
> really is no point in doing all this extra work. Will change it.

It's not critical either way, but this way it's at least less code to
maintain / read.

> > pac_corruptor:
> > paciasp
> > eor lr, lr, #1 << 53
> > autiasp
> > ret
> >
> > Cheers
> > ---Dave

[...]

Cheers
---Dave

2020-09-07 10:28:37

[permalink] [raw]

Subject: Re: [PATCH 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

On Thu, Sep 03, 2020 at 11:20:25AM +0100, Boyan Karatotev wrote:
> On 02/09/2020 18:00, Dave Martin wrote:
> > On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
> >> Kernel documentation states that it will change PAuth keys on exec() calls.
> >>
> >> Verify that all keys are correctly switched to new ones.
> >>
> >> Cc: Shuah Khan <[email protected]>
> >> Cc: Catalin Marinas <[email protected]>
> >> Cc: Will Deacon <[email protected]>
> >> Signed-off-by: Boyan Karatotev <[email protected]>
> >> ---

[...]

> >> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> >> index cdbffa8bf61e..16dea47b11c7 100644
> >> --- a/tools/testing/selftests/arm64/pauth/pac.c
> >> +++ b/tools/testing/selftests/arm64/pauth/pac.c

[...]

> >> +int exec_sign_all(struct signatures *signed_vals, size_t val)
> >> +{
> >
> > Could popen(3) be used here?
> >
> > Fork-and-exec is notoriously fiddly, so it's preferable to use a library
> > function to do it where applicable.I would love to, but the worker needs a bidirectional channel and popen
> only gives a unidirectional stream.

Ah, fair point.

Would it help if you created an additional pipe before calling popen()?

May not be worth it, though. For one thing, wiring that extra pipe to
stdin or stdout in the child process would require some extra work...

Cheers
---Dave

2020-09-07 10:32:26

[permalink] [raw]

Subject: Re: [PATCH 0/4] kselftests/arm64: add PAuth tests

On Thu, Sep 03, 2020 at 10:46:33AM +0100, Boyan Karatotev wrote:
> On 02/09/2020 17:48, Dave Martin wrote:
> > On Fri, Aug 28, 2020 at 02:16:02PM +0100, Boyan Karatotev wrote:
> >> Pointer Authentication (PAuth) is a security feature introduced in ARMv8.3.
> >> It introduces instructions to sign addresses and later check for potential
> >> corruption using a second modifier value and one of a set of keys. The
> >> signature, in the form of the Pointer Authentication Code (PAC), is stored
> >> in some of the top unused bits of the virtual address (e.g. [54: 49] if
> >> TBID0 is enabled and TnSZ is set to use a 48 bit VA space). A set of
> >> controls are present to enable/disable groups of instructions (which use
> >> certain keys) for compatibility with libraries that do not utilize the
> >> feature. PAuth is used to verify the integrity of return addresses on the
> >> stack with less memory than the stack canary.
> >>
> >> This patchset adds kselftests to verify the kernel's configuration of the
> >> feature and its runtime behaviour. There are 7 tests which verify that:
> >> * an authentication failure leads to a SIGSEGV
> >> * the data/instruction instruction groups are enabled
> >> * the generic instructions are enabled
> >> * all 5 keys are unique for a single thread
> >> * exec() changes all keys to new unique ones
> >> * context switching preserves the 4 data/instruction keys
> >> * context switching preserves the generic keys
> >>
> >> The tests have been verified to work on qemu without a working PAUTH
> >> Implementation and on ARM's FVP with a full or partial PAuth
> >> implementation.
> >>
> >> Note: This patchset is only verified for ARMv8.3 and there will be some
> >> changes required for ARMv8.6. More details can be found here [1]. Once
> >> ARMv8.6 PAuth is merged the first test in this series will required to be
> >> updated.
> >
> > Nit: is it worth running checkpatch over this series?
> >
> > Although this is not kernel code, there are a number of formatting
> > weirdnesses and surplus blank lines etc. that checkpatch would probably
> > warn about.
> >
> I ran it through checkpatch and it came out clean except for some
> MAINTAINERS warnings. I see that when I add --strict it does complain
> about multiple blank lines which I can fix for the next version. Are
> there any other flags I should be running checkpatch with?

Hmmm, probably not. I had thought checkpatch was generally noisier
about that kind of thing.

Since the issues were all minor and nobody else objected, I would
suggest not to worry about them.

Cheers
---Dave

2020-09-15 16:11:36

[permalink] [raw]

Subject: Re: [PATCH 3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

On 07/09/2020 11:27 am, Dave Martin wrote:
> On Thu, Sep 03, 2020 at 11:20:25AM +0100, Boyan Karatotev wrote:
>> On 02/09/2020 18:00, Dave Martin wrote:
>>> On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
>>>> +int exec_sign_all(struct signatures *signed_vals, size_t val)
>>>> +{
>>>
>>> Could popen(3) be used here?
>>>
>>> Fork-and-exec is notoriously fiddly, so it's preferable to use a library
>>> function to do it where applicable.I would love to, but the worker needs a bidirectional channel and popen
>> only gives a unidirectional stream.
>
> Ah, fair point.
>
> Would it help if you created an additional pipe before calling popen()?
>
> May not be worth it, though. For one thing, wiring that extra pipe to
> stdin or stdout in the child process would require some extra work...
Well, I probably could, but I doubt the result would be any better. I
agree that I'm not sure the effort is worth it and would rather keep it
the same.

2020-09-16 18:05:43