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

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

[Boyan Karatotev]

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.

Changes in v2:
* remove extra lines at end of files
* Patch 1: "kselftests: add a basic arm64 Pointer Authentication test"
	* add checks for a compatible compiler in Makefile
* Patch 4: "kselftests: add PAuth tests for single threaded consistency and
key uniqueness"
	* rephrase comment for clarity in pac.c

Cc: Shuah Khan <shuah@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>

Boyan Karatotev (4):
  kselftests/arm64: add a basic Pointer Authentication test
  kselftests/arm64: add nop checks for PAuth tests
  kselftests/arm64: add PAuth test for whether exec() changes keys
  kselftests/arm64: add PAuth tests for single threaded consistency and
    key uniqueness

 tools/testing/selftests/arm64/Makefile        |   2 +-
 .../testing/selftests/arm64/pauth/.gitignore  |   2 +
 tools/testing/selftests/arm64/pauth/Makefile  |  39 ++
 .../selftests/arm64/pauth/exec_target.c       |  35 ++
 tools/testing/selftests/arm64/pauth/helper.c  |  40 ++
 tools/testing/selftests/arm64/pauth/helper.h  |  29 ++
 tools/testing/selftests/arm64/pauth/pac.c     | 348 ++++++++++++++++++
 .../selftests/arm64/pauth/pac_corruptor.S     |  35 ++
 8 files changed, 529 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/exec_target.c
 create mode 100644 tools/testing/selftests/arm64/pauth/helper.c
 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

--
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Boyan Karatotev]

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 <shuah@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
---
 tools/testing/selftests/arm64/Makefile        |  2 +-
 .../testing/selftests/arm64/pauth/.gitignore  |  1 +
 tools/testing/selftests/arm64/pauth/Makefile  | 32 +++++++++++++++++
 tools/testing/selftests/arm64/pauth/helper.h  |  9 +++++
 tools/testing/selftests/arm64/pauth/pac.c     | 31 ++++++++++++++++
 .../selftests/arm64/pauth/pac_corruptor.S     | 35 +++++++++++++++++++
 6 files changed, 109 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..01d35aaa610a
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/Makefile
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2020 ARM Limited
+
+# preserve CC value from top level Makefile
+ifeq ($(CC),cc)
+CC := $(CROSS_COMPILE)gcc
+endif
+
+CFLAGS += -mbranch-protection=pac-ret
+# check if the compiler supports ARMv8.3 and branch protection with PAuth
+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
+	$(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
+endif
diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
new file mode 100644
index 000000000000..3e0a2a404bf4
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/helper.h
@@ -0,0 +1,9 @@
+/* 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..7fc02b02dede
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/pac.c
@@ -0,0 +1,31 @@
+// 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..0780052ac3b5
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/pac_corruptor.S
@@ -0,0 +1,35 @@
+/* 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Boyan Karatotev]

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 <shuah@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
---
 .../testing/selftests/arm64/pauth/.gitignore  |  1 +
 tools/testing/selftests/arm64/pauth/Makefile  |  7 ++-
 tools/testing/selftests/arm64/pauth/helper.c  | 40 +++++++++++++++
 tools/testing/selftests/arm64/pauth/helper.h  | 10 ++++
 tools/testing/selftests/arm64/pauth/pac.c     | 51 +++++++++++++++++++
 5 files changed, 107 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 01d35aaa610a..5c0dd129562f 100644
--- a/tools/testing/selftests/arm64/pauth/Makefile
+++ b/tools/testing/selftests/arm64/pauth/Makefile
@@ -12,7 +12,7 @@ pauth_cc_support := $(shell if ($(CC) $(CFLAGS) -march=armv8.3-a -E -x c /dev/nu
 
 ifeq ($(pauth_cc_support),1)
 TEST_GEN_PROGS := pac
-TEST_GEN_FILES := pac_corruptor.o
+TEST_GEN_FILES := pac_corruptor.o helper.o
 endif
 
 include ../../lib.mk
@@ -23,10 +23,13 @@ ifeq ($(pauth_cc_support),1)
 $(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
 endif
diff --git a/tools/testing/selftests/arm64/pauth/helper.c b/tools/testing/selftests/arm64/pauth/helper.c
new file mode 100644
index 000000000000..a5d205fffe6f
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/helper.c
@@ -0,0 +1,40 @@
+// 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 3e0a2a404bf4..e2ed910c9863 100644
--- a/tools/testing/selftests/arm64/pauth/helper.h
+++ b/tools/testing/selftests/arm64/pauth/helper.h
@@ -4,6 +4,16 @@
 #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 7fc02b02dede..035fdd6aae9b 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,4 +41,42 @@ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Boyan Karatotev]

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 <shuah@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
---
 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 5c0dd129562f..72e290b0b10c 100644
--- a/tools/testing/selftests/arm64/pauth/Makefile
+++ b/tools/testing/selftests/arm64/pauth/Makefile
@@ -13,6 +13,7 @@ pauth_cc_support := $(shell if ($(CC) $(CFLAGS) -march=armv8.3-a -E -x c /dev/nu
 ifeq ($(pauth_cc_support),1)
 TEST_GEN_PROGS := pac
 TEST_GEN_FILES := pac_corruptor.o helper.o
+TEST_GEN_PROGS_EXTENDED := exec_target
 endif
 
 include ../../lib.mk
@@ -30,6 +31,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
 endif
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 e2ed910c9863..da6457177727 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 035fdd6aae9b..1b9e3acfeb61 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,4 +192,39 @@ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Boyan Karatotev]

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 <shuah@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
---
 tools/testing/selftests/arm64/pauth/pac.c | 118 ++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
index 1b9e3acfeb61..7d0ba2ec2e9e 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,40 @@ 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;
+	}
+
+	/*
+	 * In Linux the PAC field can be up to 7 bits wide. Even if keys are
+	 * unique, there is about 5% chance for PACs to collide with different
+	 * addresses. This chance rapidly increases with fewer bits allocated
+	 * for the PAC (e.g. wider address). 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,4 +302,47 @@ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Shuah Khan]

On 8/31/20 5:04 AM, 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.
> 
> Changes in v2:
> * remove extra lines at end of files
> * Patch 1: "kselftests: add a basic arm64 Pointer Authentication test"
> 	* add checks for a compatible compiler in Makefile
> * Patch 4: "kselftests: add PAuth tests for single threaded consistency and
> key uniqueness"
> 	* rephrase comment for clarity in pac.c
> 
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
> Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
> 
> Boyan Karatotev (4):
>    kselftests/arm64: add a basic Pointer Authentication test
>    kselftests/arm64: add nop checks for PAuth tests
>    kselftests/arm64: add PAuth test for whether exec() changes keys
>    kselftests/arm64: add PAuth tests for single threaded consistency and
>      key uniqueness
> 
>   tools/testing/selftests/arm64/Makefile        |   2 +-
>   .../testing/selftests/arm64/pauth/.gitignore  |   2 +
>   tools/testing/selftests/arm64/pauth/Makefile  |  39 ++
>   .../selftests/arm64/pauth/exec_target.c       |  35 ++
>   tools/testing/selftests/arm64/pauth/helper.c  |  40 ++
>   tools/testing/selftests/arm64/pauth/helper.h  |  29 ++
>   tools/testing/selftests/arm64/pauth/pac.c     | 348 ++++++++++++++++++
>   .../selftests/arm64/pauth/pac_corruptor.S     |  35 ++
>   8 files changed, 529 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/exec_target.c
>   create mode 100644 tools/testing/selftests/arm64/pauth/helper.c
>   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
> 
> --
> 2.17.1
> 
> 

Will, Catalin,

Patches look good to me from selftests perspective. My acked by
for these patches to go through arm64.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

If you would like me to take these through kselftest tree, give
me your Acks. I can queue these up for 5.10-rc1

thanks,
-- Shuah



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dave Martin]

On Mon, Aug 31, 2020 at 12:04:49PM +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 <shuah@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
> Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
> ---
>  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 5c0dd129562f..72e290b0b10c 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -13,6 +13,7 @@ pauth_cc_support := $(shell if ($(CC) $(CFLAGS) -march=armv8.3-a -E -x c /dev/nu
>  ifeq ($(pauth_cc_support),1)
>  TEST_GEN_PROGS := pac
>  TEST_GEN_FILES := pac_corruptor.o helper.o
> +TEST_GEN_PROGS_EXTENDED := exec_target
>  endif
>  
>  include ../../lib.mk
> @@ -30,6 +31,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
>  endif
> 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 e2ed910c9863..da6457177727 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 035fdd6aae9b..1b9e3acfeb61 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;

Can we simplify this with popen(3)?  Fork-and-exec is notoriously
fiddly...

[...]

> +/*
> + * 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)
> +{

The kernel doesn't guarantee that keys are unique.

Can we present all the "unique keys" wording differently, say

	exec_key_collision_likely()

Otherwise people might infer from this test code that the keys are
supposed to be truly unique and start reporting bugs on the kernel.

I can't see an obvious security argument for unique keys (rather, the
keys just need to be "unique enough".  That's the job of
get_random_bytes().)

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Boyan Karatotev]

On 02/09/2020 18:08, Dave Martin wrote:
> On Mon, Aug 31, 2020 at 12:04:49PM +0100, Boyan Karatotev wrote:
>> +/*
>> + * 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)
>> +{
> 
> The kernel doesn't guarantee that keys are unique.
> 
> Can we present all the "unique keys" wording differently, say
> 
> 	exec_key_collision_likely()

I agree that this test's name is a bit out of place. I would rather have
it named "exec_changed_keys" though.

> Otherwise people might infer from this test code that the keys are
> supposed to be truly unique and start reporting bugs on the kernel.
> 
> I can't see an obvious security argument for unique keys (rather, the
> keys just need to be "unique enough".  That's the job of
> get_random_bytes().)

The "exec_unique_keys" test only checks that the keys changed after an
exec() which I think the name change would reflect.

The thing with the "single_thread_unique_keys" test is that the kernel
says the the keys will be random. Yes, there is no uniqueness guarantee
but I'm not sure how to phrase it differently. There is some minuscule
chance that the keys end up the same, but for this test I pretend this
will not happen. Would changing up the comments and the failure message
communicate this? Maybe substitute "unique" for "different" and say how
many keys clashed?

-- 
Regards,
Boyan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dave Martin]

On Thu, Sep 03, 2020 at 11:48:37AM +0100, Boyan Karatotev wrote:
> On 02/09/2020 18:08, Dave Martin wrote:
> > On Mon, Aug 31, 2020 at 12:04:49PM +0100, Boyan Karatotev wrote:
> >> +/*
> >> + * 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)
> >> +{
> > 
> > The kernel doesn't guarantee that keys are unique.
> > 
> > Can we present all the "unique keys" wording differently, say
> > 
> > 	exec_key_collision_likely()
> 
> I agree that this test's name is a bit out of place. I would rather have
> it named "exec_changed_keys" though.
> 
> > Otherwise people might infer from this test code that the keys are
> > supposed to be truly unique and start reporting bugs on the kernel.
> > 
> > I can't see an obvious security argument for unique keys (rather, the
> > keys just need to be "unique enough".  That's the job of
> > get_random_bytes().)
> 
> The "exec_unique_keys" test only checks that the keys changed after an
> exec() which I think the name change would reflect.
> 
> The thing with the "single_thread_unique_keys" test is that the kernel
> says the the keys will be random. Yes, there is no uniqueness guarantee
> but I'm not sure how to phrase it differently. There is some minuscule
> chance that the keys end up the same, but for this test I pretend this
> will not happen. Would changing up the comments and the failure message
> communicate this? Maybe substitute "unique" for "different" and say how
> many keys clashed?

Yes, something like that seems reasonable.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Will Deacon]

On Mon, Aug 31, 2020 at 12:04:46PM +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.

Any chance of a v3 addressing the couple of small comments from Dave on
the third patch, please? Then I can pick up the whole lot for 5.10.

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Vincenzo Frascino]

Hi Will,

On 9/11/20 7:15 PM, Will Deacon wrote:
> On Mon, Aug 31, 2020 at 12:04:46PM +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.
> 
> Any chance of a v3 addressing the couple of small comments from Dave on
> the third patch, please? Then I can pick up the whole lot for 5.10.
> 

Boyan is on it. Thank you.

> Cheers,
> 
> Will
> 

-- 
Regards,
Vincenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Amit Kachhap]

Hi Boyan,

On 8/31/20 4:34 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 <shuah@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>
> Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
> ---
>   tools/testing/selftests/arm64/Makefile        |  2 +-

[...]

> +
> +/* check that a corrupted PAC results in SIGSEGV */
> +TEST_SIGNAL(corrupt_pac, SIGSEGV)
> +{
> +	ASSERT_PAUTH_ENABLED();
> +
> +	pac_corruptor();

With 8.6-Pauth extension merged in arm tree [1]. It makes sense to 
verify PAC corruption for both SIGSEGV and SIGILL signals.

Code something like below handles both the cases.

-----------------------------------------------------------------------------------
  int exec_sign_all(struct signatures *signed_vals, size_t val)
@@ -187,12 +188,29 @@ int exec_sign_all(struct signatures *signed_vals, 
size_t val)
         return 0;
  }

-/* check that a corrupted PAC results in SIGSEGV */
-TEST_SIGNAL(corrupt_pac, SIGSEGV)
+sigjmp_buf jmpbuf;
+void pac_signal_handler(int signum, siginfo_t *si, void *uc)
  {
-       ASSERT_PAUTH_ENABLED();
+       if (signum == SIGSEGV || signum == SIGILL) {
+               siglongjmp(jmpbuf, 1);
+       }
+}
+
+/* check that a corrupted PAC results in SIGSEGV or SIGILL */
+TEST(corrupt_pac)
+{
+       struct sigaction sa;

-       pac_corruptor();
+       ASSERT_PAUTH_ENABLED();
+       if (sigsetjmp(jmpbuf, 1) == 0) {
+               sa.sa_sigaction = pac_signal_handler;
+               sa.sa_flags = SA_SIGINFO;
+               sigemptyset(&sa.sa_mask);
+               sigaction(SIGSEGV, &sa, NULL);
+               sigaction(SIGILL, &sa, NULL);
+               pac_corruptor();
+               ASSERT_TRUE(0) TH_LOG("SIGSEGV/SIGILL signal did not 
occur");
+       }
  }

  /*
@@ -265,7 +283,7 @@ TEST(single_thread_different_keys)

                 tmp = n_same_single_set(&signed_vals, nkeys);
---------------------------------------------------------------------------------------


Thanks,
Amit Daniel

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=for-next/ptrauth


Regards,
Amit Daniel
> +}
> +
> +TEST_HARNESS_MAIN

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Boyan Karatotev]

On 16/09/2020 1:11 pm, Amit Kachhap wrote:
> On 8/31/20 4:34 PM, Boyan Karatotev wrote:
>> PAuth signs and verifies return addresses on the stack. It does so by
>> +
>> +/* check that a corrupted PAC results in SIGSEGV */
>> +TEST_SIGNAL(corrupt_pac, SIGSEGV)
>> +{
>> +    ASSERT_PAUTH_ENABLED();
>> +
>> +    pac_corruptor();
>
> With 8.6-Pauth extension merged in arm tree [1]. It makes sense to
> verify PAC corruption for both SIGSEGV and SIGILL signals.
>
> Code something like below handles both the cases.
>
>
-----------------------------------------------------------------------------------
>
>  int exec_sign_all(struct signatures *signed_vals, size_t val)
> @@ -187,12 +188,29 @@ int exec_sign_all(struct signatures *signed_vals,
> size_t val)
>         return 0;
>  }
>
> -/* check that a corrupted PAC results in SIGSEGV */
> -TEST_SIGNAL(corrupt_pac, SIGSEGV)
> +sigjmp_buf jmpbuf;
> +void pac_signal_handler(int signum, siginfo_t *si, void *uc)
>  {
> -       ASSERT_PAUTH_ENABLED();
> +       if (signum == SIGSEGV || signum == SIGILL) {
> +               siglongjmp(jmpbuf, 1);
> +       }
> +}
> +
> +/* check that a corrupted PAC results in SIGSEGV or SIGILL */
> +TEST(corrupt_pac)
> +{
> +       struct sigaction sa;
>
> -       pac_corruptor();
> +       ASSERT_PAUTH_ENABLED();
> +       if (sigsetjmp(jmpbuf, 1) == 0) {
> +               sa.sa_sigaction = pac_signal_handler;
> +               sa.sa_flags = SA_SIGINFO;
> +               sigemptyset(&sa.sa_mask);
> +               sigaction(SIGSEGV, &sa, NULL);
> +               sigaction(SIGILL, &sa, NULL);
> +               pac_corruptor();
> +               ASSERT_TRUE(0) TH_LOG("SIGSEGV/SIGILL signal did not
> occur");
> +       }
>  }
>
>  /*
> @@ -265,7 +283,7 @@ TEST(single_thread_different_keys)
>
>                 tmp = n_same_single_set(&signed_vals, nkeys);
>
---------------------------------------------------------------------------------------
>
>
>
> Thanks,
> Amit Daniel
>
> [1]:
>
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=for-next/ptrauth

Okay, I will add this and post it with the next version.

Regards,
Boyan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel