[PATCH v2 00/12] arm64: ARMv8.5-A: Branch Target Identification support

[PATCH v2 00/12] arm64: ARMv8.5-A: Branch Target Identification support

[Dave Martin]

This patch implements support for ARMv8.5-A Branch Target Identification
(BTI), which is a control flow integrity protection feature introduced
as part of the ARMv8.5-A extensions.

The series is based on v5.4-rc2.

A branch for this series is available in Git [4].

This series supersedes the previous posting [1], and also incorporates
my proposed ELF GNU property parsing implementation, previously posted
separately [2] (see [3] for the ABI spec describing
NT_GNU_PROPERTY_TYPE_0).

Changes:

 * Mostly minor cleanups, renaming of #defines, renumbering of HWCAPs
   that lost the race for upstream etc.

   See the individual patches for details.


Potential open issues:

 * Not tested with hugepages yet.  (If anyone has any suggestions about
   how best to do that, please shout!)

   Possibly this series is missing some hugepage related #define updates
   to allow the GP bit to be taken into account when merging/shattering
   hugepages -- anyone who understands this stuff, please comment :)

 * The VM_ARM64_BTI flag (i.e., the intenal vma flag corresponding to
   PROT_BTI) currently reads out in /proc/<pid>/smaps as the generic
   string "ar".  Perhaps we should have a dedicated string for this,
   and/or use a dedicated VM_HIGH_ARCH_BIT_* flag instead of VM_ARCH_1.

 * This series does not add BTI protection in the vDSO, so user code can
   still jump to random locations in there via function pointers.  This
   doesn't break anything, but it would be a good idea to close it down,
   to minimise the number of potentially accessible JOP gadgets for
   userspace.

   This could be added in a later patch.

Tested on the ARM Fast Model.

Notes:

 * GCC 9 can compile backwards-compatible BTI-enabled code with
   -mbranch-protection=bti or -mbranch-protection=standard.

 * Binutils trunk supports the new ELF note, but this wasn't in a release
   the last time I posted this series.  (The situation _might_ have changed
   in the meantime...)

   Creation of a BTI-enabled binary requires _everything_ linked in to
   be BTI-enabled.  For now ld --force-bti can be used to override this,
   but some things may break until the required C library support is in
   place.

   There is no straightforward way to mark a .s file as BTI-enabled:
   scraping the output from gcc -S works as a quick hack for now.

   readelf -n can be used to examing the program properties in an ELF
   file.

 * Runtime mmap() and mprotect() can be used to enable BTI on a
   page-by-page basis using the new PROT_BTI, but the code in the
   affected pages still needs to be written or compiled to contain the
   appopriate BTI landing pads.


[1] [PATCH 0/8] arm64: ARMv8.5-A: Branch Target Identification support
https://lore.kernel.org/linux-arm-kernel/1558693533-13465-1-git-send-email-Dave.Martin@arm.com/

[2] [RFC PATCH v2 0/2] ELF: Alternate program property parser
https://lore.kernel.org/lkml/1566581020-9953-1-git-send-email-Dave.Martin@arm.com/

[3] Linux Extensions to gABI
https://github.com/hjl-tools/linux-abi/wiki/Linux-Extensions-to-gABI

[4] Git branch:
git://linux-arm.org/linux-dm.git arm64/bti/v2/head
http://linux-arm.org/git?p=linux-dm.git;a=shortlog;h=refs/heads/arm64/bti/v2/head


Dave Martin (12):
  ELF: UAPI and Kconfig additions for ELF program properties
  ELF: Add ELF program property parsing support
  mm: Reserve asm-generic prot flag 0x10 for arch use
  arm64: docs: cpu-feature-registers: Document ID_AA64PFR1_EL1
  arm64: Basic Branch Target Identification support
  elf: Allow arch to tweak initial mmap prot flags
  arm64: elf: Enable BTI at exec based on ELF program properties
  arm64: BTI: Decode BYTPE bits when printing PSTATE
  arm64: traps: Fix inconsistent faulting instruction skipping
  arm64: traps: Shuffle code to eliminate forward declarations
  arm64: BTI: Reset BTYPE when skipping emulated instructions
  KVM: arm64: BTI: Reset BTYPE when skipping emulated instructions

 Documentation/arm64/cpu-feature-registers.rst |  17 ++-
 Documentation/arm64/elf_hwcaps.rst            |   4 +
 arch/arm64/Kconfig                            |  26 +++++
 arch/arm64/include/asm/cpucaps.h              |   3 +-
 arch/arm64/include/asm/cpufeature.h           |   6 ++
 arch/arm64/include/asm/elf.h                  |  50 +++++++++
 arch/arm64/include/asm/esr.h                  |   2 +-
 arch/arm64/include/asm/hwcap.h                |   1 +
 arch/arm64/include/asm/kvm_emulate.h          |   4 +-
 arch/arm64/include/asm/mman.h                 |  33 ++++++
 arch/arm64/include/asm/pgtable-hwdef.h        |   1 +
 arch/arm64/include/asm/pgtable.h              |   2 +-
 arch/arm64/include/asm/ptrace.h               |   8 ++
 arch/arm64/include/asm/sysreg.h               |   4 +
 arch/arm64/include/uapi/asm/hwcap.h           |   1 +
 arch/arm64/include/uapi/asm/mman.h            |   9 ++
 arch/arm64/include/uapi/asm/ptrace.h          |   1 +
 arch/arm64/kernel/cpufeature.c                |  33 ++++++
 arch/arm64/kernel/cpuinfo.c                   |   1 +
 arch/arm64/kernel/entry.S                     |  11 ++
 arch/arm64/kernel/process.c                   |  36 ++++++-
 arch/arm64/kernel/ptrace.c                    |   2 +-
 arch/arm64/kernel/signal.c                    |   5 +
 arch/arm64/kernel/syscall.c                   |  18 ++++
 arch/arm64/kernel/traps.c                     | 126 +++++++++++-----------
 fs/Kconfig.binfmt                             |   6 ++
 fs/binfmt_elf.c                               | 145 ++++++++++++++++++++++++--
 fs/compat_binfmt_elf.c                        |   4 +
 include/linux/elf.h                           |  43 ++++++++
 include/linux/mm.h                            |   3 +
 include/uapi/asm-generic/mman-common.h        |   1 +
 include/uapi/linux/elf.h                      |  11 ++
 32 files changed, 539 insertions(+), 78 deletions(-)
 create mode 100644 arch/arm64/include/asm/mman.h
 create mode 100644 arch/arm64/include/uapi/asm/mman.h

-- 
2.1.4

[PATCH v2 01/12] ELF: UAPI and Kconfig additions for ELF program properties

[Dave Martin]

Pull the basic ELF definitions relating to the
NT_GNU_PROPERTY_TYPE_0 note from Yu-Cheng Yu's earlier x86 shstk
series.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since RFC v2:

 * Fix struct gnu_property indentation to follow the linux coding style
---
 fs/Kconfig.binfmt        | 3 +++
 include/linux/elf.h      | 8 ++++++++
 include/uapi/linux/elf.h | 1 +
 3 files changed, 12 insertions(+)

diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index 62dc4f5..d2cfe07 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -36,6 +36,9 @@ config COMPAT_BINFMT_ELF
 config ARCH_BINFMT_ELF_STATE
 	bool
 
+config ARCH_USE_GNU_PROPERTY
+	bool
+
 config BINFMT_ELF_FDPIC
 	bool "Kernel support for FDPIC ELF binaries"
 	default y if !BINFMT_ELF
diff --git a/include/linux/elf.h b/include/linux/elf.h
index e3649b3..459cddc 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -2,6 +2,7 @@
 #ifndef _LINUX_ELF_H
 #define _LINUX_ELF_H
 
+#include <linux/types.h>
 #include <asm/elf.h>
 #include <uapi/linux/elf.h>
 
@@ -56,4 +57,11 @@ static inline int elf_coredump_extra_notes_write(struct coredump_params *cprm) {
 extern int elf_coredump_extra_notes_size(void);
 extern int elf_coredump_extra_notes_write(struct coredump_params *cprm);
 #endif
+
+/* NT_GNU_PROPERTY_TYPE_0 header */
+struct gnu_property {
+	u32 pr_type;
+	u32 pr_datasz;
+};
+
 #endif /* _LINUX_ELF_H */
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 34c02e4..c377314 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -36,6 +36,7 @@ typedef __s64	Elf64_Sxword;
 #define PT_LOPROC  0x70000000
 #define PT_HIPROC  0x7fffffff
 #define PT_GNU_EH_FRAME		0x6474e550
+#define PT_GNU_PROPERTY		0x6474e553
 
 #define PT_GNU_STACK	(PT_LOOS + 0x474e551)
 
-- 
2.1.4

[PATCH v2 02/12] ELF: Add ELF program property parsing support

[Dave Martin]

ELF program properties will be needed for detecting whether to
enable optional architecture or ABI features for a new ELF process.

For now, there are no generic properties that we care about, so do
nothing unless CONFIG_ARCH_USE_GNU_PROPERTY=y.

Otherwise, the presence of properties using the PT_PROGRAM_PROPERTY
phdrs entry (if any), and notify each property to the arch code.

For now, the added code is not used.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

[Dropped Kees' Reviewed-by -- rework in parse_elf_property() needs
review.]

Changes since RFC v2:

 * Rework parse_elf_property() for clearer tracking the current parsing
   cursor in the property data.

   o is now the current offset into data.

   datasz is now kept updated to reflect the amount of data _remaining_
   starting at offset o.  This hopefully makes
   "next item size > remaining data size" checks more intuitive.

   The final offset is assigned back to *off on success, as before.

 * Rename elf*_gnu_property_align #defines to upper case, to reflect
   the fact that these are contants, not type name aliases.
---
 fs/binfmt_elf.c          | 127 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/compat_binfmt_elf.c   |   4 ++
 include/linux/elf.h      |  19 +++++++
 include/uapi/linux/elf.h |   4 ++
 4 files changed, 154 insertions(+)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index c5642bc..ae345f6 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -39,12 +39,18 @@
 #include <linux/sched/coredump.h>
 #include <linux/sched/task_stack.h>
 #include <linux/sched/cputime.h>
+#include <linux/sizes.h>
+#include <linux/types.h>
 #include <linux/cred.h>
 #include <linux/dax.h>
 #include <linux/uaccess.h>
 #include <asm/param.h>
 #include <asm/page.h>
 
+#ifndef ELF_COMPAT
+#define ELF_COMPAT 0
+#endif
+
 #ifndef user_long_t
 #define user_long_t long
 #endif
@@ -670,6 +676,111 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
  * libraries.  There is no binary dependent code anywhere else.
  */
 
+static int parse_elf_property(const char *data, size_t *off, size_t datasz,
+			      struct arch_elf_state *arch,
+			      bool have_prev_type, u32 *prev_type)
+{
+	size_t o, step;
+	const struct gnu_property *pr;
+	int ret;
+
+	if (*off == datasz)
+		return -ENOENT;
+
+	if (WARN_ON(*off > datasz || *off % ELF_GNU_PROPERTY_ALIGN))
+		return -EIO;
+	o = *off;
+	datasz -= *off;
+
+	if (datasz < sizeof(*pr))
+		return -EIO;
+	pr = (const struct gnu_property *)(data + o);
+	o += sizeof(*pr);
+	datasz -= sizeof(*pr);
+
+	if (pr->pr_datasz > datasz)
+		return -EIO;
+
+	WARN_ON(o % ELF_GNU_PROPERTY_ALIGN);
+	step = round_up(pr->pr_datasz, ELF_GNU_PROPERTY_ALIGN);
+	if (step > datasz)
+		return -EIO;
+
+	/* Properties are supposed to be unique and sorted on pr_type: */
+	if (have_prev_type && pr->pr_type <= *prev_type)
+		return -EIO;
+	*prev_type = pr->pr_type;
+
+	ret = arch_parse_elf_property(pr->pr_type, data + o,
+				      pr->pr_datasz, ELF_COMPAT, arch);
+	if (ret)
+		return ret;
+
+	*off = o + step;
+	return 0;
+}
+
+#define NOTE_DATA_SZ SZ_1K
+#define GNU_PROPERTY_TYPE_0_NAME "GNU"
+#define NOTE_NAME_SZ (sizeof(GNU_PROPERTY_TYPE_0_NAME))
+
+static int parse_elf_properties(struct file *f, const struct elf_phdr *phdr,
+				struct arch_elf_state *arch)
+{
+	union {
+		struct elf_note nhdr;
+		char data[NOTE_DATA_SZ];
+	} note;
+	loff_t pos;
+	ssize_t n;
+	size_t off, datasz;
+	int ret;
+	bool have_prev_type;
+	u32 prev_type;
+
+	if (!IS_ENABLED(CONFIG_ARCH_USE_GNU_PROPERTY) || !phdr)
+		return 0;
+
+	/* load_elf_binary() shouldn't call us unless this is true... */
+	if (WARN_ON(phdr->p_type != PT_GNU_PROPERTY))
+		return -EIO;
+
+	/* If the properties are crazy large, that's too bad (for now): */
+	if (phdr->p_filesz > sizeof(note))
+		return -ENOEXEC;
+
+	pos = phdr->p_offset;
+	n = kernel_read(f, &note, phdr->p_filesz, &pos);
+
+	BUILD_BUG_ON(sizeof(note) < sizeof(note.nhdr) + NOTE_NAME_SZ);
+	if (n < 0 || n < sizeof(note.nhdr) + NOTE_NAME_SZ)
+		return -EIO;
+
+	if (note.nhdr.n_type != NT_GNU_PROPERTY_TYPE_0 ||
+	    note.nhdr.n_namesz != NOTE_NAME_SZ ||
+	    strncmp(note.data + sizeof(note.nhdr),
+		    GNU_PROPERTY_TYPE_0_NAME, n - sizeof(note.nhdr)))
+		return -EIO;
+
+	off = round_up(sizeof(note.nhdr) + NOTE_NAME_SZ,
+		       ELF_GNU_PROPERTY_ALIGN);
+	if (off > n)
+		return -EIO;
+
+	if (note.nhdr.n_descsz > n - off)
+		return -EIO;
+	datasz = off + note.nhdr.n_descsz;
+
+	have_prev_type = false;
+	do {
+		ret = parse_elf_property(note.data, &off, datasz, arch,
+					 have_prev_type, &prev_type);
+		have_prev_type = true;
+	} while (!ret);
+
+	return ret == -ENOENT ? 0 : ret;
+}
+
 static int load_elf_binary(struct linux_binprm *bprm)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */
@@ -677,6 +788,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
 	int load_addr_set = 0;
 	unsigned long error;
 	struct elf_phdr *elf_ppnt, *elf_phdata, *interp_elf_phdata = NULL;
+	struct elf_phdr *elf_property_phdata = NULL;
 	unsigned long elf_bss, elf_brk;
 	int bss_prot = 0;
 	int retval, i;
@@ -724,6 +836,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		char *elf_interpreter;
 		loff_t pos;
 
+		if (elf_ppnt->p_type == PT_GNU_PROPERTY) {
+			elf_property_phdata = elf_ppnt;
+			continue;
+		}
+
 		if (elf_ppnt->p_type != PT_INTERP)
 			continue;
 
@@ -819,9 +936,14 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			goto out_free_dentry;
 
 		/* Pass PT_LOPROC..PT_HIPROC headers to arch code */
+		elf_property_phdata = NULL;
 		elf_ppnt = interp_elf_phdata;
 		for (i = 0; i < loc->interp_elf_ex.e_phnum; i++, elf_ppnt++)
 			switch (elf_ppnt->p_type) {
+			case PT_GNU_PROPERTY:
+				elf_property_phdata = elf_ppnt;
+				break;
+
 			case PT_LOPROC ... PT_HIPROC:
 				retval = arch_elf_pt_proc(&loc->interp_elf_ex,
 							  elf_ppnt, interpreter,
@@ -832,6 +954,11 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			}
 	}
 
+	retval = parse_elf_properties(interpreter ?: bprm->file,
+				      elf_property_phdata, &arch_state);
+	if (retval)
+		goto out_free_dentry;
+
 	/*
 	 * Allow arch code to reject the ELF at this point, whilst it's
 	 * still possible to return an error to the code that invoked
diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c
index b7f9ffa..67896e0 100644
--- a/fs/compat_binfmt_elf.c
+++ b/fs/compat_binfmt_elf.c
@@ -17,6 +17,8 @@
 #include <linux/elfcore-compat.h>
 #include <linux/time.h>
 
+#define ELF_COMPAT	1
+
 /*
  * Rename the basic ELF layout types to refer to the 32-bit class of files.
  */
@@ -28,11 +30,13 @@
 #undef	elf_shdr
 #undef	elf_note
 #undef	elf_addr_t
+#undef	ELF_GNU_PROPERTY_ALIGN
 #define elfhdr		elf32_hdr
 #define elf_phdr	elf32_phdr
 #define elf_shdr	elf32_shdr
 #define elf_note	elf32_note
 #define elf_addr_t	Elf32_Addr
+#define ELF_GNU_PROPERTY_ALIGN	ELF32_GNU_PROPERTY_ALIGN
 
 /*
  * Some data types as stored in coredump.
diff --git a/include/linux/elf.h b/include/linux/elf.h
index 459cddc..7bdc6da 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -22,6 +22,9 @@
 	SET_PERSONALITY(ex)
 #endif
 
+#define ELF32_GNU_PROPERTY_ALIGN	4
+#define ELF64_GNU_PROPERTY_ALIGN	8
+
 #if ELF_CLASS == ELFCLASS32
 
 extern Elf32_Dyn _DYNAMIC [];
@@ -32,6 +35,7 @@ extern Elf32_Dyn _DYNAMIC [];
 #define elf_addr_t	Elf32_Off
 #define Elf_Half	Elf32_Half
 #define Elf_Word	Elf32_Word
+#define ELF_GNU_PROPERTY_ALIGN	ELF32_GNU_PROPERTY_ALIGN
 
 #else
 
@@ -43,6 +47,7 @@ extern Elf64_Dyn _DYNAMIC [];
 #define elf_addr_t	Elf64_Off
 #define Elf_Half	Elf64_Half
 #define Elf_Word	Elf64_Word
+#define ELF_GNU_PROPERTY_ALIGN	ELF64_GNU_PROPERTY_ALIGN
 
 #endif
 
@@ -64,4 +69,18 @@ struct gnu_property {
 	u32 pr_datasz;
 };
 
+struct arch_elf_state;
+
+#ifndef CONFIG_ARCH_USE_GNU_PROPERTY
+static inline int arch_parse_elf_property(u32 type, const void *data,
+					  size_t datasz, bool compat,
+					  struct arch_elf_state *arch)
+{
+	return 0;
+}
+#else
+extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
+				   bool compat, struct arch_elf_state *arch);
+#endif
+
 #endif /* _LINUX_ELF_H */
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index c377314..20900f4 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -368,6 +368,7 @@ typedef struct elf64_shdr {
  * Notes used in ET_CORE. Architectures export some of the arch register sets
  * using the corresponding note types via the PTRACE_GETREGSET and
  * PTRACE_SETREGSET requests.
+ * The note name for all these is "LINUX".
  */
 #define NT_PRSTATUS	1
 #define NT_PRFPREG	2
@@ -430,6 +431,9 @@ typedef struct elf64_shdr {
 #define NT_MIPS_FP_MODE	0x801		/* MIPS floating-point mode */
 #define NT_MIPS_MSA	0x802		/* MIPS SIMD registers */
 
+/* Note types with note name "GNU" */
+#define NT_GNU_PROPERTY_TYPE_0	5
+
 /* Note header in a PT_NOTE section */
 typedef struct elf32_note {
   Elf32_Word	n_namesz;	/* Name size */
-- 
2.1.4

[PATCH v2 03/12] mm: Reserve asm-generic prot flag 0x10 for arch use

[Dave Martin]

The asm-generic mman definitions are used by a few architectures
that also define an arch-specific PROT flag with value 0x10.  This
currently applies to sparc and powerpc, and arm64 will soon join
in.

To help future maintainers, document the use of this flag in the
asm-generic header too.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 include/uapi/asm-generic/mman-common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index c160a53..81442d2 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -11,6 +11,7 @@
 #define PROT_WRITE	0x2		/* page can be written */
 #define PROT_EXEC	0x4		/* page can be executed */
 #define PROT_SEM	0x8		/* page may be used for atomic ops */
+ /*			0x10		   reserved for arch-specific use */
 #define PROT_NONE	0x0		/* page can not be accessed */
 #define PROT_GROWSDOWN	0x01000000	/* mprotect flag: extend change to start of growsdown vma */
 #define PROT_GROWSUP	0x02000000	/* mprotect flag: extend change to end of growsup vma */
-- 
2.1.4

[PATCH v2 04/12] arm64: docs: cpu-feature-registers: Document ID_AA64PFR1_EL1

[Dave Martin]

Commit d71be2b6c0e1 ("arm64: cpufeature: Detect SSBS and advertise
to userspace") exposes ID_AA64PFR1_EL1 to userspace, but didn't
update the documentation to match.

Add it.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Note to maintainers:

 * This patch has been racing with various other attempts to fix
   the same documentation in the meantime.

   Since this patch only fixes the documenting for pre-existing
   features, it can safely be dropped if appropriate.

   The _new_ documentation relating to BTI feature reporting
   is in a subsequent patch, and needs to be retained.
---
 Documentation/arm64/cpu-feature-registers.rst | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst
index 2955287..b86828f 100644
--- a/Documentation/arm64/cpu-feature-registers.rst
+++ b/Documentation/arm64/cpu-feature-registers.rst
@@ -168,8 +168,15 @@ infrastructure:
      +------------------------------+---------+---------+
 
 
-  3) MIDR_EL1 - Main ID Register
+  3) ID_AA64PFR1_EL1 - Processor Feature Register 1
+     +------------------------------+---------+---------+
+     | Name                         |  bits   | visible |
+     +------------------------------+---------+---------+
+     | SSBS                         | [7-4]   |    y    |
+     +------------------------------+---------+---------+
+
 
+  4) MIDR_EL1 - Main ID Register
      +------------------------------+---------+---------+
      | Name                         |  bits   | visible |
      +------------------------------+---------+---------+
@@ -188,7 +195,7 @@ infrastructure:
    as available on the CPU where it is fetched and is not a system
    wide safe value.
 
-  4) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
+  5) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
 
      +------------------------------+---------+---------+
      | Name                         |  bits   | visible |
@@ -210,7 +217,7 @@ infrastructure:
      | DPB                          | [3-0]   |    y    |
      +------------------------------+---------+---------+
 
-  5) ID_AA64MMFR2_EL1 - Memory model feature register 2
+  6) ID_AA64MMFR2_EL1 - Memory model feature register 2
 
      +------------------------------+---------+---------+
      | Name                         |  bits   | visible |
@@ -218,7 +225,7 @@ infrastructure:
      | AT                           | [35-32] |    y    |
      +------------------------------+---------+---------+
 
-  6) ID_AA64ZFR0_EL1 - SVE feature ID register 0
+  7) ID_AA64ZFR0_EL1 - SVE feature ID register 0
 
      +------------------------------+---------+---------+
      | Name                         |  bits   | visible |
-- 
2.1.4

[PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

This patch adds the bare minimum required to expose the ARMv8.5
Branch Target Identification feature to userspace.

By itself, this does _not_ automatically enable BTI for any initial
executable pages mapped by execve().  This will come later, but for
now it should be possible to enable BTI manually on those pages by
using mprotect() from within the target process.

Other arches already using the generic mman.h are already using
0x10 for arch-specific prot flags, so we use that for PROT_BTI
here.

For consistency, signal handler entry points in BTI guarded pages
are required to be annotated as such, just like any other function.
This blocks a relatively minor attack vector, but comforming
userspace will have the annotations anyway, so we may as well
enforce them.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since v1:

 * Configure SCTLR_EL1.BTx to disallow BR onto a PACIxSP instruction
   (except via X16/X17):

   The AArch64 procedure call standard requires binaries marked with
   GNU_PROPERTY_AARCH64_FEATURE_1_BTI to use X16/X17 in trampolines
   and tail calls, so it makes no sense to be permissive.

 * Rename PROT_BTI_GUARDED to PROT_BTI.

 * Rename VM_ARM64_GP to VM_ARM64_BTI:

   Although the architectural name for the BTI page table bit is "GP",
   BTI is nonetheless the feature it controls.  So avoid introducing
   the "GP" naming just for this -- it's just an unecessary extra
   source of confusion.

 * Tidy up masking with ~PSR_BTYPE_MASK.

 * Drop masking out of BTYPE on SVC, with a comment outlining why.

 * Split PSR_BTYPE_SHIFT definition into this patch.  It's not
   useful yet, but it makes sense to define PSR_BTYPE_* using this
   from the outset.

 * Migrate to ct_user_exit_irqoff in entry.S:el0_bti.
---
 Documentation/arm64/cpu-feature-registers.rst |  2 ++
 Documentation/arm64/elf_hwcaps.rst            |  4 ++++
 arch/arm64/Kconfig                            | 23 +++++++++++++++++++
 arch/arm64/include/asm/cpucaps.h              |  3 ++-
 arch/arm64/include/asm/cpufeature.h           |  6 +++++
 arch/arm64/include/asm/esr.h                  |  2 +-
 arch/arm64/include/asm/hwcap.h                |  1 +
 arch/arm64/include/asm/mman.h                 | 33 +++++++++++++++++++++++++++
 arch/arm64/include/asm/pgtable-hwdef.h        |  1 +
 arch/arm64/include/asm/pgtable.h              |  2 +-
 arch/arm64/include/asm/ptrace.h               |  3 +++
 arch/arm64/include/asm/sysreg.h               |  4 ++++
 arch/arm64/include/uapi/asm/hwcap.h           |  1 +
 arch/arm64/include/uapi/asm/mman.h            |  9 ++++++++
 arch/arm64/include/uapi/asm/ptrace.h          |  1 +
 arch/arm64/kernel/cpufeature.c                | 33 +++++++++++++++++++++++++++
 arch/arm64/kernel/cpuinfo.c                   |  1 +
 arch/arm64/kernel/entry.S                     | 11 +++++++++
 arch/arm64/kernel/ptrace.c                    |  2 +-
 arch/arm64/kernel/signal.c                    |  5 ++++
 arch/arm64/kernel/syscall.c                   | 18 +++++++++++++++
 arch/arm64/kernel/traps.c                     |  7 ++++++
 include/linux/mm.h                            |  3 +++
 23 files changed, 171 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm64/include/asm/mman.h
 create mode 100644 arch/arm64/include/uapi/asm/mman.h

diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst
index b86828f..c96c7df 100644
--- a/Documentation/arm64/cpu-feature-registers.rst
+++ b/Documentation/arm64/cpu-feature-registers.rst
@@ -174,6 +174,8 @@ infrastructure:
      +------------------------------+---------+---------+
      | SSBS                         | [7-4]   |    y    |
      +------------------------------+---------+---------+
+     | BT                           | [3-0]   |    y    |
+     +------------------------------+---------+---------+
 
 
   4) MIDR_EL1 - Main ID Register
diff --git a/Documentation/arm64/elf_hwcaps.rst b/Documentation/arm64/elf_hwcaps.rst
index 91f7952..296dcac 100644
--- a/Documentation/arm64/elf_hwcaps.rst
+++ b/Documentation/arm64/elf_hwcaps.rst
@@ -201,6 +201,10 @@ HWCAP2_FRINT
 
     Functionality implied by ID_AA64ISAR1_EL1.FRINTTS == 0b0001.
 
+HWCAP2_BTI
+
+    Functionality implied by ID_AA64PFR0_EL1.BT == 0b0001.
+
 
 4. Unused AT_HWCAP bits
 -----------------------
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 41a9b42..159ee69 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1410,6 +1410,29 @@ config ARM64_PTR_AUTH
 
 endmenu
 
+menu "ARMv8.5 architectural features"
+
+config ARM64_BTI
+	bool "Branch Target Identification support"
+	default y
+	help
+	  Branch Target Identification (part of the ARMv8.5 Extensions)
+	  provides a mechanism to limit the set of locations to which computed
+	  branch instructions such as BR or BLR can jump.
+
+	  This is intended to provide complementary protection to other control
+	  flow integrity protection mechanisms, such as the Pointer
+	  authentication mechanism provided as part of the ARMv8.2 Extensions.
+
+	  To make use of BTI on CPUs that support it, say Y.
+
+	  Userspace binaries must also be specifically compiled to make use of
+	  this mechanism.  If you say N here or the hardware does not support
+	  BTI, such binaries can still run, but you get no additional
+	  enforcement of branch destinations.
+
+endmenu
+
 config ARM64_SVE
 	bool "ARM Scalable Vector Extension support"
 	default y
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index f19fe4b..946165e 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -52,7 +52,8 @@
 #define ARM64_HAS_IRQ_PRIO_MASKING		42
 #define ARM64_HAS_DCPODP			43
 #define ARM64_WORKAROUND_1463225		44
+#define ARM64_BTI				45
 
-#define ARM64_NCAPS				45
+#define ARM64_NCAPS				46
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 9cde5d2..84fa48f 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -613,6 +613,12 @@ static inline bool system_has_prio_mask_debugging(void)
 	       system_uses_irq_prio_masking();
 }
 
+static inline bool system_supports_bti(void)
+{
+	return IS_ENABLED(CONFIG_ARM64_BTI) &&
+		cpus_have_const_cap(ARM64_BTI);
+}
+
 #define ARM64_BP_HARDEN_UNKNOWN		-1
 #define ARM64_BP_HARDEN_WA_NEEDED	0
 #define ARM64_BP_HARDEN_NOT_REQUIRED	1
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index cb29253..390b8ba 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -22,7 +22,7 @@
 #define ESR_ELx_EC_PAC		(0x09)	/* EL2 and above */
 /* Unallocated EC: 0x0A - 0x0B */
 #define ESR_ELx_EC_CP14_64	(0x0C)
-/* Unallocated EC: 0x0d */
+#define ESR_ELx_EC_BTI		(0x0D)
 #define ESR_ELx_EC_ILL		(0x0E)
 /* Unallocated EC: 0x0F - 0x10 */
 #define ESR_ELx_EC_SVC32	(0x11)
diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
index 3d2f247..f9e681d 100644
--- a/arch/arm64/include/asm/hwcap.h
+++ b/arch/arm64/include/asm/hwcap.h
@@ -86,6 +86,7 @@
 #define KERNEL_HWCAP_SVESM4		__khwcap2_feature(SVESM4)
 #define KERNEL_HWCAP_FLAGM2		__khwcap2_feature(FLAGM2)
 #define KERNEL_HWCAP_FRINT		__khwcap2_feature(FRINT)
+#define KERNEL_HWCAP_BTI		__khwcap2_feature(BTI)
 
 /*
  * This yields a mask that user programs can use to figure out what
diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
new file mode 100644
index 0000000..cbfe3238
--- /dev/null
+++ b/arch/arm64/include/asm/mman.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_MMAN_H__
+#define __ASM_MMAN_H__
+
+#include <uapi/asm/mman.h>
+
+#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
+static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
+{
+	if (system_supports_bti() && (prot & PROT_BTI))
+		return VM_ARM64_BTI;
+
+	return 0;
+}
+
+#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
+static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
+{
+	return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
+}
+
+#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
+static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
+{
+	unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
+
+	if (system_supports_bti())
+		supported |= PROT_BTI;
+
+	return (prot & ~supported) == 0;
+}
+
+#endif /* ! __ASM_MMAN_H__ */
diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
index 3df60f9..f85d1fc 100644
--- a/arch/arm64/include/asm/pgtable-hwdef.h
+++ b/arch/arm64/include/asm/pgtable-hwdef.h
@@ -150,6 +150,7 @@
 #define PTE_SHARED		(_AT(pteval_t, 3) << 8)		/* SH[1:0], inner shareable */
 #define PTE_AF			(_AT(pteval_t, 1) << 10)	/* Access Flag */
 #define PTE_NG			(_AT(pteval_t, 1) << 11)	/* nG */
+#define PTE_GP			(_AT(pteval_t, 1) << 50)	/* BTI guarded */
 #define PTE_DBM			(_AT(pteval_t, 1) << 51)	/* Dirty Bit Management */
 #define PTE_CONT		(_AT(pteval_t, 1) << 52)	/* Contiguous range */
 #define PTE_PXN			(_AT(pteval_t, 1) << 53)	/* Privileged XN */
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 7576df0..a7b5a81 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -679,7 +679,7 @@ static inline phys_addr_t pgd_page_paddr(pgd_t pgd)
 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 {
 	const pteval_t mask = PTE_USER | PTE_PXN | PTE_UXN | PTE_RDONLY |
-			      PTE_PROT_NONE | PTE_VALID | PTE_WRITE;
+			      PTE_PROT_NONE | PTE_VALID | PTE_WRITE | PTE_GP;
 	/* preserve the hardware dirty information */
 	if (pte_hw_dirty(pte))
 		pte = pte_mkdirty(pte);
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index fbebb41..7d4cd59 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -35,7 +35,10 @@
 #define GIC_PRIO_PSR_I_SET		(1 << 4)
 
 /* Additional SPSR bits not exposed in the UABI */
+#define PSR_BTYPE_SHIFT		10
+
 #define PSR_IL_BIT		(1 << 20)
+#define PSR_BTYPE_CALL		(2 << PSR_BTYPE_SHIFT)
 
 /* AArch32-specific ptrace requests */
 #define COMPAT_PTRACE_GETREGS		12
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 972d196..58a5e5e 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -510,6 +510,8 @@
 #endif
 
 /* SCTLR_EL1 specific flags. */
+#define SCTLR_EL1_BT1		(BIT(36))
+#define SCTLR_EL1_BT0		(BIT(35))
 #define SCTLR_EL1_UCI		(BIT(26))
 #define SCTLR_EL1_E0E		(BIT(24))
 #define SCTLR_EL1_SPAN		(BIT(23))
@@ -599,10 +601,12 @@
 
 /* id_aa64pfr1 */
 #define ID_AA64PFR1_SSBS_SHIFT		4
+#define ID_AA64PFR1_BT_SHIFT		0
 
 #define ID_AA64PFR1_SSBS_PSTATE_NI	0
 #define ID_AA64PFR1_SSBS_PSTATE_ONLY	1
 #define ID_AA64PFR1_SSBS_PSTATE_INSNS	2
+#define ID_AA64PFR1_BT_BTI		0x1
 
 /* id_aa64zfr0 */
 #define ID_AA64ZFR0_SM4_SHIFT		40
diff --git a/arch/arm64/include/uapi/asm/hwcap.h b/arch/arm64/include/uapi/asm/hwcap.h
index a1e7288..363f569 100644
--- a/arch/arm64/include/uapi/asm/hwcap.h
+++ b/arch/arm64/include/uapi/asm/hwcap.h
@@ -65,5 +65,6 @@
 #define HWCAP2_SVESM4		(1 << 6)
 #define HWCAP2_FLAGM2		(1 << 7)
 #define HWCAP2_FRINT		(1 << 8)
+#define HWCAP2_BTI		(1 << 9)
 
 #endif /* _UAPI__ASM_HWCAP_H */
diff --git a/arch/arm64/include/uapi/asm/mman.h b/arch/arm64/include/uapi/asm/mman.h
new file mode 100644
index 0000000..6fdd71e
--- /dev/null
+++ b/arch/arm64/include/uapi/asm/mman.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI__ASM_MMAN_H
+#define _UAPI__ASM_MMAN_H
+
+#include <asm-generic/mman.h>
+
+#define PROT_BTI	0x10		/* BTI guarded page */
+
+#endif /* ! _UAPI__ASM_MMAN_H */
diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
index 7ed9294..09e66fa 100644
--- a/arch/arm64/include/uapi/asm/ptrace.h
+++ b/arch/arm64/include/uapi/asm/ptrace.h
@@ -46,6 +46,7 @@
 #define PSR_I_BIT	0x00000080
 #define PSR_A_BIT	0x00000100
 #define PSR_D_BIT	0x00000200
+#define PSR_BTYPE_MASK	0x00000c00
 #define PSR_SSBS_BIT	0x00001000
 #define PSR_PAN_BIT	0x00400000
 #define PSR_UAO_BIT	0x00800000
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9323bcc..4bab6e7 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -171,6 +171,8 @@ static const struct arm64_ftr_bits ftr_id_aa64pfr0[] = {
 
 static const struct arm64_ftr_bits ftr_id_aa64pfr1[] = {
 	ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR1_SSBS_SHIFT, 4, ID_AA64PFR1_SSBS_PSTATE_NI),
+	ARM64_FTR_BITS(FTR_VISIBLE_IF_IS_ENABLED(CONFIG_ARM64_BTI),
+				    FTR_STRICT, FTR_LOWER_SAFE, ID_AA64PFR1_BT_SHIFT, 4, 0),
 	ARM64_FTR_END,
 };
 
@@ -1260,6 +1262,21 @@ static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
 }
 #endif
 
+#ifdef CONFIG_ARM64_BTI
+static void bti_enable(const struct arm64_cpu_capabilities *__unused)
+{
+	/*
+	 * Use of X16/X17 for tail-calls and trampolines that jump to
+	 * function entry points using BR is a requirement for
+	 * marking binaries with GNU_PROPERTY_AARCH64_FEATURE_1_BTI.
+	 * So, be strict and forbid other BRs using other registers to
+	 * jump onto a PACIxSP instruction:
+	 */
+	sysreg_clear_set(sctlr_el1, 0, SCTLR_EL1_BT0 | SCTLR_EL1_BT1);
+	isb();
+}
+#endif /* CONFIG_ARM64_BTI */
+
 static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		.desc = "GIC system register CPU interface",
@@ -1560,6 +1577,19 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.min_field_value = 1,
 	},
 #endif
+#ifdef CONFIG_ARM64_BTI
+	{
+		.desc = "Branch Target Identification",
+		.capability = ARM64_BTI,
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.matches = has_cpuid_feature,
+		.cpu_enable = bti_enable,
+		.sys_reg = SYS_ID_AA64PFR1_EL1,
+		.field_pos = ID_AA64PFR1_BT_SHIFT,
+		.min_field_value = ID_AA64PFR1_BT_BTI,
+		.sign = FTR_UNSIGNED,
+	},
+#endif
 	{},
 };
 
@@ -1655,6 +1685,9 @@ static const struct arm64_cpu_capabilities arm64_elf_hwcaps[] = {
 	HWCAP_CAP(SYS_ID_AA64ZFR0_EL1, ID_AA64ZFR0_SM4_SHIFT, FTR_UNSIGNED, ID_AA64ZFR0_SM4, CAP_HWCAP, KERNEL_HWCAP_SVESM4),
 #endif
 	HWCAP_CAP(SYS_ID_AA64PFR1_EL1, ID_AA64PFR1_SSBS_SHIFT, FTR_UNSIGNED, ID_AA64PFR1_SSBS_PSTATE_INSNS, CAP_HWCAP, KERNEL_HWCAP_SSBS),
+#ifdef CONFIG_ARM64_BTI
+	HWCAP_CAP(SYS_ID_AA64PFR1_EL1, ID_AA64PFR1_BT_SHIFT, FTR_UNSIGNED, ID_AA64PFR1_BT_BTI, CAP_HWCAP, KERNEL_HWCAP_BTI),
+#endif
 #ifdef CONFIG_ARM64_PTR_AUTH
 	HWCAP_MULTI_CAP(ptr_auth_hwcap_addr_matches, CAP_HWCAP, KERNEL_HWCAP_PACA),
 	HWCAP_MULTI_CAP(ptr_auth_hwcap_gen_matches, CAP_HWCAP, KERNEL_HWCAP_PACG),
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 05933c0..e1fd053 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -84,6 +84,7 @@ static const char *const hwcap_str[] = {
 	"svesm4",
 	"flagm2",
 	"frint",
+	"bti",
 	NULL
 };
 
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 84a8227..6c5adea 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -737,6 +737,8 @@ el0_sync:
 	b.eq	el0_pc
 	cmp	x24, #ESR_ELx_EC_UNKNOWN	// unknown exception in EL0
 	b.eq	el0_undef
+	cmp	x24, #ESR_ELx_EC_BTI		// branch target exception
+	b.eq	el0_bti
 	cmp	x24, #ESR_ELx_EC_BREAKPT_LOW	// debug exception in EL0
 	b.ge	el0_dbg
 	b	el0_inv
@@ -887,6 +889,15 @@ el0_undef:
 	mov	x0, sp
 	bl	do_undefinstr
 	b	ret_to_user
+el0_bti:
+	/*
+	 * Branch target exception
+	 */
+	ct_user_exit_irqoff
+	enable_daif
+	mov	x0, sp
+	bl	do_bti
+	b	ret_to_user
 el0_sys:
 	/*
 	 * System instructions, for trapped cache maintenance instructions
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 21176d0..ff5ea70 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1853,7 +1853,7 @@ void syscall_trace_exit(struct pt_regs *regs)
  */
 #define SPSR_EL1_AARCH64_RES0_BITS \
 	(GENMASK_ULL(63, 32) | GENMASK_ULL(27, 25) | GENMASK_ULL(23, 22) | \
-	 GENMASK_ULL(20, 13) | GENMASK_ULL(11, 10) | GENMASK_ULL(5, 5))
+	 GENMASK_ULL(20, 13) | GENMASK_ULL(5, 5))
 #define SPSR_EL1_AARCH32_RES0_BITS \
 	(GENMASK_ULL(63, 32) | GENMASK_ULL(22, 22) | GENMASK_ULL(20, 20))
 
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index dd2cdc0..4a3bd32 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -730,6 +730,11 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
 	regs->regs[29] = (unsigned long)&user->next_frame->fp;
 	regs->pc = (unsigned long)ka->sa.sa_handler;
 
+	if (system_supports_bti()) {
+		regs->pstate &= ~PSR_BTYPE_MASK;
+		regs->pstate |= PSR_BTYPE_CALL;
+	}
+
 	if (ka->sa.sa_flags & SA_RESTORER)
 		sigtramp = ka->sa.sa_restorer;
 	else
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 871c739..b6b8e48 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -98,6 +98,24 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
 	regs->orig_x0 = regs->regs[0];
 	regs->syscallno = scno;
 
+	/*
+	 * BTI note:
+	 * The architecture does not guarantee that SPSR.BTYPE is zero
+	 * on taking an SVC, so we could return to userspace with a
+	 * non-zero BTYPE after the syscall.
+	 *
+	 * This shouldn't matter except when userspace is explicitly
+	 * doing something stupid, such as setting PROT_BTI on a page
+	 * that lacks conforming BTI/PACIxSP instructions, falling
+	 * through from one executable page to another with differing
+	 * PROT_BTI, or messing with BYTPE via ptrace: in such cases,
+	 * userspace should not be surprised if a SIGILL occurs on
+	 * syscall return.
+	 *
+	 * So, don't touch regs->pstate & PSR_BTYPE_MASK here.
+	 * (Similarly for HVC and SMC elsewhere.)
+	 */
+
 	cortex_a76_erratum_1463225_svc_handler();
 	local_daif_restore(DAIF_PROCCTX);
 	user_exit();
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 34739e8..15e3c4f 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -406,6 +406,12 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
 	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc);
 }
 
+asmlinkage void __exception do_bti(struct pt_regs *regs)
+{
+	BUG_ON(!user_mode(regs));
+	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc);
+}
+
 #define __user_cache_maint(insn, address, res)			\
 	if (address >= user_addr_max()) {			\
 		res = -EFAULT;					\
@@ -737,6 +743,7 @@ static const char *esr_class_str[] = {
 	[ESR_ELx_EC_CP10_ID]		= "CP10 MRC/VMRS",
 	[ESR_ELx_EC_PAC]		= "PAC",
 	[ESR_ELx_EC_CP14_64]		= "CP14 MCRR/MRRC",
+	[ESR_ELx_EC_BTI]		= "BTI",
 	[ESR_ELx_EC_ILL]		= "PSTATE.IL",
 	[ESR_ELx_EC_SVC32]		= "SVC (AArch32)",
 	[ESR_ELx_EC_HVC32]		= "HVC (AArch32)",
diff --git a/include/linux/mm.h b/include/linux/mm.h
index cc29227..5ed5a99 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -329,6 +329,9 @@ extern unsigned int kobjsize(const void *objp);
 #elif defined(CONFIG_SPARC64)
 # define VM_SPARC_ADI	VM_ARCH_1	/* Uses ADI tag for access control */
 # define VM_ARCH_CLEAR	VM_SPARC_ADI
+#elif defined(CONFIG_ARM64)
+# define VM_ARM64_BTI	VM_ARCH_1	/* BTI guarded page, a.k.a. GP bit */
+# define VM_ARCH_CLEAR	VM_ARM64_BTI
 #elif !defined(CONFIG_MMU)
 # define VM_MAPPED_COPY	VM_ARCH_1	/* T if mapped copy of data (nommu mmap) */
 #endif
-- 
2.1.4

[PATCH v2 06/12] elf: Allow arch to tweak initial mmap prot flags

[Dave Martin]

An arch may want to tweak the mmap prot flags for an
ELFexecutable's initial mappings.  For example, arm64 is going to
need to add PROT_BTI for executable pages in an ELF process whose
executable is marked as using Branch Target Identification (an
ARMv8.5-A control flow integrity feature).

So that this can be done in a generic way, add a hook
arch_elf_adjust_prot() to modify the prot flags as desired: arches
can select CONFIG_HAVE_ELF_PROT and implement their own backend
where necessary.

By default, leave the prot flags unchanged.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 fs/Kconfig.binfmt   |  3 +++
 fs/binfmt_elf.c     | 18 ++++++++++++------
 include/linux/elf.h | 12 ++++++++++++
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index d2cfe07..2358368 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -36,6 +36,9 @@ config COMPAT_BINFMT_ELF
 config ARCH_BINFMT_ELF_STATE
 	bool
 
+config ARCH_HAVE_ELF_PROT
+	bool
+
 config ARCH_USE_GNU_PROPERTY
 	bool
 
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index ae345f6..dbfab2e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -531,7 +531,8 @@ static inline int arch_check_elf(struct elfhdr *ehdr, bool has_interp,
 
 #endif /* !CONFIG_ARCH_BINFMT_ELF_STATE */
 
-static inline int make_prot(u32 p_flags)
+static inline int make_prot(u32 p_flags, struct arch_elf_state *arch_state,
+			    bool has_interp, bool is_interp)
 {
 	int prot = 0;
 
@@ -541,7 +542,8 @@ static inline int make_prot(u32 p_flags)
 		prot |= PROT_WRITE;
 	if (p_flags & PF_X)
 		prot |= PROT_EXEC;
-	return prot;
+
+	return arch_elf_adjust_prot(prot, arch_state, has_interp, is_interp);
 }
 
 /* This is much more generalized than the library routine read function,
@@ -551,7 +553,8 @@ static inline int make_prot(u32 p_flags)
 
 static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 		struct file *interpreter, unsigned long *interp_map_addr,
-		unsigned long no_base, struct elf_phdr *interp_elf_phdata)
+		unsigned long no_base, struct elf_phdr *interp_elf_phdata,
+		struct arch_elf_state *arch_state)
 {
 	struct elf_phdr *eppnt;
 	unsigned long load_addr = 0;
@@ -583,7 +586,8 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
 	for (i = 0; i < interp_elf_ex->e_phnum; i++, eppnt++) {
 		if (eppnt->p_type == PT_LOAD) {
 			int elf_type = MAP_PRIVATE | MAP_DENYWRITE;
-			int elf_prot = make_prot(eppnt->p_flags);
+			int elf_prot = make_prot(eppnt->p_flags, arch_state,
+						 true, true);
 			unsigned long vaddr = 0;
 			unsigned long k, map_addr;
 
@@ -1040,7 +1044,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
 			}
 		}
 
-		elf_prot = make_prot(elf_ppnt->p_flags);
+		elf_prot = make_prot(elf_ppnt->p_flags, &arch_state,
+				     !!interpreter, false);
 
 		elf_flags = MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE;
 
@@ -1186,7 +1191,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
 		elf_entry = load_elf_interp(&loc->interp_elf_ex,
 					    interpreter,
 					    &interp_map_addr,
-					    load_bias, interp_elf_phdata);
+					    load_bias, interp_elf_phdata,
+					    &arch_state);
 		if (!IS_ERR((void *)elf_entry)) {
 			/*
 			 * load_elf_interp() returns relocation
diff --git a/include/linux/elf.h b/include/linux/elf.h
index 7bdc6da..1b6e895 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -83,4 +83,16 @@ extern int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
 				   bool compat, struct arch_elf_state *arch);
 #endif
 
+#ifdef CONFIG_ARCH_HAVE_ELF_PROT
+int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
+			 bool has_interp, bool is_interp);
+#else
+static inline int arch_elf_adjust_prot(int prot,
+				       const struct arch_elf_state *state,
+				       bool has_interp, bool is_interp)
+{
+	return prot;
+}
+#endif
+
 #endif /* _LINUX_ELF_H */
-- 
2.1.4

[PATCH v2 07/12] arm64: elf: Enable BTI at exec based on ELF program properties

[Dave Martin]

For BTI protection to be as comprehensive as possible, it is
desirable to have BTI enabled from process startup.  If this is not
done, the process must use mprotect() to enable BTI for each of its
executable mappings, but this is painful to do in the libc startup
code.  It's simpler and more sound to have the kernel do it
instead.

To this end, detect BTI support in the executable (or ELF
interpreter, as appropriate), via the
NT_GNU_PROGRAM_PROPERTY_TYPE_0 note, and tweak the initial prot
flags for the process' executable pages to include PROT_BTI as
appropriate.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since v1:

 * Migrate to new ELF program property parser
   (implemented by "ELF: Add ELF program property parsing support").

 * Rename PROT_BTI_GUARDED to PROT_BTI.
---
 arch/arm64/Kconfig           |  3 +++
 arch/arm64/include/asm/elf.h | 50 ++++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/process.c  | 19 +++++++++++++++++
 include/linux/elf.h          |  6 +++++-
 include/uapi/linux/elf.h     |  6 ++++++
 5 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 159ee69..563dec5 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -9,6 +9,7 @@ config ARM64
 	select ACPI_MCFG if (ACPI && PCI)
 	select ACPI_SPCR_TABLE if ACPI
 	select ACPI_PPTT if ACPI
+	select ARCH_BINFMT_ELF_STATE
 	select ARCH_CLOCKSOURCE_DATA
 	select ARCH_HAS_DEBUG_VIRTUAL
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
@@ -34,6 +35,7 @@ config ARM64
 	select ARCH_HAS_SYSCALL_WRAPPER
 	select ARCH_HAS_TEARDOWN_DMA_OPS if IOMMU_SUPPORT
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
+	select ARCH_HAVE_ELF_PROT
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
 	select ARCH_INLINE_READ_LOCK if !PREEMPT
 	select ARCH_INLINE_READ_LOCK_BH if !PREEMPT
@@ -63,6 +65,7 @@ config ARM64
 	select ARCH_INLINE_SPIN_UNLOCK_IRQRESTORE if !PREEMPT
 	select ARCH_KEEP_MEMBLOCK
 	select ARCH_USE_CMPXCHG_LOCKREF
+	select ARCH_USE_GNU_PROPERTY if BINFMT_ELF
 	select ARCH_USE_QUEUED_RWLOCKS
 	select ARCH_USE_QUEUED_SPINLOCKS
 	select ARCH_SUPPORTS_MEMORY_FAILURE
diff --git a/arch/arm64/include/asm/elf.h b/arch/arm64/include/asm/elf.h
index b618017..8bc154c 100644
--- a/arch/arm64/include/asm/elf.h
+++ b/arch/arm64/include/asm/elf.h
@@ -114,7 +114,11 @@
 
 #ifndef __ASSEMBLY__
 
+#include <uapi/linux/elf.h>
 #include <linux/bug.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/types.h>
 #include <asm/processor.h> /* for signal_minsigstksz, used by ARCH_DLINFO */
 
 typedef unsigned long elf_greg_t;
@@ -224,6 +228,52 @@ extern int aarch32_setup_additional_pages(struct linux_binprm *bprm,
 
 #endif /* CONFIG_COMPAT */
 
+struct arch_elf_state {
+	int flags;
+};
+
+#define ARM64_ELF_BTI		(1 << 0)
+
+#define INIT_ARCH_ELF_STATE {			\
+	.flags = 0,				\
+}
+
+static inline int arch_parse_elf_property(u32 type, const void *data,
+					  size_t datasz, bool compat,
+					  struct arch_elf_state *arch)
+{
+	/* No known properties for AArch32 yet */
+	if (IS_ENABLED(CONFIG_COMPAT) && compat)
+		return 0;
+
+	if (type == GNU_PROPERTY_AARCH64_FEATURE_1_AND) {
+		const u32 *p = data;
+
+		if (datasz != sizeof(*p))
+			return -EIO;
+
+		if (IS_ENABLED(CONFIG_ARM64_BTI) &&
+		    (*p & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
+			arch->flags |= ARM64_ELF_BTI;
+	}
+
+	return 0;
+}
+
+static inline int arch_elf_pt_proc(void *ehdr, void *phdr,
+				   struct file *f, bool is_interp,
+				   struct arch_elf_state *state)
+{
+	return 0;
+}
+
+static inline int arch_check_elf(void *ehdr, bool has_interp,
+				 void *interp_ehdr,
+				 struct arch_elf_state *state)
+{
+	return 0;
+}
+
 #endif /* !__ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index a47462d..4c78937 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -11,12 +11,14 @@
 
 #include <linux/compat.h>
 #include <linux/efi.h>
+#include <linux/elf.h>
 #include <linux/export.h>
 #include <linux/sched.h>
 #include <linux/sched/debug.h>
 #include <linux/sched/task.h>
 #include <linux/sched/task_stack.h>
 #include <linux/kernel.h>
+#include <linux/mman.h>
 #include <linux/mm.h>
 #include <linux/stddef.h>
 #include <linux/sysctl.h>
@@ -633,3 +635,20 @@ static int __init tagged_addr_init(void)
 
 core_initcall(tagged_addr_init);
 #endif	/* CONFIG_ARM64_TAGGED_ADDR_ABI */
+
+#ifdef CONFIG_BINFMT_ELF
+int arch_elf_adjust_prot(int prot, const struct arch_elf_state *state,
+			 bool has_interp, bool is_interp)
+{
+	if (is_interp != has_interp)
+		return prot;
+
+	if (!(state->flags & ARM64_ELF_BTI))
+		return prot;
+
+	if (prot & PROT_EXEC)
+		prot |= PROT_BTI;
+
+	return prot;
+}
+#endif
diff --git a/include/linux/elf.h b/include/linux/elf.h
index 1b6e895..5d5b032 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -63,7 +63,11 @@ extern int elf_coredump_extra_notes_size(void);
 extern int elf_coredump_extra_notes_write(struct coredump_params *cprm);
 #endif
 
-/* NT_GNU_PROPERTY_TYPE_0 header */
+/*
+ * NT_GNU_PROPERTY_TYPE_0 header:
+ * Keep this internal until/unless there is an agreed UAPI definition.
+ * pr_type values (GNU_PROPERTY_*) are public and defined in the UAPI header.
+ */
 struct gnu_property {
 	u32 pr_type;
 	u32 pr_datasz;
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 20900f4..c6dd021 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -448,4 +448,10 @@ typedef struct elf64_note {
   Elf64_Word n_type;	/* Content type */
 } Elf64_Nhdr;
 
+/* .note.gnu.property types for EM_AARCH64: */
+#define GNU_PROPERTY_AARCH64_FEATURE_1_AND	0xc0000000
+
+/* Bits for GNU_PROPERTY_AARCH64_FEATURE_1_BTI */
+#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI	(1U << 0)
+
 #endif /* _UAPI_LINUX_ELF_H */
-- 
2.1.4

[PATCH v2 08/12] arm64: BTI: Decode BYTPE bits when printing PSTATE

[Dave Martin]

The current code to print PSTATE symbolically when generating
backtraces etc., does not include the BYTPE field used by Branch
Target Identification.

So, decode BYTPE and print it too.

In the interests of human-readability, print the classes of BTI
matched.  The symbolic notation, BYTPE (PSTATE[11:10]) and
permitted classes of subsequent instruction are:

    -- (BTYPE=0b00): any insn
    jc (BTYPE=0b01): BTI jc, BTI j, BTI c, PACIxSP
    -c (BYTPE=0b10): BTI jc, BTI c, PACIxSP
    j- (BTYPE=0b11): BTI jc, BTI j

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since v1:

 * Add convenience definitions for all the BTYPE codes, even if we
   don't directly use them all yet.

 * For consistency, align PSR_BTYPE_foo names with the above print
   format:

      PSR_BTYPE_NONE -> -- (BTYPE=0b00)
      PSR_BTYPE_JC -> jc (BTYPE=0b01)
      etc.
---
 arch/arm64/include/asm/ptrace.h |  7 ++++++-
 arch/arm64/kernel/process.c     | 17 +++++++++++++++--
 arch/arm64/kernel/signal.c      |  2 +-
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 7d4cd59..212bba1 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -38,7 +38,12 @@
 #define PSR_BTYPE_SHIFT		10
 
 #define PSR_IL_BIT		(1 << 20)
-#define PSR_BTYPE_CALL		(2 << PSR_BTYPE_SHIFT)
+
+/* Convenience names for the values of PSTATE.BTYPE */
+#define PSR_BTYPE_NONE		(0b00 << PSR_BTYPE_SHIFT)
+#define PSR_BTYPE_JC		(0b01 << PSR_BTYPE_SHIFT)
+#define PSR_BTYPE_C		(0b10 << PSR_BTYPE_SHIFT)
+#define PSR_BTYPE_J		(0b11 << PSR_BTYPE_SHIFT)
 
 /* AArch32-specific ptrace requests */
 #define COMPAT_PTRACE_GETREGS		12
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 4c78937..a2b555a 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -209,6 +209,15 @@ void machine_restart(char *cmd)
 	while (1);
 }
 
+#define bstr(suffix, str) [PSR_BTYPE_ ## suffix >> PSR_BTYPE_SHIFT] = str
+static const char *const btypes[] = {
+	bstr(NONE, "--"),
+	bstr(  JC, "jc"),
+	bstr(   C, "-c"),
+	bstr(  J , "j-")
+};
+#undef bstr
+
 static void print_pstate(struct pt_regs *regs)
 {
 	u64 pstate = regs->pstate;
@@ -227,7 +236,10 @@ static void print_pstate(struct pt_regs *regs)
 			pstate & PSR_AA32_I_BIT ? 'I' : 'i',
 			pstate & PSR_AA32_F_BIT ? 'F' : 'f');
 	} else {
-		printk("pstate: %08llx (%c%c%c%c %c%c%c%c %cPAN %cUAO)\n",
+		const char *btype_str = btypes[(pstate & PSR_BTYPE_MASK) >>
+					       PSR_BTYPE_SHIFT];
+
+		printk("pstate: %08llx (%c%c%c%c %c%c%c%c %cPAN %cUAO BTYPE=%s)\n",
 			pstate,
 			pstate & PSR_N_BIT ? 'N' : 'n',
 			pstate & PSR_Z_BIT ? 'Z' : 'z',
@@ -238,7 +250,8 @@ static void print_pstate(struct pt_regs *regs)
 			pstate & PSR_I_BIT ? 'I' : 'i',
 			pstate & PSR_F_BIT ? 'F' : 'f',
 			pstate & PSR_PAN_BIT ? '+' : '-',
-			pstate & PSR_UAO_BIT ? '+' : '-');
+			pstate & PSR_UAO_BIT ? '+' : '-',
+			btype_str);
 	}
 }
 
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 4a3bd32..452ac5b 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -732,7 +732,7 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
 
 	if (system_supports_bti()) {
 		regs->pstate &= ~PSR_BTYPE_MASK;
-		regs->pstate |= PSR_BTYPE_CALL;
+		regs->pstate |= PSR_BTYPE_C;
 	}
 
 	if (ka->sa.sa_flags & SA_RESTORER)
-- 
2.1.4

[PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

[Dave Martin]

Correct skipping of an instruction on AArch32 works a bit
differently from AArch64, mainly due to the different CPSR/PSTATE
semantics.

There have been various attempts to get this right.  Currenty
arm64_skip_faulting_instruction() mostly does the right thing, but
does not advance the IT state machine for the AArch32 case.

arm64_compat_skip_faulting_instruction() handles the IT state
machine but is local to traps.c, and porting other code to use it
will make a mess since there are some call sites that apply for
both the compat and native cases.

Since manual instruction skipping implies a trap, it's a relatively
slow path.

So, make arm64_skip_faulting_instruction() handle both compat and
native, and get rid of the arm64_compat_skip_faulting_instruction()
special case.

Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler")
Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance")
Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps")
Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/traps.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 15e3c4f..44c91d4 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -268,6 +268,8 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 	}
 }
 
+static void advance_itstate(struct pt_regs *regs);
+
 void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
 {
 	regs->pc += size;
@@ -278,6 +280,9 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
 	 */
 	if (user_mode(regs))
 		user_fastforward_single_step(current);
+
+	if (regs->pstate & PSR_MODE32_BIT)
+		advance_itstate(regs);
 }
 
 static LIST_HEAD(undef_hook);
@@ -629,19 +634,12 @@ static void advance_itstate(struct pt_regs *regs)
 	compat_set_it_state(regs, it);
 }
 
-static void arm64_compat_skip_faulting_instruction(struct pt_regs *regs,
-						   unsigned int sz)
-{
-	advance_itstate(regs);
-	arm64_skip_faulting_instruction(regs, sz);
-}
-
 static void compat_cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
 {
 	int reg = (esr & ESR_ELx_CP15_32_ISS_RT_MASK) >> ESR_ELx_CP15_32_ISS_RT_SHIFT;
 
 	pt_regs_write_reg(regs, reg, arch_timer_get_rate());
-	arm64_compat_skip_faulting_instruction(regs, 4);
+	arm64_skip_faulting_instruction(regs, 4);
 }
 
 static const struct sys64_hook cp15_32_hooks[] = {
@@ -661,7 +659,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
 
 	pt_regs_write_reg(regs, rt, lower_32_bits(val));
 	pt_regs_write_reg(regs, rt2, upper_32_bits(val));
-	arm64_compat_skip_faulting_instruction(regs, 4);
+	arm64_skip_faulting_instruction(regs, 4);
 }
 
 static const struct sys64_hook cp15_64_hooks[] = {
@@ -682,7 +680,7 @@ asmlinkage void __exception do_cp15instr(unsigned int esr, struct pt_regs *regs)
 		 * There is no T16 variant of a CP access, so we
 		 * always advance PC by 4 bytes.
 		 */
-		arm64_compat_skip_faulting_instruction(regs, 4);
+		arm64_skip_faulting_instruction(regs, 4);
 		return;
 	}
 
-- 
2.1.4

[PATCH v2 10/12] arm64: traps: Shuffle code to eliminate forward declarations

[Dave Martin]

Hoist the IT state handling code earlier in traps.c, to avoid
accumulating forward declarations.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/traps.c | 101 ++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 52 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 44c91d4..3af2768 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -268,7 +268,55 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 	}
 }
 
-static void advance_itstate(struct pt_regs *regs);
+#ifdef CONFIG_COMPAT
+#define PSTATE_IT_1_0_SHIFT	25
+#define PSTATE_IT_1_0_MASK	(0x3 << PSTATE_IT_1_0_SHIFT)
+#define PSTATE_IT_7_2_SHIFT	10
+#define PSTATE_IT_7_2_MASK	(0x3f << PSTATE_IT_7_2_SHIFT)
+
+static u32 compat_get_it_state(struct pt_regs *regs)
+{
+	u32 it, pstate = regs->pstate;
+
+	it  = (pstate & PSTATE_IT_1_0_MASK) >> PSTATE_IT_1_0_SHIFT;
+	it |= ((pstate & PSTATE_IT_7_2_MASK) >> PSTATE_IT_7_2_SHIFT) << 2;
+
+	return it;
+}
+
+static void compat_set_it_state(struct pt_regs *regs, u32 it)
+{
+	u32 pstate_it;
+
+	pstate_it  = (it << PSTATE_IT_1_0_SHIFT) & PSTATE_IT_1_0_MASK;
+	pstate_it |= ((it >> 2) << PSTATE_IT_7_2_SHIFT) & PSTATE_IT_7_2_MASK;
+
+	regs->pstate &= ~PSR_AA32_IT_MASK;
+	regs->pstate |= pstate_it;
+}
+
+static void advance_itstate(struct pt_regs *regs)
+{
+	u32 it;
+
+	/* ARM mode */
+	if (!(regs->pstate & PSR_AA32_T_BIT) ||
+	    !(regs->pstate & PSR_AA32_IT_MASK))
+		return;
+
+	it  = compat_get_it_state(regs);
+
+	/*
+	 * If this is the last instruction of the block, wipe the IT
+	 * state. Otherwise advance it.
+	 */
+	if (!(it & 7))
+		it = 0;
+	else
+		it = (it & 0xe0) | ((it << 1) & 0x1f);
+
+	compat_set_it_state(regs, it);
+}
 
 void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
 {
@@ -563,34 +611,6 @@ static const struct sys64_hook sys64_hooks[] = {
 	{},
 };
 
-
-#ifdef CONFIG_COMPAT
-#define PSTATE_IT_1_0_SHIFT	25
-#define PSTATE_IT_1_0_MASK	(0x3 << PSTATE_IT_1_0_SHIFT)
-#define PSTATE_IT_7_2_SHIFT	10
-#define PSTATE_IT_7_2_MASK	(0x3f << PSTATE_IT_7_2_SHIFT)
-
-static u32 compat_get_it_state(struct pt_regs *regs)
-{
-	u32 it, pstate = regs->pstate;
-
-	it  = (pstate & PSTATE_IT_1_0_MASK) >> PSTATE_IT_1_0_SHIFT;
-	it |= ((pstate & PSTATE_IT_7_2_MASK) >> PSTATE_IT_7_2_SHIFT) << 2;
-
-	return it;
-}
-
-static void compat_set_it_state(struct pt_regs *regs, u32 it)
-{
-	u32 pstate_it;
-
-	pstate_it  = (it << PSTATE_IT_1_0_SHIFT) & PSTATE_IT_1_0_MASK;
-	pstate_it |= ((it >> 2) << PSTATE_IT_7_2_SHIFT) & PSTATE_IT_7_2_MASK;
-
-	regs->pstate &= ~PSR_AA32_IT_MASK;
-	regs->pstate |= pstate_it;
-}
-
 static bool cp15_cond_valid(unsigned int esr, struct pt_regs *regs)
 {
 	int cond;
@@ -611,29 +631,6 @@ static bool cp15_cond_valid(unsigned int esr, struct pt_regs *regs)
 	return aarch32_opcode_cond_checks[cond](regs->pstate);
 }
 
-static void advance_itstate(struct pt_regs *regs)
-{
-	u32 it;
-
-	/* ARM mode */
-	if (!(regs->pstate & PSR_AA32_T_BIT) ||
-	    !(regs->pstate & PSR_AA32_IT_MASK))
-		return;
-
-	it  = compat_get_it_state(regs);
-
-	/*
-	 * If this is the last instruction of the block, wipe the IT
-	 * state. Otherwise advance it.
-	 */
-	if (!(it & 7))
-		it = 0;
-	else
-		it = (it & 0xe0) | ((it << 1) & 0x1f);
-
-	compat_set_it_state(regs, it);
-}
-
 static void compat_cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
 {
 	int reg = (esr & ESR_ELx_CP15_32_ISS_RT_MASK) >> ESR_ELx_CP15_32_ISS_RT_SHIFT;
-- 
2.1.4

[PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions

[Dave Martin]

Since normal execution of any non-branch instruction resets the
PSTATE BTYPE field to 0, so do the same thing when emulating a
trapped instruction.

Branches don't trap directly, so we should never need to assign a
non-zero value to BTYPE here.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/traps.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 3af2768..4d8ce50 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
 
 	if (regs->pstate & PSR_MODE32_BIT)
 		advance_itstate(regs);
+	else
+		regs->pstate &= ~(u64)PSR_BTYPE_MASK;
 }
 
 static LIST_HEAD(undef_hook);
-- 
2.1.4

[PATCH v2 12/12] KVM: arm64: BTI: Reset BTYPE when skipping emulated instructions

[Dave Martin]

Since normal execution of any non-branch instruction resets the
PSTATE BTYPE field to 0, so do the same thing when emulating a
trapped instruction.

Branches don't trap directly, so we should never need to assign a
non-zero value to BTYPE here.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/kvm_emulate.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index d69c1ef..33957a12 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -452,8 +452,10 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
 {
 	if (vcpu_mode_is_32bit(vcpu))
 		kvm_skip_instr32(vcpu, is_wide_instr);
-	else
+	else {
 		*vcpu_pc(vcpu) += 4;
+		*vcpu_cpsr(vcpu) &= ~(u64)PSR_BTYPE_MASK;
+	}
 
 	/* advance the singlestep state machine */
 	*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
-- 
2.1.4

Re: [PATCH v2 04/12] arm64: docs: cpu-feature-registers: Document ID_AA64PFR1_EL1

[Alex Bennée]

Dave Martin <Dave.Martin@arm.com> writes:

> Commit d71be2b6c0e1 ("arm64: cpufeature: Detect SSBS and advertise
> to userspace") exposes ID_AA64PFR1_EL1 to userspace, but didn't
> update the documentation to match.
>
> Add it.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>
> ---
>
> Note to maintainers:
>
>  * This patch has been racing with various other attempts to fix
>    the same documentation in the meantime.
>
>    Since this patch only fixes the documenting for pre-existing
>    features, it can safely be dropped if appropriate.
>
>    The _new_ documentation relating to BTI feature reporting
>    is in a subsequent patch, and needs to be retained.
> ---
>  Documentation/arm64/cpu-feature-registers.rst | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst
> index 2955287..b86828f 100644
> --- a/Documentation/arm64/cpu-feature-registers.rst
> +++ b/Documentation/arm64/cpu-feature-registers.rst
> @@ -168,8 +168,15 @@ infrastructure:
>       +------------------------------+---------+---------+
>
>
> -  3) MIDR_EL1 - Main ID Register
> +  3) ID_AA64PFR1_EL1 - Processor Feature Register 1
> +     +------------------------------+---------+---------+
> +     | Name                         |  bits   | visible |
> +     +------------------------------+---------+---------+
> +     | SSBS                         | [7-4]   |    y    |
> +     +------------------------------+---------+---------+
> +
>
> +  4) MIDR_EL1 - Main ID Register
>       +------------------------------+---------+---------+
>       | Name                         |  bits   | visible |
>       +------------------------------+---------+---------+
> @@ -188,7 +195,7 @@ infrastructure:
>     as available on the CPU where it is fetched and is not a system
>     wide safe value.
>
> -  4) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
> +  5) ID_AA64ISAR1_EL1 - Instruction set attribute register 1

If I'm not mistaken .rst has support for auto-enumeration if the #
character is used. That might reduce the pain of re-numbering in future.

>
>       +------------------------------+---------+---------+
>       | Name                         |  bits   | visible |
> @@ -210,7 +217,7 @@ infrastructure:
>       | DPB                          | [3-0]   |    y    |
>       +------------------------------+---------+---------+
>
> -  5) ID_AA64MMFR2_EL1 - Memory model feature register 2
> +  6) ID_AA64MMFR2_EL1 - Memory model feature register 2
>
>       +------------------------------+---------+---------+
>       | Name                         |  bits   | visible |
> @@ -218,7 +225,7 @@ infrastructure:
>       | AT                           | [35-32] |    y    |
>       +------------------------------+---------+---------+
>
> -  6) ID_AA64ZFR0_EL1 - SVE feature ID register 0
> +  7) ID_AA64ZFR0_EL1 - SVE feature ID register 0
>
>       +------------------------------+---------+---------+
>       | Name                         |  bits   | visible |


--
Alex Bennée

Re: [PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions

[Mark Rutland]

On Thu, Oct 10, 2019 at 07:44:39PM +0100, Dave Martin wrote:
> Since normal execution of any non-branch instruction resets the
> PSTATE BTYPE field to 0, so do the same thing when emulating a
> trapped instruction.
> 
> Branches don't trap directly, so we should never need to assign a
> non-zero value to BTYPE here.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kernel/traps.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 3af2768..4d8ce50 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
>  
>  	if (regs->pstate & PSR_MODE32_BIT)
>  		advance_itstate(regs);
> +	else
> +		regs->pstate &= ~(u64)PSR_BTYPE_MASK;

This looks good to me, with one nit below.

We don't (currently) need the u64 cast here, and it's inconsistent with
what we do elsewhere. If the upper 32-bit of pstate get allocated, we'll
need to fix up all the other masking we do:

[mark@lakrids:~/src/linux]% git grep 'pstate &= ~'
arch/arm64/kernel/armv8_deprecated.c:           regs->pstate &= ~PSR_AA32_E_BIT;
arch/arm64/kernel/cpufeature.c:         regs->pstate &= ~PSR_SSBS_BIT;
arch/arm64/kernel/debug-monitors.c:     regs->pstate &= ~DBG_SPSR_SS;
arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~PSR_D_BIT;
arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~DAIF_MASK;
arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS;
arch/arm64/kernel/ptrace.c:                     regs->pstate &= ~PSR_AA32_E_BIT;
arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS;
arch/arm64/kernel/ptrace.c:             regs->pstate &= ~DBG_SPSR_SS;
arch/arm64/kernel/ssbd.c:       task_pt_regs(task)->pstate &= ~val;
arch/arm64/kernel/traps.c:      regs->pstate &= ~PSR_AA32_IT_MASK;

... and at that point I'd suggest we should just ensure the bit
definitions are all defined as unsigned long in the first place since
adding casts to each use is error-prone.

Thanks,
Mark.

Re: [PATCH v2 12/12] KVM: arm64: BTI: Reset BTYPE when skipping emulated instructions

[Mark Rutland]

On Thu, Oct 10, 2019 at 07:44:40PM +0100, Dave Martin wrote:
> Since normal execution of any non-branch instruction resets the
> PSTATE BTYPE field to 0, so do the same thing when emulating a
> trapped instruction.
> 
> Branches don't trap directly, so we should never need to assign a
> non-zero value to BTYPE here.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index d69c1ef..33957a12 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -452,8 +452,10 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
>  {
>  	if (vcpu_mode_is_32bit(vcpu))
>  		kvm_skip_instr32(vcpu, is_wide_instr);
> -	else
> +	else {
>  		*vcpu_pc(vcpu) += 4;
> +		*vcpu_cpsr(vcpu) &= ~(u64)PSR_BTYPE_MASK;
> +	}

Style nit: both sides of an if-else should match brace-wise. i.e. please
add braces to the other side.

As with the prior patch, the u64 cast can also go.

Otherwise, this looks right to me.

Thanks,
Mark.

Re: [PATCH v2 12/12] KVM: arm64: BTI: Reset BTYPE when skipping emulated instructions

[Dave Martin]

On Fri, Oct 11, 2019 at 03:24:55PM +0100, Mark Rutland wrote:
> On Thu, Oct 10, 2019 at 07:44:40PM +0100, Dave Martin wrote:
> > Since normal execution of any non-branch instruction resets the
> > PSTATE BTYPE field to 0, so do the same thing when emulating a
> > trapped instruction.
> > 
> > Branches don't trap directly, so we should never need to assign a
> > non-zero value to BTYPE here.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_emulate.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > index d69c1ef..33957a12 100644
> > --- a/arch/arm64/include/asm/kvm_emulate.h
> > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > @@ -452,8 +452,10 @@ static inline void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr)
> >  {
> >  	if (vcpu_mode_is_32bit(vcpu))
> >  		kvm_skip_instr32(vcpu, is_wide_instr);
> > -	else
> > +	else {
> >  		*vcpu_pc(vcpu) += 4;
> > +		*vcpu_cpsr(vcpu) &= ~(u64)PSR_BTYPE_MASK;
> > +	}
> 
> Style nit: both sides of an if-else should match brace-wise. i.e. please
> add braces to the other side.

Will fix.  Strange, checkpatch didn't catch that, maybe because only one
arm of the if was patched.

> As with the prior patch, the u64 cast can also go.
> 
> Otherwise, this looks right to me.

For some reason I thought there was a different prevailing style in the
KVM code, but now I see no evidence of that.

Will fix.

Thanks for the review.

Cheers
---Dave

Re: [PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions

[Dave Martin]

On Fri, Oct 11, 2019 at 03:21:58PM +0100, Mark Rutland wrote:
> On Thu, Oct 10, 2019 at 07:44:39PM +0100, Dave Martin wrote:
> > Since normal execution of any non-branch instruction resets the
> > PSTATE BTYPE field to 0, so do the same thing when emulating a
> > trapped instruction.
> > 
> > Branches don't trap directly, so we should never need to assign a
> > non-zero value to BTYPE here.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/kernel/traps.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index 3af2768..4d8ce50 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
> >  
> >  	if (regs->pstate & PSR_MODE32_BIT)
> >  		advance_itstate(regs);
> > +	else
> > +		regs->pstate &= ~(u64)PSR_BTYPE_MASK;
> 
> This looks good to me, with one nit below.
> 
> We don't (currently) need the u64 cast here, and it's inconsistent with
> what we do elsewhere. If the upper 32-bit of pstate get allocated, we'll
> need to fix up all the other masking we do:

Huh, looks like I missed that.  Dang.  Will fix.

> [mark@lakrids:~/src/linux]% git grep 'pstate &= ~'
> arch/arm64/kernel/armv8_deprecated.c:           regs->pstate &= ~PSR_AA32_E_BIT;
> arch/arm64/kernel/cpufeature.c:         regs->pstate &= ~PSR_SSBS_BIT;
> arch/arm64/kernel/debug-monitors.c:     regs->pstate &= ~DBG_SPSR_SS;
> arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
> arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
> arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~PSR_D_BIT;
> arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~DAIF_MASK;
> arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS;
> arch/arm64/kernel/ptrace.c:                     regs->pstate &= ~PSR_AA32_E_BIT;
> arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS;
> arch/arm64/kernel/ptrace.c:             regs->pstate &= ~DBG_SPSR_SS;
> arch/arm64/kernel/ssbd.c:       task_pt_regs(task)->pstate &= ~val;
> arch/arm64/kernel/traps.c:      regs->pstate &= ~PSR_AA32_IT_MASK;
> 
> ... and at that point I'd suggest we should just ensure the bit
> definitions are all defined as unsigned long in the first place since
> adding casts to each use is error-prone.

Are we concerned about changing the types of UAPI #defines?  That can
cause subtle and unexpected breakage, especially when the signedness
of a #define changes.

Ideally, we'd just change all these to 1UL << n.

Cheers
---Dave

Re: [PATCH v2 04/12] arm64: docs: cpu-feature-registers: Document ID_AA64PFR1_EL1

[Dave Martin]

On Fri, Oct 11, 2019 at 02:19:48PM +0100, Alex Bennée wrote:
> 
> Dave Martin <Dave.Martin@arm.com> writes:
> 
> > Commit d71be2b6c0e1 ("arm64: cpufeature: Detect SSBS and advertise
> > to userspace") exposes ID_AA64PFR1_EL1 to userspace, but didn't
> > update the documentation to match.
> >
> > Add it.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> >
> > ---
> >
> > Note to maintainers:
> >
> >  * This patch has been racing with various other attempts to fix
> >    the same documentation in the meantime.
> >
> >    Since this patch only fixes the documenting for pre-existing
> >    features, it can safely be dropped if appropriate.
> >
> >    The _new_ documentation relating to BTI feature reporting
> >    is in a subsequent patch, and needs to be retained.
> > ---
> >  Documentation/arm64/cpu-feature-registers.rst | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/arm64/cpu-feature-registers.rst b/Documentation/arm64/cpu-feature-registers.rst
> > index 2955287..b86828f 100644
> > --- a/Documentation/arm64/cpu-feature-registers.rst
> > +++ b/Documentation/arm64/cpu-feature-registers.rst
> > @@ -168,8 +168,15 @@ infrastructure:
> >       +------------------------------+---------+---------+
> >
> >
> > -  3) MIDR_EL1 - Main ID Register
> > +  3) ID_AA64PFR1_EL1 - Processor Feature Register 1
> > +     +------------------------------+---------+---------+
> > +     | Name                         |  bits   | visible |
> > +     +------------------------------+---------+---------+
> > +     | SSBS                         | [7-4]   |    y    |
> > +     +------------------------------+---------+---------+
> > +
> >
> > +  4) MIDR_EL1 - Main ID Register
> >       +------------------------------+---------+---------+
> >       | Name                         |  bits   | visible |
> >       +------------------------------+---------+---------+
> > @@ -188,7 +195,7 @@ infrastructure:
> >     as available on the CPU where it is fetched and is not a system
> >     wide safe value.
> >
> > -  4) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
> > +  5) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
> 
> If I'm not mistaken .rst has support for auto-enumeration if the #
> character is used. That might reduce the pain of re-numbering in future.

Ack, though it would be good to go one better and generate this document
from the cpufeature.c tables (or from some common source).  The numbers
are relatively easy to maintain -- remembering to update the document
at all seems the bigger maintenance headache right now.

I think this particular patch is superseded by similar fixes from other
people, just not in torvalds/master yet.

[...]

Cheers
---Dave

[FIXUP 0/2] Fixups to patch 5

[Dave Martin]

Here are a couple of late-breaking fixups, which may be integrated in
the next posting.

The first is trivial; the second needs discussion.


Dave Martin (2):
  squash! arm64: Basic Branch Target Identification support
  squash! arm64: Basic Branch Target Identification support

 arch/arm64/Kconfig | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.1.4

[FIXUP 1/2] squash! arm64: Basic Branch Target Identification support

[Dave Martin]

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since v2:

 * Fix Kconfig typo that claimed that Pointer authentication is part of
   ARMv8.2.  It's v8.3.
---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 563dec5..6e26b72 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1425,7 +1425,7 @@ config ARM64_BTI
 
 	  This is intended to provide complementary protection to other control
 	  flow integrity protection mechanisms, such as the Pointer
-	  authentication mechanism provided as part of the ARMv8.2 Extensions.
+	  authentication mechanism provided as part of the ARMv8.3 Extensions.
 
 	  To make use of BTI on CPUs that support it, say Y.
 
-- 
2.1.4

[FIXUP 2/2] squash! arm64: Basic Branch Target Identification support

[Dave Martin]

[Add Kconfig dependency on CONFIG_ARM64_PTR_AUTH]

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

This one could use some discussion.

Two conforming hardware implementations containing BTI could nonetheless
have incompatible Pointer auth implementations, meaning that we expose
BTI to userspace but not Pointer auth.

That's stupid hardware design, but the architecture doesn't forbid it
today.  We _could_ detect this and hide BTI from userspace too, but
if a big.LITTLE system contains Pointer auth implementations with
mismatched IMP DEF algorithms, we lose -- we have no direct way to
detect that.

Since BTI still provides some limited value without Pointer auth,
disabling it unnecessarily might be regarded as too heavy-handed.

Changes since v2:

 * Depend on CONFIG_ARM64_PTR_AUTH=y.

   During test hacking, I observed that there are situations where
   userspace should be entitled to assume that Pointer auth is present
   if BTI is present.

   Although the kernel BTI support doesn't require any aspect of
   Pointer authentication, there are architectural dependencies:

    * ARMv8.5 requires BTI to be implemented. [1]
    * BTI requires ARMv8.4-A to be implemented. [1], [2]
    * ARMv8.4 requires ARMv8.3 to be implemented. [3]
    * ARMv8.3 requires Pointer authentication to be implemented. [4]

   i.e., an implementation that supports BTI but not Pointer auth is
   broken.

   BTI is also designed to be complementary to Pointer authentication:
   without Pointer auth, BTI would offer no protection for function
   returns, seriously undermining the value of the feature.

   See ARM ARM for ARMv8-A (ARM DDI 0487E.a) Sections:

   [1] A2.8.1, "Architectural features added by ARMv8.5"

   [2] A2.2.1, "Permitted implementation of subsets of ARMv8.x and
       ARMv8.(x+1) architectural features"

   [3] A2.6.1, "Architectural features added by Armv8.3"

   [4] A2.6, "The Armv8.3 architecture extension"
---
 arch/arm64/Kconfig | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6e26b72..a64d91d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1418,16 +1418,21 @@ menu "ARMv8.5 architectural features"
 config ARM64_BTI
 	bool "Branch Target Identification support"
 	default y
+	depends on ARM64_PTR_AUTH
 	help
 	  Branch Target Identification (part of the ARMv8.5 Extensions)
 	  provides a mechanism to limit the set of locations to which computed
 	  branch instructions such as BR or BLR can jump.
 
-	  This is intended to provide complementary protection to other control
+	  To make use of BTI on CPUs that support it, say Y.
+
+	  BTI is intended to provide complementary protection to other control
 	  flow integrity protection mechanisms, such as the Pointer
 	  authentication mechanism provided as part of the ARMv8.3 Extensions.
+	  For this reason, it does not make sense to enable this option without
+	  also enabling support for Pointer authentication.
 
-	  To make use of BTI on CPUs that support it, say Y.
+	  Thus, to enable this option you also need to select ARM64_PTR_AUTH=y.
 
 	  Userspace binaries must also be specifically compiled to make use of
 	  this mechanism.  If you say N here or the hardware does not support
-- 
2.1.4

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Mark Rutland]

On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> This patch adds the bare minimum required to expose the ARMv8.5
> Branch Target Identification feature to userspace.
> 
> By itself, this does _not_ automatically enable BTI for any initial
> executable pages mapped by execve().  This will come later, but for
> now it should be possible to enable BTI manually on those pages by
> using mprotect() from within the target process.
> 
> Other arches already using the generic mman.h are already using
> 0x10 for arch-specific prot flags, so we use that for PROT_BTI
> here.
> 
> For consistency, signal handler entry points in BTI guarded pages
> are required to be annotated as such, just like any other function.
> This blocks a relatively minor attack vector, but comforming
> userspace will have the annotations anyway, so we may as well
> enforce them.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> ---
> 
> Changes since v1:
> 
>  * Configure SCTLR_EL1.BTx to disallow BR onto a PACIxSP instruction
>    (except via X16/X17):
> 
>    The AArch64 procedure call standard requires binaries marked with
>    GNU_PROPERTY_AARCH64_FEATURE_1_BTI to use X16/X17 in trampolines
>    and tail calls, so it makes no sense to be permissive.
> 
>  * Rename PROT_BTI_GUARDED to PROT_BTI.
> 
>  * Rename VM_ARM64_GP to VM_ARM64_BTI:
> 
>    Although the architectural name for the BTI page table bit is "GP",
>    BTI is nonetheless the feature it controls.  So avoid introducing
>    the "GP" naming just for this -- it's just an unecessary extra
>    source of confusion.
> 
>  * Tidy up masking with ~PSR_BTYPE_MASK.
> 
>  * Drop masking out of BTYPE on SVC, with a comment outlining why.
> 
>  * Split PSR_BTYPE_SHIFT definition into this patch.  It's not
>    useful yet, but it makes sense to define PSR_BTYPE_* using this
>    from the outset.
> 
>  * Migrate to ct_user_exit_irqoff in entry.S:el0_bti.

[...]

> diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> new file mode 100644
> index 0000000..cbfe3238
> --- /dev/null
> +++ b/arch/arm64/include/asm/mman.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_MMAN_H__
> +#define __ASM_MMAN_H__
> +
> +#include <uapi/asm/mman.h>
> +
> +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> +{
> +	if (system_supports_bti() && (prot & PROT_BTI))
> +		return VM_ARM64_BTI;
> +
> +	return 0;
> +}

Can we call this arch_calc_vm_prot_bits() directly, with all the
arguments:

static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
						   unsigned long pkey)
{
	...
}
#define arch_calc_vm_prot_bits arch_calc_vm_prot_bits

... as that makes it a bit easier to match definition with use, and just
definign the name makes it a bit clearer that that's probably for the
benefit of some ifdeffery.

Likewise for the other functions here.

> +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> +{
> +	return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
> +}
> +
> +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
> +{
> +	unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> +
> +	if (system_supports_bti())
> +		supported |= PROT_BTI;
> +
> +	return (prot & ~supported) == 0;
> +}

If we have this check, can we ever get into arm64_calc_vm_prot_bits()
with PROT_BIT but !system_supports_bti()?

... or can that become:

	return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;

> +#endif /* ! __ASM_MMAN_H__ */
> diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
> index 3df60f9..f85d1fc 100644
> --- a/arch/arm64/include/asm/pgtable-hwdef.h
> +++ b/arch/arm64/include/asm/pgtable-hwdef.h
> @@ -150,6 +150,7 @@
>  #define PTE_SHARED		(_AT(pteval_t, 3) << 8)		/* SH[1:0], inner shareable */
>  #define PTE_AF			(_AT(pteval_t, 1) << 10)	/* Access Flag */
>  #define PTE_NG			(_AT(pteval_t, 1) << 11)	/* nG */
> +#define PTE_GP			(_AT(pteval_t, 1) << 50)	/* BTI guarded */

As a heads-up for anyone looking at the latest ARM ARM (ARM DDI
0487E.a), GP is missing from some of the descriptions of the table
formats in section D5.3.1 in the latest ARM ARM (ARM DDI 0487E.a), which
imply it's RES0.

It looks like that'll be fixed for the next release.

[...]

> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 84a8227..6c5adea 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -737,6 +737,8 @@ el0_sync:
>  	b.eq	el0_pc
>  	cmp	x24, #ESR_ELx_EC_UNKNOWN	// unknown exception in EL0
>  	b.eq	el0_undef
> +	cmp	x24, #ESR_ELx_EC_BTI		// branch target exception
> +	b.eq	el0_bti
>  	cmp	x24, #ESR_ELx_EC_BREAKPT_LOW	// debug exception in EL0
>  	b.ge	el0_dbg
>  	b	el0_inv
> @@ -887,6 +889,15 @@ el0_undef:
>  	mov	x0, sp
>  	bl	do_undefinstr
>  	b	ret_to_user
> +el0_bti:
> +	/*
> +	 * Branch target exception
> +	 */
> +	ct_user_exit_irqoff
> +	enable_daif
> +	mov	x0, sp
> +	bl	do_bti
> +	b	ret_to_user

As a heads-up, this'll conflict with James's conversion of the sync
entry points to C.

> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index dd2cdc0..4a3bd32 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -730,6 +730,11 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
>  	regs->pc = (unsigned long)ka->sa.sa_handler;
>  
> +	if (system_supports_bti()) {
> +		regs->pstate &= ~PSR_BTYPE_MASK;
> +		regs->pstate |= PSR_BTYPE_CALL;
> +	}
> +

I think we might need a comment as to what we're trying to ensure here.

I was under the (perhaps mistaken) impression that we'd generate a
pristine pstate for a signal handler, and it's not clear to me that we
must ensure the first instruction is a target instruction.

[...]

> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 34739e8..15e3c4f 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -406,6 +406,12 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>  	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc);
>  }
>  
> +asmlinkage void __exception do_bti(struct pt_regs *regs)
> +{
> +	BUG_ON(!user_mode(regs));
> +	force_signal_inject(SIGILL, ILL_ILLOPC, regs->pc);
> +}

This was only wired up into the EL0 sync entry paths, so I think we can
drop the BUG_ON() -- we don't have similar in other EL0-only paths.

Thanks,
Mark.

Re: [PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

[Mark Rutland]

On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote:
> Correct skipping of an instruction on AArch32 works a bit
> differently from AArch64, mainly due to the different CPSR/PSTATE
> semantics.
> 
> There have been various attempts to get this right.  Currenty
> arm64_skip_faulting_instruction() mostly does the right thing, but
> does not advance the IT state machine for the AArch32 case.
> 
> arm64_compat_skip_faulting_instruction() handles the IT state
> machine but is local to traps.c, and porting other code to use it
> will make a mess since there are some call sites that apply for
> both the compat and native cases.
> 
> Since manual instruction skipping implies a trap, it's a relatively
> slow path.
> 
> So, make arm64_skip_faulting_instruction() handle both compat and
> native, and get rid of the arm64_compat_skip_faulting_instruction()
> special case.
> 
> Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler")
> Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance")
> Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps")
> Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kernel/traps.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

This looks good to me; it's certainly easier to reason about.

I couldn't spot a place where we do the wrong thing today, given AFAICT
all the instances in arch/arm64/kernel/armv8_deprecated.c would be
UNPREDICTABLE within an IT block.

It might be worth calling out an example in the commit message to
justify the fixes tags.

Thanks,
Mark.

> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 15e3c4f..44c91d4 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -268,6 +268,8 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
>  	}
>  }
>  
> +static void advance_itstate(struct pt_regs *regs);
> +
>  void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
>  {
>  	regs->pc += size;
> @@ -278,6 +280,9 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
>  	 */
>  	if (user_mode(regs))
>  		user_fastforward_single_step(current);
> +
> +	if (regs->pstate & PSR_MODE32_BIT)
> +		advance_itstate(regs);
>  }
>  
>  static LIST_HEAD(undef_hook);
> @@ -629,19 +634,12 @@ static void advance_itstate(struct pt_regs *regs)
>  	compat_set_it_state(regs, it);
>  }
>  
> -static void arm64_compat_skip_faulting_instruction(struct pt_regs *regs,
> -						   unsigned int sz)
> -{
> -	advance_itstate(regs);
> -	arm64_skip_faulting_instruction(regs, sz);
> -}
> -
>  static void compat_cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
>  {
>  	int reg = (esr & ESR_ELx_CP15_32_ISS_RT_MASK) >> ESR_ELx_CP15_32_ISS_RT_SHIFT;
>  
>  	pt_regs_write_reg(regs, reg, arch_timer_get_rate());
> -	arm64_compat_skip_faulting_instruction(regs, 4);
> +	arm64_skip_faulting_instruction(regs, 4);
>  }
>  
>  static const struct sys64_hook cp15_32_hooks[] = {
> @@ -661,7 +659,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
>  
>  	pt_regs_write_reg(regs, rt, lower_32_bits(val));
>  	pt_regs_write_reg(regs, rt2, upper_32_bits(val));
> -	arm64_compat_skip_faulting_instruction(regs, 4);
> +	arm64_skip_faulting_instruction(regs, 4);
>  }
>  
>  static const struct sys64_hook cp15_64_hooks[] = {
> @@ -682,7 +680,7 @@ asmlinkage void __exception do_cp15instr(unsigned int esr, struct pt_regs *regs)
>  		 * There is no T16 variant of a CP access, so we
>  		 * always advance PC by 4 bytes.
>  		 */
> -		arm64_compat_skip_faulting_instruction(regs, 4);
> +		arm64_skip_faulting_instruction(regs, 4);
>  		return;
>  	}
>  
> -- 
> 2.1.4
> 

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Richard Henderson]

On 10/11/19 11:10 AM, Mark Rutland wrote:
> On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
>> @@ -730,6 +730,11 @@ static void setup_return
>>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
>>  	regs->pc = (unsigned long)ka->sa.sa_handler;
>>  
>> +	if (system_supports_bti()) {
>> +		regs->pstate &= ~PSR_BTYPE_MASK;
>> +		regs->pstate |= PSR_BTYPE_CALL;
>> +	}
>> +
> 
> I think we might need a comment as to what we're trying to ensure here.
> 
> I was under the (perhaps mistaken) impression that we'd generate a
> pristine pstate for a signal handler, and it's not clear to me that we
> must ensure the first instruction is a target instruction.

I think it makes sense to treat entry into a signal handler as a call.  Code
that has been compiled for BTI, and whose page has been marked with PROT_BTI,
will already have the pauth/bti markup at the beginning of the signal handler
function; we might as well verify that.

Otherwise sigaction becomes a hole by which an attacker can force execution to
start at any arbitrary address.


r~

Re: [PATCH v2 08/12] arm64: BTI: Decode BYTPE bits when printing PSTATE

[Richard Henderson]

On 10/10/19 2:44 PM, Dave Martin wrote:
>  #define PSR_IL_BIT		(1 << 20)
> -#define PSR_BTYPE_CALL		(2 << PSR_BTYPE_SHIFT)
> +
> +/* Convenience names for the values of PSTATE.BTYPE */
> +#define PSR_BTYPE_NONE		(0b00 << PSR_BTYPE_SHIFT)
> +#define PSR_BTYPE_JC		(0b01 << PSR_BTYPE_SHIFT)
> +#define PSR_BTYPE_C		(0b10 << PSR_BTYPE_SHIFT)
> +#define PSR_BTYPE_J		(0b11 << PSR_BTYPE_SHIFT)

It'd be nice to sort this patch earlier, so that ...

> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 4a3bd32..452ac5b 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -732,7 +732,7 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
>  
>  	if (system_supports_bti()) {
>  		regs->pstate &= ~PSR_BTYPE_MASK;
> -		regs->pstate |= PSR_BTYPE_CALL;
> +		regs->pstate |= PSR_BTYPE_C;
>  	}
>  
>  	if (ka->sa.sa_flags & SA_RESTORER)

... setup_return does not need to be adjusted a second time.

I don't see any other conflicts vs patch 5.


r~

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> On 10/11/19 11:10 AM, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> >> @@ -730,6 +730,11 @@ static void setup_return
> >>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> >>  	regs->pc = (unsigned long)ka->sa.sa_handler;
> >>  
> >> +	if (system_supports_bti()) {
> >> +		regs->pstate &= ~PSR_BTYPE_MASK;
> >> +		regs->pstate |= PSR_BTYPE_CALL;
> >> +	}
> >> +
> > 
> > I think we might need a comment as to what we're trying to ensure here.
> > 
> > I was under the (perhaps mistaken) impression that we'd generate a
> > pristine pstate for a signal handler, and it's not clear to me that we
> > must ensure the first instruction is a target instruction.
> 
> I think it makes sense to treat entry into a signal handler as a call.  Code
> that has been compiled for BTI, and whose page has been marked with PROT_BTI,
> will already have the pauth/bti markup at the beginning of the signal handler
> function; we might as well verify that.
> 
> Otherwise sigaction becomes a hole by which an attacker can force execution to
> start at any arbitrary address.

Ack, that's the intended rationale -- I also outlined this in the commit
message.

Does this sound reasonable?


Either way, I feel we should do this: any function in a PROT_BTI page
should have a suitable landing pad.  There's no reason I can see why
a protection given to any other callback function should be omitted
for a signal handler.

Note, if the signal handler isn't in a PROT_BTI page then overriding
BTYPE here will not trigger a Branch Target exception.

I'm happy to drop a brief comment into the code also, once we're
agreed on what the code should be doing.

Cheers
---Dave

Re: [PATCH v2 08/12] arm64: BTI: Decode BYTPE bits when printing PSTATE

[Dave Martin]

On Fri, Oct 11, 2019 at 11:31:02AM -0400, Richard Henderson wrote:
> On 10/10/19 2:44 PM, Dave Martin wrote:
> >  #define PSR_IL_BIT		(1 << 20)
> > -#define PSR_BTYPE_CALL		(2 << PSR_BTYPE_SHIFT)
> > +
> > +/* Convenience names for the values of PSTATE.BTYPE */
> > +#define PSR_BTYPE_NONE		(0b00 << PSR_BTYPE_SHIFT)
> > +#define PSR_BTYPE_JC		(0b01 << PSR_BTYPE_SHIFT)
> > +#define PSR_BTYPE_C		(0b10 << PSR_BTYPE_SHIFT)
> > +#define PSR_BTYPE_J		(0b11 << PSR_BTYPE_SHIFT)
> 
> It'd be nice to sort this patch earlier, so that ...
> 
> > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > index 4a3bd32..452ac5b 100644
> > --- a/arch/arm64/kernel/signal.c
> > +++ b/arch/arm64/kernel/signal.c
> > @@ -732,7 +732,7 @@ static void setup_return(struct pt_regs *regs, struct k_sigaction *ka,
> >  
> >  	if (system_supports_bti()) {
> >  		regs->pstate &= ~PSR_BTYPE_MASK;
> > -		regs->pstate |= PSR_BTYPE_CALL;
> > +		regs->pstate |= PSR_BTYPE_C;
> >  	}
> >  
> >  	if (ka->sa.sa_flags & SA_RESTORER)
> 
> ... setup_return does not need to be adjusted a second time.
> 
> I don't see any other conflicts vs patch 5.

Ack, looks like I mis-split this during rebase.

Will fix.

Cheers
---Dave

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Mark Rutland]

On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > >> @@ -730,6 +730,11 @@ static void setup_return
> > >>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > >>  	regs->pc = (unsigned long)ka->sa.sa_handler;
> > >>  
> > >> +	if (system_supports_bti()) {
> > >> +		regs->pstate &= ~PSR_BTYPE_MASK;
> > >> +		regs->pstate |= PSR_BTYPE_CALL;
> > >> +	}
> > >> +
> > > 
> > > I think we might need a comment as to what we're trying to ensure here.
> > > 
> > > I was under the (perhaps mistaken) impression that we'd generate a
> > > pristine pstate for a signal handler, and it's not clear to me that we
> > > must ensure the first instruction is a target instruction.
> > 
> > I think it makes sense to treat entry into a signal handler as a call.  Code
> > that has been compiled for BTI, and whose page has been marked with PROT_BTI,
> > will already have the pauth/bti markup at the beginning of the signal handler
> > function; we might as well verify that.
> > 
> > Otherwise sigaction becomes a hole by which an attacker can force execution to
> > start at any arbitrary address.
> 
> Ack, that's the intended rationale -- I also outlined this in the commit
> message.

Ah, sorry. I evidently did not read that thoroughly enough.

> Does this sound reasonable?
> 
> 
> Either way, I feel we should do this: any function in a PROT_BTI page
> should have a suitable landing pad.  There's no reason I can see why
> a protection given to any other callback function should be omitted
> for a signal handler.
> 
> Note, if the signal handler isn't in a PROT_BTI page then overriding
> BTYPE here will not trigger a Branch Target exception.
> 
> I'm happy to drop a brief comment into the code also, once we're
> agreed on what the code should be doing.

So long as there's a comment as to why, I have no strong feelings here.
:)

Thanks,
Mark.

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > >>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > >>  	regs->pc = (unsigned long)ka->sa.sa_handler;
> > > >>  
> > > >> +	if (system_supports_bti()) {
> > > >> +		regs->pstate &= ~PSR_BTYPE_MASK;
> > > >> +		regs->pstate |= PSR_BTYPE_CALL;
> > > >> +	}
> > > >> +
> > > > 
> > > > I think we might need a comment as to what we're trying to ensure here.
> > > > 
> > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > pristine pstate for a signal handler, and it's not clear to me that we
> > > > must ensure the first instruction is a target instruction.
> > > 
> > > I think it makes sense to treat entry into a signal handler as a call.  Code
> > > that has been compiled for BTI, and whose page has been marked with PROT_BTI,
> > > will already have the pauth/bti markup at the beginning of the signal handler
> > > function; we might as well verify that.
> > > 
> > > Otherwise sigaction becomes a hole by which an attacker can force execution to
> > > start at any arbitrary address.
> > 
> > Ack, that's the intended rationale -- I also outlined this in the commit
> > message.
> 
> Ah, sorry. I evidently did not read that thoroughly enough.
> 
> > Does this sound reasonable?
> > 
> > 
> > Either way, I feel we should do this: any function in a PROT_BTI page
> > should have a suitable landing pad.  There's no reason I can see why
> > a protection given to any other callback function should be omitted
> > for a signal handler.
> > 
> > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > BTYPE here will not trigger a Branch Target exception.
> > 
> > I'm happy to drop a brief comment into the code also, once we're
> > agreed on what the code should be doing.
> 
> So long as there's a comment as to why, I have no strong feelings here.
> :)

OK, I think it's worth a brief comment in the code either way, so I'll
add something.

Cheers
---Dave

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > > >>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > > >>  	regs->pc = (unsigned long)ka->sa.sa_handler;
> > > > >>  
> > > > >> +	if (system_supports_bti()) {
> > > > >> +		regs->pstate &= ~PSR_BTYPE_MASK;
> > > > >> +		regs->pstate |= PSR_BTYPE_CALL;
> > > > >> +	}
> > > > >> +
> > > > > 
> > > > > I think we might need a comment as to what we're trying to ensure here.
> > > > > 
> > > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > > pristine pstate for a signal handler, and it's not clear to me that we
> > > > > must ensure the first instruction is a target instruction.
> > > > 
> > > > I think it makes sense to treat entry into a signal handler as a call.  Code
> > > > that has been compiled for BTI, and whose page has been marked with PROT_BTI,
> > > > will already have the pauth/bti markup at the beginning of the signal handler
> > > > function; we might as well verify that.
> > > > 
> > > > Otherwise sigaction becomes a hole by which an attacker can force execution to
> > > > start at any arbitrary address.
> > > 
> > > Ack, that's the intended rationale -- I also outlined this in the commit
> > > message.
> > 
> > Ah, sorry. I evidently did not read that thoroughly enough.
> > 
> > > Does this sound reasonable?
> > > 
> > > 
> > > Either way, I feel we should do this: any function in a PROT_BTI page
> > > should have a suitable landing pad.  There's no reason I can see why
> > > a protection given to any other callback function should be omitted
> > > for a signal handler.
> > > 
> > > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > > BTYPE here will not trigger a Branch Target exception.
> > > 
> > > I'm happy to drop a brief comment into the code also, once we're
> > > agreed on what the code should be doing.
> > 
> > So long as there's a comment as to why, I have no strong feelings here.
> > :)
> 
> OK, I think it's worth a brief comment in the code either way, so I'll
> add something.

Hmm, come to think of it we do need special logic for a particular case
here:

If we are delivering a SIGILL here and the SIGILL handler was registered
with SA_NODEFER then we will get into a spin, repeatedly delivering
the BTI-triggered SIGILL to the same (bad) entry point.

Without SA_NODEFER, the SIGILL becomes fatal, which is the desired
behaviour, but we'll need to catch this recursion explicitly.


It's similar to the special force_sigsegv() case in
linux/kernel/signal.c...

Thoughts?

Cheers
---Dave

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 11, 2019 at 05:01:13PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> > > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > > > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > > > >>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > > > >>  	regs->pc = (unsigned long)ka->sa.sa_handler;
> > > > > >>  
> > > > > >> +	if (system_supports_bti()) {
> > > > > >> +		regs->pstate &= ~PSR_BTYPE_MASK;
> > > > > >> +		regs->pstate |= PSR_BTYPE_CALL;
> > > > > >> +	}
> > > > > >> +
> > > > > > 
> > > > > > I think we might need a comment as to what we're trying to ensure here.
> > > > > > 
> > > > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > > > pristine pstate for a signal handler, and it's not clear to me that we
> > > > > > must ensure the first instruction is a target instruction.
> > > > > 
> > > > > I think it makes sense to treat entry into a signal handler as a call.  Code
> > > > > that has been compiled for BTI, and whose page has been marked with PROT_BTI,
> > > > > will already have the pauth/bti markup at the beginning of the signal handler
> > > > > function; we might as well verify that.
> > > > > 
> > > > > Otherwise sigaction becomes a hole by which an attacker can force execution to
> > > > > start at any arbitrary address.
> > > > 
> > > > Ack, that's the intended rationale -- I also outlined this in the commit
> > > > message.
> > > 
> > > Ah, sorry. I evidently did not read that thoroughly enough.
> > > 
> > > > Does this sound reasonable?
> > > > 
> > > > 
> > > > Either way, I feel we should do this: any function in a PROT_BTI page
> > > > should have a suitable landing pad.  There's no reason I can see why
> > > > a protection given to any other callback function should be omitted
> > > > for a signal handler.
> > > > 
> > > > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > > > BTYPE here will not trigger a Branch Target exception.
> > > > 
> > > > I'm happy to drop a brief comment into the code also, once we're
> > > > agreed on what the code should be doing.
> > > 
> > > So long as there's a comment as to why, I have no strong feelings here.
> > > :)
> > 
> > OK, I think it's worth a brief comment in the code either way, so I'll
> > add something.
> 
> Hmm, come to think of it we do need special logic for a particular case
> here:
> 
> If we are delivering a SIGILL here and the SIGILL handler was registered
> with SA_NODEFER then we will get into a spin, repeatedly delivering
> the BTI-triggered SIGILL to the same (bad) entry point.
> 
> Without SA_NODEFER, the SIGILL becomes fatal, which is the desired
> behaviour, but we'll need to catch this recursion explicitly.
> 
> 
> It's similar to the special force_sigsegv() case in
> linux/kernel/signal.c...
> 
> Thoughts?

On second thought, maybe we don't need to do anything special.

A SIGSEGV handler registered with (SA_NODEFER & ~SA_RESETHAND) and that
dereferences a duff address would spin similarly.

This SIGILL case doesn't really seem different.  Either way it's a
livelock of the user task that doesn't compromise the kernel.  There
are plenty of ways for such a livelock to happen.

Cheers
---Dave

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > This patch adds the bare minimum required to expose the ARMv8.5
> > Branch Target Identification feature to userspace.
> > 
> > By itself, this does _not_ automatically enable BTI for any initial
> > executable pages mapped by execve().  This will come later, but for
> > now it should be possible to enable BTI manually on those pages by
> > using mprotect() from within the target process.
> > 
> > Other arches already using the generic mman.h are already using
> > 0x10 for arch-specific prot flags, so we use that for PROT_BTI
> > here.
> > 
> > For consistency, signal handler entry points in BTI guarded pages
> > are required to be annotated as such, just like any other function.
> > This blocks a relatively minor attack vector, but comforming
> > userspace will have the annotations anyway, so we may as well
> > enforce them.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > 
> > ---
> > 
> > Changes since v1:
> > 
> >  * Configure SCTLR_EL1.BTx to disallow BR onto a PACIxSP instruction
> >    (except via X16/X17):
> > 
> >    The AArch64 procedure call standard requires binaries marked with
> >    GNU_PROPERTY_AARCH64_FEATURE_1_BTI to use X16/X17 in trampolines
> >    and tail calls, so it makes no sense to be permissive.
> > 
> >  * Rename PROT_BTI_GUARDED to PROT_BTI.
> > 
> >  * Rename VM_ARM64_GP to VM_ARM64_BTI:
> > 
> >    Although the architectural name for the BTI page table bit is "GP",
> >    BTI is nonetheless the feature it controls.  So avoid introducing
> >    the "GP" naming just for this -- it's just an unecessary extra
> >    source of confusion.
> > 
> >  * Tidy up masking with ~PSR_BTYPE_MASK.
> > 
> >  * Drop masking out of BTYPE on SVC, with a comment outlining why.
> > 
> >  * Split PSR_BTYPE_SHIFT definition into this patch.  It's not
> >    useful yet, but it makes sense to define PSR_BTYPE_* using this
> >    from the outset.
> > 
> >  * Migrate to ct_user_exit_irqoff in entry.S:el0_bti.
> 
> [...]
> 
> > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
> > new file mode 100644
> > index 0000000..cbfe3238
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/mman.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_MMAN_H__
> > +#define __ASM_MMAN_H__
> > +
> > +#include <uapi/asm/mman.h>
> > +
> > +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> > +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> > +{
> > +	if (system_supports_bti() && (prot & PROT_BTI))
> > +		return VM_ARM64_BTI;
> > +
> > +	return 0;
> > +}
> 
> Can we call this arch_calc_vm_prot_bits() directly, with all the
> arguments:
> 
> static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> 						   unsigned long pkey)
> {
> 	...
> }
> #define arch_calc_vm_prot_bits arch_calc_vm_prot_bits
> 
> ... as that makes it a bit easier to match definition with use, and just
> definign the name makes it a bit clearer that that's probably for the
> benefit of some ifdeffery.
> 
> Likewise for the other functions here.
> 
> > +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> > +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> > +{
> > +	return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
> > +}
> > +
> > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)

Can do, though it looks like a used sparc as a template, and that has a
sparc_ prefix.

powerpc uses the generic name, as does x86 ... in its UAPI headers.
Odd.

I can change the names here, though I'm not sure it adds a lot of value.

If you feel strongly I can do it.

> > +{
> > +	unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> > +
> > +	if (system_supports_bti())
> > +		supported |= PROT_BTI;
> > +
> > +	return (prot & ~supported) == 0;
> > +}
> 
> If we have this check, can we ever get into arm64_calc_vm_prot_bits()
> with PROT_BIT but !system_supports_bti()?
> 
> ... or can that become:
> 
> 	return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;

We can reach this via mmap() and friends IIUC.

Since this function only gets called once-ish per vma I have a weak
preference for keeping the check here to avoid code fragility.


It does feel like arch_validate_prot() is supposed to be a generic gate
for prot flags coming into the kernel via any route though, but only the
mprotect() path actually uses it.

This function originally landed in v2.6.27 as part of the powerpc strong
access ordering support (PROT_SAO):

b845f313d78e ("mm: Allow architectures to define additional protection bits")
ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support")

where the mmap() path uses arch_calc_vm_prot_bits() without
arch_validate_prot(), just as in the current code.  powerpc's original
arch_calc_vm_prot_bits() does no obvious policing.


This might be a bug.  I can draft a patch to add it for the mmap() path
for people to comment on ... I can't figure out yet whether or not the
difference is intentional or there's some subtlety that I'm missed.

mmap( ... prot = -1 ... ) succeeds with effective rwx permissions and no
apparent ill effects on my random x86 box, but mprotect(..., -1) fails
with -EINVAL.

This is at least strange.

Theoretically, tightening this would be an ABI break, though I'd say
this behaviour is not intentional.

Thoughts?

[...]

Cheers
---Dave

Re: [PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

[Dave Martin]

On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote:
> On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote:
> > Correct skipping of an instruction on AArch32 works a bit
> > differently from AArch64, mainly due to the different CPSR/PSTATE
> > semantics.
> > 
> > There have been various attempts to get this right.  Currenty
> > arm64_skip_faulting_instruction() mostly does the right thing, but
> > does not advance the IT state machine for the AArch32 case.
> > 
> > arm64_compat_skip_faulting_instruction() handles the IT state
> > machine but is local to traps.c, and porting other code to use it
> > will make a mess since there are some call sites that apply for
> > both the compat and native cases.
> > 
> > Since manual instruction skipping implies a trap, it's a relatively
> > slow path.
> > 
> > So, make arm64_skip_faulting_instruction() handle both compat and
> > native, and get rid of the arm64_compat_skip_faulting_instruction()
> > special case.
> > 
> > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler")
> > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance")
> > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps")
> > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm")
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/kernel/traps.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> This looks good to me; it's certainly easier to reason about.
> 
> I couldn't spot a place where we do the wrong thing today, given AFAICT
> all the instances in arch/arm64/kernel/armv8_deprecated.c would be
> UNPREDICTABLE within an IT block.
> 
> It might be worth calling out an example in the commit message to
> justify the fixes tags.

IIRC I found no bug here; rather we have pointlessly fragmented code,
so I followed the "if fixing the same bug in multiple places, merge
those places so you need only fix it in one place next time" rule.

Since arm64_skip_faulting_instruction() is most of the way to being
generically usable anyway, this series merges all the special-case
handling into it.

I could add something like

--8<--

This allows this fiddly operation to be maintained in a single
place rather than trying to maintain fragmented versions spread
around arch/arm64.

-->8--

Any good?

Cheers
---Dave

[...]

> > 
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index 15e3c4f..44c91d4 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -268,6 +268,8 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
> >  	}
> >  }
> >  
> > +static void advance_itstate(struct pt_regs *regs);
> > +
> >  void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
> >  {
> >  	regs->pc += size;
> > @@ -278,6 +280,9 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
> >  	 */
> >  	if (user_mode(regs))
> >  		user_fastforward_single_step(current);
> > +
> > +	if (regs->pstate & PSR_MODE32_BIT)
> > +		advance_itstate(regs);
> >  }
> >  
> >  static LIST_HEAD(undef_hook);
> > @@ -629,19 +634,12 @@ static void advance_itstate(struct pt_regs *regs)
> >  	compat_set_it_state(regs, it);
> >  }
> >  
> > -static void arm64_compat_skip_faulting_instruction(struct pt_regs *regs,
> > -						   unsigned int sz)
> > -{
> > -	advance_itstate(regs);
> > -	arm64_skip_faulting_instruction(regs, sz);
> > -}
> > -
> >  static void compat_cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
> >  {
> >  	int reg = (esr & ESR_ELx_CP15_32_ISS_RT_MASK) >> ESR_ELx_CP15_32_ISS_RT_SHIFT;
> >  
> >  	pt_regs_write_reg(regs, reg, arch_timer_get_rate());
> > -	arm64_compat_skip_faulting_instruction(regs, 4);
> > +	arm64_skip_faulting_instruction(regs, 4);
> >  }
> >  
> >  static const struct sys64_hook cp15_32_hooks[] = {
> > @@ -661,7 +659,7 @@ static void compat_cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
> >  
> >  	pt_regs_write_reg(regs, rt, lower_32_bits(val));
> >  	pt_regs_write_reg(regs, rt2, upper_32_bits(val));
> > -	arm64_compat_skip_faulting_instruction(regs, 4);
> > +	arm64_skip_faulting_instruction(regs, 4);
> >  }
> >  
> >  static const struct sys64_hook cp15_64_hooks[] = {
> > @@ -682,7 +680,7 @@ asmlinkage void __exception do_cp15instr(unsigned int esr, struct pt_regs *regs)
> >  		 * There is no T16 variant of a CP access, so we
> >  		 * always advance PC by 4 bytes.
> >  		 */
> > -		arm64_compat_skip_faulting_instruction(regs, 4);
> > +		arm64_skip_faulting_instruction(regs, 4);
> >  		return;
> >  	}
> >  
> > -- 
> > 2.1.4
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

[Mark Rutland]

On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote:
> > > Correct skipping of an instruction on AArch32 works a bit
> > > differently from AArch64, mainly due to the different CPSR/PSTATE
> > > semantics.
> > > 
> > > There have been various attempts to get this right.  Currenty
> > > arm64_skip_faulting_instruction() mostly does the right thing, but
> > > does not advance the IT state machine for the AArch32 case.
> > > 
> > > arm64_compat_skip_faulting_instruction() handles the IT state
> > > machine but is local to traps.c, and porting other code to use it
> > > will make a mess since there are some call sites that apply for
> > > both the compat and native cases.
> > > 
> > > Since manual instruction skipping implies a trap, it's a relatively
> > > slow path.
> > > 
> > > So, make arm64_skip_faulting_instruction() handle both compat and
> > > native, and get rid of the arm64_compat_skip_faulting_instruction()
> > > special case.
> > > 
> > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler")
> > > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance")
> > > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps")
> > > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm")
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > >  arch/arm64/kernel/traps.c | 18 ++++++++----------
> > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > This looks good to me; it's certainly easier to reason about.
> > 
> > I couldn't spot a place where we do the wrong thing today, given AFAICT
> > all the instances in arch/arm64/kernel/armv8_deprecated.c would be
> > UNPREDICTABLE within an IT block.
> > 
> > It might be worth calling out an example in the commit message to
> > justify the fixes tags.
> 
> IIRC I found no bug here; rather we have pointlessly fragmented code,
> so I followed the "if fixing the same bug in multiple places, merge
> those places so you need only fix it in one place next time" rule.

Sure thing, that makes sense to me.

> Since arm64_skip_faulting_instruction() is most of the way to being
> generically usable anyway, this series merges all the special-case
> handling into it.
> 
> I could add something like
> 
> --8<--
> 
> This allows this fiddly operation to be maintained in a single
> place rather than trying to maintain fragmented versions spread
> around arch/arm64.
> 
> -->8--
> 
> Any good?

My big concern is that the commit message reads as a fix, implying that
there's an existing correctness bug. I think that simplifying it to make
it clearer that it's a cleanup/improvement would be preferable.

How about:

| arm64: unify native/compat instruction skipping
|
| Skipping of an instruction on AArch32 works a bit differently from
| AArch64, mainly due to the different CPSR/PSTATE semantics.
|
| Currently arm64_skip_faulting_instruction() is only suitable for
| AArch64, and arm64_compat_skip_faulting_instruction() handles the IT
| state machine but is local to traps.c.
| 
| Since manual instruction skipping implies a trap, it's a relatively
| slow path.
| 
| So, make arm64_skip_faulting_instruction() handle both compat and
| native, and get rid of the arm64_compat_skip_faulting_instruction()
| special case.
|
| Signed-off-by: Dave Martin <Dave.Martin@arm.com>

With that, feel free to add:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

We could even point out that the armv8_deprecated cases are
UNPREDICTABLE in an IT block, and correctly emulated either way.

Thanks,
Mark.

Re: [PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

[Dave Martin]

On Tue, Oct 15, 2019 at 05:42:04PM +0100, Mark Rutland wrote:
> On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote:
> > > > Correct skipping of an instruction on AArch32 works a bit
> > > > differently from AArch64, mainly due to the different CPSR/PSTATE
> > > > semantics.
> > > > 
> > > > There have been various attempts to get this right.  Currenty
> > > > arm64_skip_faulting_instruction() mostly does the right thing, but
> > > > does not advance the IT state machine for the AArch32 case.
> > > > 
> > > > arm64_compat_skip_faulting_instruction() handles the IT state
> > > > machine but is local to traps.c, and porting other code to use it
> > > > will make a mess since there are some call sites that apply for
> > > > both the compat and native cases.
> > > > 
> > > > Since manual instruction skipping implies a trap, it's a relatively
> > > > slow path.
> > > > 
> > > > So, make arm64_skip_faulting_instruction() handle both compat and
> > > > native, and get rid of the arm64_compat_skip_faulting_instruction()
> > > > special case.
> > > > 
> > > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler")
> > > > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance")
> > > > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps")
> > > > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm")
> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > ---
> > > >  arch/arm64/kernel/traps.c | 18 ++++++++----------
> > > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > > 
> > > This looks good to me; it's certainly easier to reason about.
> > > 
> > > I couldn't spot a place where we do the wrong thing today, given AFAICT
> > > all the instances in arch/arm64/kernel/armv8_deprecated.c would be
> > > UNPREDICTABLE within an IT block.
> > > 
> > > It might be worth calling out an example in the commit message to
> > > justify the fixes tags.
> > 
> > IIRC I found no bug here; rather we have pointlessly fragmented code,
> > so I followed the "if fixing the same bug in multiple places, merge
> > those places so you need only fix it in one place next time" rule.
> 
> Sure thing, that makes sense to me.
> 
> > Since arm64_skip_faulting_instruction() is most of the way to being
> > generically usable anyway, this series merges all the special-case
> > handling into it.
> > 
> > I could add something like
> > 
> > --8<--
> > 
> > This allows this fiddly operation to be maintained in a single
> > place rather than trying to maintain fragmented versions spread
> > around arch/arm64.
> > 
> > -->8--
> > 
> > Any good?
> 
> My big concern is that the commit message reads as a fix, implying that
> there's an existing correctness bug. I think that simplifying it to make
> it clearer that it's a cleanup/improvement would be preferable.
> 
> How about:
> 
> | arm64: unify native/compat instruction skipping
> |
> | Skipping of an instruction on AArch32 works a bit differently from
> | AArch64, mainly due to the different CPSR/PSTATE semantics.
> |
> | Currently arm64_skip_faulting_instruction() is only suitable for
> | AArch64, and arm64_compat_skip_faulting_instruction() handles the IT
> | state machine but is local to traps.c.
> | 
> | Since manual instruction skipping implies a trap, it's a relatively
> | slow path.
> | 
> | So, make arm64_skip_faulting_instruction() handle both compat and
> | native, and get rid of the arm64_compat_skip_faulting_instruction()
> | special case.
> |
> | Signed-off-by: Dave Martin <Dave.Martin@arm.com>

And drop the Fixes tags.  Yes, I think that's reasonable.

I think I was probably glossing over the fact that we don't need to get
the ITSTATE machine advance correct for the compat insn emulation; as
you say, I can't see what else this patch "fixes".

> With that, feel free to add:
>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks!

> We could even point out that the armv8_deprecated cases are
> UNPREDICTABLE in an IT block, and correctly emulated either way.

Yes, I'll add something along those lines.

Cheers
---Dave

Re: [PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions

[Mark Rutland]

On Fri, Oct 11, 2019 at 03:47:43PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 03:21:58PM +0100, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:39PM +0100, Dave Martin wrote:
> > > Since normal execution of any non-branch instruction resets the
> > > PSTATE BTYPE field to 0, so do the same thing when emulating a
> > > trapped instruction.
> > > 
> > > Branches don't trap directly, so we should never need to assign a
> > > non-zero value to BTYPE here.
> > > 
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > >  arch/arm64/kernel/traps.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > index 3af2768..4d8ce50 100644
> > > --- a/arch/arm64/kernel/traps.c
> > > +++ b/arch/arm64/kernel/traps.c
> > > @@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
> > >  
> > >  	if (regs->pstate & PSR_MODE32_BIT)
> > >  		advance_itstate(regs);
> > > +	else
> > > +		regs->pstate &= ~(u64)PSR_BTYPE_MASK;
> > 
> > This looks good to me, with one nit below.
> > 
> > We don't (currently) need the u64 cast here, and it's inconsistent with
> > what we do elsewhere. If the upper 32-bit of pstate get allocated, we'll
> > need to fix up all the other masking we do:
> 
> Huh, looks like I missed that.  Dang.  Will fix.
> 
> > [mark@lakrids:~/src/linux]% git grep 'pstate &= ~'
> > arch/arm64/kernel/armv8_deprecated.c:           regs->pstate &= ~PSR_AA32_E_BIT;
> > arch/arm64/kernel/cpufeature.c:         regs->pstate &= ~PSR_SSBS_BIT;
> > arch/arm64/kernel/debug-monitors.c:     regs->pstate &= ~DBG_SPSR_SS;
> > arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
> > arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
> > arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~PSR_D_BIT;
> > arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~DAIF_MASK;
> > arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS;
> > arch/arm64/kernel/ptrace.c:                     regs->pstate &= ~PSR_AA32_E_BIT;
> > arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS;
> > arch/arm64/kernel/ptrace.c:             regs->pstate &= ~DBG_SPSR_SS;
> > arch/arm64/kernel/ssbd.c:       task_pt_regs(task)->pstate &= ~val;
> > arch/arm64/kernel/traps.c:      regs->pstate &= ~PSR_AA32_IT_MASK;
> > 
> > ... and at that point I'd suggest we should just ensure the bit
> > definitions are all defined as unsigned long in the first place since
> > adding casts to each use is error-prone.
> 
> Are we concerned about changing the types of UAPI #defines?  That can
> cause subtle and unexpected breakage, especially when the signedness
> of a #define changes.
> 
> Ideally, we'd just change all these to 1UL << n.

I agree that's the ideal -- I don't know how concerned we are w.r.t. the
UAPI headers, I'm afraid.

Thanks,
Mark.

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Mark Rutland]

On Fri, Oct 11, 2019 at 05:42:00PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 05:01:13PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote:
> > > On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> > > > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:
> > > > > On Fri, Oct 11, 2019 at 11:25:33AM -0400, Richard Henderson wrote:
> > > > > > On 10/11/19 11:10 AM, Mark Rutland wrote:
> > > > > > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > > > >> @@ -730,6 +730,11 @@ static void setup_return
> > > > > > >>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> > > > > > >>  	regs->pc = (unsigned long)ka->sa.sa_handler;
> > > > > > >>  
> > > > > > >> +	if (system_supports_bti()) {
> > > > > > >> +		regs->pstate &= ~PSR_BTYPE_MASK;
> > > > > > >> +		regs->pstate |= PSR_BTYPE_CALL;
> > > > > > >> +	}
> > > > > > >> +
> > > > > > > 
> > > > > > > I think we might need a comment as to what we're trying to ensure here.
> > > > > > > 
> > > > > > > I was under the (perhaps mistaken) impression that we'd generate a
> > > > > > > pristine pstate for a signal handler, and it's not clear to me that we
> > > > > > > must ensure the first instruction is a target instruction.
> > > > > > 
> > > > > > I think it makes sense to treat entry into a signal handler as a call.  Code
> > > > > > that has been compiled for BTI, and whose page has been marked with PROT_BTI,
> > > > > > will already have the pauth/bti markup at the beginning of the signal handler
> > > > > > function; we might as well verify that.
> > > > > > 
> > > > > > Otherwise sigaction becomes a hole by which an attacker can force execution to
> > > > > > start at any arbitrary address.
> > > > > 
> > > > > Ack, that's the intended rationale -- I also outlined this in the commit
> > > > > message.
> > > > 
> > > > Ah, sorry. I evidently did not read that thoroughly enough.
> > > > 
> > > > > Does this sound reasonable?
> > > > > 
> > > > > 
> > > > > Either way, I feel we should do this: any function in a PROT_BTI page
> > > > > should have a suitable landing pad.  There's no reason I can see why
> > > > > a protection given to any other callback function should be omitted
> > > > > for a signal handler.
> > > > > 
> > > > > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > > > > BTYPE here will not trigger a Branch Target exception.
> > > > > 
> > > > > I'm happy to drop a brief comment into the code also, once we're
> > > > > agreed on what the code should be doing.
> > > > 
> > > > So long as there's a comment as to why, I have no strong feelings here.
> > > > :)
> > > 
> > > OK, I think it's worth a brief comment in the code either way, so I'll
> > > add something.
> > 
> > Hmm, come to think of it we do need special logic for a particular case
> > here:
> > 
> > If we are delivering a SIGILL here and the SIGILL handler was registered
> > with SA_NODEFER then we will get into a spin, repeatedly delivering
> > the BTI-triggered SIGILL to the same (bad) entry point.
> > 
> > Without SA_NODEFER, the SIGILL becomes fatal, which is the desired
> > behaviour, but we'll need to catch this recursion explicitly.
> > 
> > 
> > It's similar to the special force_sigsegv() case in
> > linux/kernel/signal.c...
> > 
> > Thoughts?
> 
> On second thought, maybe we don't need to do anything special.
> 
> A SIGSEGV handler registered with (SA_NODEFER & ~SA_RESETHAND) and that
> dereferences a duff address would spin similarly.
> 
> This SIGILL case doesn't really seem different.  Either way it's a
> livelock of the user task that doesn't compromise the kernel.  There
> are plenty of ways for such a livelock to happen.

That sounds reasonable to me.

Thanks,
Mark.

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Mark Rutland]

On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> > > +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> > > +{
> > > +	if (system_supports_bti() && (prot & PROT_BTI))
> > > +		return VM_ARM64_BTI;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Can we call this arch_calc_vm_prot_bits() directly, with all the
> > arguments:
> > 
> > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> > 						   unsigned long pkey)
> > {
> > 	...
> > }
> > #define arch_calc_vm_prot_bits arch_calc_vm_prot_bits
> > 
> > ... as that makes it a bit easier to match definition with use, and just
> > definign the name makes it a bit clearer that that's probably for the
> > benefit of some ifdeffery.
> > 
> > Likewise for the other functions here.
> > 
> > > +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> > > +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> > > +{
> > > +	return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
> > > +}
> > > +
> > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
> 
> Can do, though it looks like a used sparc as a template, and that has a
> sparc_ prefix.
> 
> powerpc uses the generic name, as does x86 ... in its UAPI headers.
> Odd.
> 
> I can change the names here, though I'm not sure it adds a lot of value.
> 
> If you feel strongly I can do it.

I'd really prefer it because it minimizes surprises, and makes it much
easier to hop around the codebase and find the thing you're looking for.

I'll reply on the other issue in a separate reply.

Thanks,
Mark.

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Mark Rutland]

[adding mm folk]

On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
> > > +{
> > > +	unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> > > +
> > > +	if (system_supports_bti())
> > > +		supported |= PROT_BTI;
> > > +
> > > +	return (prot & ~supported) == 0;
> > > +}
> > 
> > If we have this check, can we ever get into arm64_calc_vm_prot_bits()
> > with PROT_BIT but !system_supports_bti()?
> > 
> > ... or can that become:
> > 
> > 	return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;
> 
> We can reach this via mmap() and friends IIUC.
> 
> Since this function only gets called once-ish per vma I have a weak
> preference for keeping the check here to avoid code fragility.
> 
> 
> It does feel like arch_validate_prot() is supposed to be a generic gate
> for prot flags coming into the kernel via any route though, but only the
> mprotect() path actually uses it.
> 
> This function originally landed in v2.6.27 as part of the powerpc strong
> access ordering support (PROT_SAO):
> 
> b845f313d78e ("mm: Allow architectures to define additional protection bits")
> ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support")
> 
> where the mmap() path uses arch_calc_vm_prot_bits() without
> arch_validate_prot(), just as in the current code.  powerpc's original
> arch_calc_vm_prot_bits() does no obvious policing.
> 
> This might be a bug.  I can draft a patch to add it for the mmap() path
> for people to comment on ... I can't figure out yet whether or not the
> difference is intentional or there's some subtlety that I'm missed.

From reading those two commit messages, it looks like this was an
oversight. I'd expect that we should apply this check for any
user-provided prot (i.e. it should apply to both mprotect and mmap).

Ben, Andrew, does that make sense to you?

... or was there some reason to only do this for mprotect?

Thanks,
Mark.

> mmap( ... prot = -1 ... ) succeeds with effective rwx permissions and no
> apparent ill effects on my random x86 box, but mprotect(..., -1) fails
> with -EINVAL.
> 
> This is at least strange.
> 
> Theoretically, tightening this would be an ABI break, though I'd say
> this behaviour is not intentional.
> 
> Thoughts?
> 
> [...]
> 
> Cheers
> ---Dave

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 18, 2019 at 12:05:52PM +0100, Mark Rutland wrote:
> On Fri, Oct 11, 2019 at 05:42:00PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 05:01:13PM +0100, Dave Martin wrote:
> > > On Fri, Oct 11, 2019 at 04:44:45PM +0100, Dave Martin wrote:
> > > > On Fri, Oct 11, 2019 at 04:40:43PM +0100, Mark Rutland wrote:
> > > > > On Fri, Oct 11, 2019 at 04:32:26PM +0100, Dave Martin wrote:

[...]

> > > > > > Either way, I feel we should do this: any function in a PROT_BTI page
> > > > > > should have a suitable landing pad.  There's no reason I can see why
> > > > > > a protection given to any other callback function should be omitted
> > > > > > for a signal handler.
> > > > > > 
> > > > > > Note, if the signal handler isn't in a PROT_BTI page then overriding
> > > > > > BTYPE here will not trigger a Branch Target exception.
> > > > > > 
> > > > > > I'm happy to drop a brief comment into the code also, once we're
> > > > > > agreed on what the code should be doing.
> > > > > 
> > > > > So long as there's a comment as to why, I have no strong feelings here.
> > > > > :)
> > > > 
> > > > OK, I think it's worth a brief comment in the code either way, so I'll
> > > > add something.
> > > 
> > > Hmm, come to think of it we do need special logic for a particular case
> > > here:
> > > 
> > > If we are delivering a SIGILL here and the SIGILL handler was registered
> > > with SA_NODEFER then we will get into a spin, repeatedly delivering
> > > the BTI-triggered SIGILL to the same (bad) entry point.
> > > 
> > > Without SA_NODEFER, the SIGILL becomes fatal, which is the desired
> > > behaviour, but we'll need to catch this recursion explicitly.
> > > 
> > > 
> > > It's similar to the special force_sigsegv() case in
> > > linux/kernel/signal.c...
> > > 
> > > Thoughts?
> > 
> > On second thought, maybe we don't need to do anything special.
> > 
> > A SIGSEGV handler registered with (SA_NODEFER & ~SA_RESETHAND) and that
> > dereferences a duff address would spin similarly.
> > 
> > This SIGILL case doesn't really seem different.  Either way it's a
> > livelock of the user task that doesn't compromise the kernel.  There
> > are plenty of ways for such a livelock to happen.
> 
> That sounds reasonable to me.

OK, I guess we can park this discussion for now.

Cheers
---Dave

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 18, 2019 at 12:10:03PM +0100, Mark Rutland wrote:
> On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > +#define arch_calc_vm_prot_bits(prot, pkey) arm64_calc_vm_prot_bits(prot)
> > > > +static inline unsigned long arm64_calc_vm_prot_bits(unsigned long prot)
> > > > +{
> > > > +	if (system_supports_bti() && (prot & PROT_BTI))
> > > > +		return VM_ARM64_BTI;
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > Can we call this arch_calc_vm_prot_bits() directly, with all the
> > > arguments:
> > > 
> > > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> > > 						   unsigned long pkey)
> > > {
> > > 	...
> > > }
> > > #define arch_calc_vm_prot_bits arch_calc_vm_prot_bits
> > > 
> > > ... as that makes it a bit easier to match definition with use, and just
> > > definign the name makes it a bit clearer that that's probably for the
> > > benefit of some ifdeffery.
> > > 
> > > Likewise for the other functions here.
> > > 
> > > > +#define arch_vm_get_page_prot(vm_flags) arm64_vm_get_page_prot(vm_flags)
> > > > +static inline pgprot_t arm64_vm_get_page_prot(unsigned long vm_flags)
> > > > +{
> > > > +	return (vm_flags & VM_ARM64_BTI) ? __pgprot(PTE_GP) : __pgprot(0);
> > > > +}
> > > > +
> > > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > > +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
> > 
> > Can do, though it looks like a used sparc as a template, and that has a
> > sparc_ prefix.
> > 
> > powerpc uses the generic name, as does x86 ... in its UAPI headers.
> > Odd.
> > 
> > I can change the names here, though I'm not sure it adds a lot of value.
> > 
> > If you feel strongly I can do it.
> 
> I'd really prefer it because it minimizes surprises, and makes it much
> easier to hop around the codebase and find the thing you're looking for.

OK, I've no objection in that case.  I'll make the change.

[...]

Cheers
---Dave

Re: [PATCH v2 05/12] arm64: Basic Branch Target Identification support

[Dave Martin]

On Fri, Oct 18, 2019 at 12:16:03PM +0100, Mark Rutland wrote:
> [adding mm folk]
> 
> On Fri, Oct 11, 2019 at 06:20:15PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 04:10:29PM +0100, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:33PM +0100, Dave Martin wrote:
> > > > +#define arch_validate_prot(prot, addr) arm64_validate_prot(prot, addr)
> > > > +static inline int arm64_validate_prot(unsigned long prot, unsigned long addr)
> > > > +{
> > > > +	unsigned long supported = PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM;
> > > > +
> > > > +	if (system_supports_bti())
> > > > +		supported |= PROT_BTI;
> > > > +
> > > > +	return (prot & ~supported) == 0;
> > > > +}
> > > 
> > > If we have this check, can we ever get into arm64_calc_vm_prot_bits()
> > > with PROT_BIT but !system_supports_bti()?
> > > 
> > > ... or can that become:
> > > 
> > > 	return (prot & PROT_BTI) ? VM_ARM64_BTI : 0;
> > 
> > We can reach this via mmap() and friends IIUC.
> > 
> > Since this function only gets called once-ish per vma I have a weak
> > preference for keeping the check here to avoid code fragility.
> > 
> > 
> > It does feel like arch_validate_prot() is supposed to be a generic gate
> > for prot flags coming into the kernel via any route though, but only the
> > mprotect() path actually uses it.
> > 
> > This function originally landed in v2.6.27 as part of the powerpc strong
> > access ordering support (PROT_SAO):
> > 
> > b845f313d78e ("mm: Allow architectures to define additional protection bits")
> > ef3d3246a0d0 ("powerpc/mm: Add Strong Access Ordering support")
> > 
> > where the mmap() path uses arch_calc_vm_prot_bits() without
> > arch_validate_prot(), just as in the current code.  powerpc's original
> > arch_calc_vm_prot_bits() does no obvious policing.
> > 
> > This might be a bug.  I can draft a patch to add it for the mmap() path
> > for people to comment on ... I can't figure out yet whether or not the
> > difference is intentional or there's some subtlety that I'm missed.
> 
> From reading those two commit messages, it looks like this was an
> oversight. I'd expect that we should apply this check for any
> user-provided prot (i.e. it should apply to both mprotect and mmap).
> 
> Ben, Andrew, does that make sense to you?
> 
> ... or was there some reason to only do this for mprotect?
> 
> Thanks,
> Mark.

For now, I'll drop a comment under the tearoff noting this outstanding
question.

The resulting behaviour is slightly odd, but doesn't seem unsafe, and
we can of course tidy it up later.  I think the risk of userspace
becoming dependent on randomly passing PROT_BTI to mprotect() even
when unsupported is low.

[...]

Cheers
---Dave

Re: [PATCH v2 11/12] arm64: BTI: Reset BTYPE when skipping emulated instructions

[Dave Martin]

On Fri, Oct 18, 2019 at 12:04:29PM +0100, Mark Rutland wrote:
> On Fri, Oct 11, 2019 at 03:47:43PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 03:21:58PM +0100, Mark Rutland wrote:
> > > On Thu, Oct 10, 2019 at 07:44:39PM +0100, Dave Martin wrote:
> > > > Since normal execution of any non-branch instruction resets the
> > > > PSTATE BTYPE field to 0, so do the same thing when emulating a
> > > > trapped instruction.
> > > > 
> > > > Branches don't trap directly, so we should never need to assign a
> > > > non-zero value to BTYPE here.
> > > > 
> > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > ---
> > > >  arch/arm64/kernel/traps.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > > index 3af2768..4d8ce50 100644
> > > > --- a/arch/arm64/kernel/traps.c
> > > > +++ b/arch/arm64/kernel/traps.c
> > > > @@ -331,6 +331,8 @@ void arm64_skip_faulting_instruction(struct pt_regs *regs, unsigned long size)
> > > >  
> > > >  	if (regs->pstate & PSR_MODE32_BIT)
> > > >  		advance_itstate(regs);
> > > > +	else
> > > > +		regs->pstate &= ~(u64)PSR_BTYPE_MASK;
> > > 
> > > This looks good to me, with one nit below.
> > > 
> > > We don't (currently) need the u64 cast here, and it's inconsistent with
> > > what we do elsewhere. If the upper 32-bit of pstate get allocated, we'll
> > > need to fix up all the other masking we do:
> > 
> > Huh, looks like I missed that.  Dang.  Will fix.
> > 
> > > [mark@lakrids:~/src/linux]% git grep 'pstate &= ~'
> > > arch/arm64/kernel/armv8_deprecated.c:           regs->pstate &= ~PSR_AA32_E_BIT;
> > > arch/arm64/kernel/cpufeature.c:         regs->pstate &= ~PSR_SSBS_BIT;
> > > arch/arm64/kernel/debug-monitors.c:     regs->pstate &= ~DBG_SPSR_SS;
> > > arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
> > > arch/arm64/kernel/insn.c:       pstate &= ~(pstate >> 1);       /* PSR_C_BIT &= ~PSR_Z_BIT */
> > > arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~PSR_D_BIT;
> > > arch/arm64/kernel/probes/kprobes.c:     regs->pstate &= ~DAIF_MASK;
> > > arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH32_RES0_BITS;
> > > arch/arm64/kernel/ptrace.c:                     regs->pstate &= ~PSR_AA32_E_BIT;
> > > arch/arm64/kernel/ptrace.c:     regs->pstate &= ~SPSR_EL1_AARCH64_RES0_BITS;
> > > arch/arm64/kernel/ptrace.c:             regs->pstate &= ~DBG_SPSR_SS;
> > > arch/arm64/kernel/ssbd.c:       task_pt_regs(task)->pstate &= ~val;
> > > arch/arm64/kernel/traps.c:      regs->pstate &= ~PSR_AA32_IT_MASK;
> > > 
> > > ... and at that point I'd suggest we should just ensure the bit
> > > definitions are all defined as unsigned long in the first place since
> > > adding casts to each use is error-prone.
> > 
> > Are we concerned about changing the types of UAPI #defines?  That can
> > cause subtle and unexpected breakage, especially when the signedness
> > of a #define changes.
> > 
> > Ideally, we'd just change all these to 1UL << n.
> 
> I agree that's the ideal -- I don't know how concerned we are w.r.t. the
> UAPI headers, I'm afraid.

OK, I'll following the existing convention for now, keep the #define as
(implicitly) signed, and drop the u64 casts.

At some point in the future we may want to refactor the headers so that
the kernel uses shadow register bit definitions that are always u64.
The new HWCAP definitions provide a reasonable template for doing that
kind of thing.

It's probably best not to do anything to alter the types of the UAPI
definitions.

I will shamelessly duck this for now :|

Cheers
---Dave

Re: [PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

[Dave Martin]

On Tue, Oct 15, 2019 at 05:49:05PM +0100, Dave Martin wrote:
> On Tue, Oct 15, 2019 at 05:42:04PM +0100, Mark Rutland wrote:
> > On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote:
> > > On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote:
> > > > On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote:
> > > > > Correct skipping of an instruction on AArch32 works a bit
> > > > > differently from AArch64, mainly due to the different CPSR/PSTATE
> > > > > semantics.
> > > > > 
> > > > > There have been various attempts to get this right.  Currenty
> > > > > arm64_skip_faulting_instruction() mostly does the right thing, but
> > > > > does not advance the IT state machine for the AArch32 case.
> > > > > 
> > > > > arm64_compat_skip_faulting_instruction() handles the IT state
> > > > > machine but is local to traps.c, and porting other code to use it
> > > > > will make a mess since there are some call sites that apply for
> > > > > both the compat and native cases.
> > > > > 
> > > > > Since manual instruction skipping implies a trap, it's a relatively
> > > > > slow path.
> > > > > 
> > > > > So, make arm64_skip_faulting_instruction() handle both compat and
> > > > > native, and get rid of the arm64_compat_skip_faulting_instruction()
> > > > > special case.
> > > > > 
> > > > > Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler")
> > > > > Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance")
> > > > > Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps")
> > > > > Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm")
> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > > ---
> > > > >  arch/arm64/kernel/traps.c | 18 ++++++++----------
> > > > >  1 file changed, 8 insertions(+), 10 deletions(-)
> > > > 
> > > > This looks good to me; it's certainly easier to reason about.
> > > > 
> > > > I couldn't spot a place where we do the wrong thing today, given AFAICT
> > > > all the instances in arch/arm64/kernel/armv8_deprecated.c would be
> > > > UNPREDICTABLE within an IT block.
> > > > 
> > > > It might be worth calling out an example in the commit message to
> > > > justify the fixes tags.
> > > 
> > > IIRC I found no bug here; rather we have pointlessly fragmented code,
> > > so I followed the "if fixing the same bug in multiple places, merge
> > > those places so you need only fix it in one place next time" rule.
> > 
> > Sure thing, that makes sense to me.
> > 
> > > Since arm64_skip_faulting_instruction() is most of the way to being
> > > generically usable anyway, this series merges all the special-case
> > > handling into it.
> > > 
> > > I could add something like
> > > 
> > > --8<--
> > > 
> > > This allows this fiddly operation to be maintained in a single
> > > place rather than trying to maintain fragmented versions spread
> > > around arch/arm64.
> > > 
> > > -->8--
> > > 
> > > Any good?
> > 
> > My big concern is that the commit message reads as a fix, implying that
> > there's an existing correctness bug. I think that simplifying it to make
> > it clearer that it's a cleanup/improvement would be preferable.
> > 
> > How about:
> > 
> > | arm64: unify native/compat instruction skipping
> > |
> > | Skipping of an instruction on AArch32 works a bit differently from
> > | AArch64, mainly due to the different CPSR/PSTATE semantics.
> > |
> > | Currently arm64_skip_faulting_instruction() is only suitable for
> > | AArch64, and arm64_compat_skip_faulting_instruction() handles the IT
> > | state machine but is local to traps.c.
> > | 
> > | Since manual instruction skipping implies a trap, it's a relatively
> > | slow path.
> > | 
> > | So, make arm64_skip_faulting_instruction() handle both compat and
> > | native, and get rid of the arm64_compat_skip_faulting_instruction()
> > | special case.
> > |
> > | Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> 
> And drop the Fixes tags.  Yes, I think that's reasonable.
> 
> I think I was probably glossing over the fact that we don't need to get
> the ITSTATE machine advance correct for the compat insn emulation; as
> you say, I can't see what else this patch "fixes".
> 
> > With that, feel free to add:
> >
> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> Thanks!
> 
> > We could even point out that the armv8_deprecated cases are
> > UNPREDICTABLE in an IT block, and correctly emulated either way.
> 
> Yes, I'll add something along those lines.

Taking another look, I now can't track down where e.g., SWP in an IT
block is specified to be UNPREDICTABLE.  I only see e.g.,
ARM DDI 0487E.a Section 1.8.2 ("F1.8.2 Partial deprecation of IT"),
which only deprecates the affected instructions.

The legacy AArch32 SWP{B} insn is obsoleted by ARMv8, but the whole
point of the armv8_deprecated stuff is to provide some backwards
compatiblity with v7.


So, this needs a closer look.

I'll leave the Fixes tags for now, so that the archaeology doesn't need
to redone if we decide that this patch does fix incorrect behaviour.

Cheers
---Dave

Re: [PATCH v2 04/12] arm64: docs: cpu-feature-registers: Document ID_AA64PFR1_EL1

[Mark Brown]

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

On Fri, Oct 11, 2019 at 03:51:49PM +0100, Dave Martin wrote:
> On Fri, Oct 11, 2019 at 02:19:48PM +0100, Alex Bennée wrote:

> > > -  4) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
> > > +  5) ID_AA64ISAR1_EL1 - Instruction set attribute register 1

> > If I'm not mistaken .rst has support for auto-enumeration if the #
> > character is used. That might reduce the pain of re-numbering in future.

> Ack, though it would be good to go one better and generate this document
> from the cpufeature.c tables (or from some common source).  The numbers
> are relatively easy to maintain -- remembering to update the document
> at all seems the bigger maintenance headache right now.

I agree, it'd be better if the table were autogenerated.  Having tried
doing the modification to # it does mean that the document looks a bit
weird when viewing it as a text file in the kernel source which TBH is
how I suspect a lot of people will view it so given the infrequency with
which new registers are added I'm not sure it's worth it.

> I think this particular patch is superseded by similar fixes from other
> people, just not in torvalds/master yet.

Nor in -next for the minute :/

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

Re: [PATCH v2 04/12] arm64: docs: cpu-feature-registers: Document ID_AA64PFR1_EL1

[Will Deacon]

On Mon, Oct 21, 2019 at 08:18:18PM +0100, Mark Brown wrote:
> On Fri, Oct 11, 2019 at 03:51:49PM +0100, Dave Martin wrote:
> > On Fri, Oct 11, 2019 at 02:19:48PM +0100, Alex Bennée wrote:
> 
> > > > -  4) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
> > > > +  5) ID_AA64ISAR1_EL1 - Instruction set attribute register 1
> 
> > > If I'm not mistaken .rst has support for auto-enumeration if the #
> > > character is used. That might reduce the pain of re-numbering in future.
> 
> > Ack, though it would be good to go one better and generate this document
> > from the cpufeature.c tables (or from some common source).  The numbers
> > are relatively easy to maintain -- remembering to update the document
> > at all seems the bigger maintenance headache right now.
> 
> I agree, it'd be better if the table were autogenerated.  Having tried
> doing the modification to # it does mean that the document looks a bit
> weird when viewing it as a text file in the kernel source which TBH is
> how I suspect a lot of people will view it so given the infrequency with
> which new registers are added I'm not sure it's worth it.
> 
> > I think this particular patch is superseded by similar fixes from other
> > people, just not in torvalds/master yet.
> 
> Nor in -next for the minute :/

Which patch is missing? The only other one on my radar is "docs/arm64:
cpu-feature-registers: Documents missing visible fields" which is currently
in -next as a8613e7070e7. "similar fixes from other people" isn't very
specific :(

Will

Re: [PATCH v2 09/12] arm64: traps: Fix inconsistent faulting instruction skipping

[Robin Murphy]

On 18/10/2019 17:40, Dave Martin wrote:
> On Tue, Oct 15, 2019 at 05:49:05PM +0100, Dave Martin wrote:
>> On Tue, Oct 15, 2019 at 05:42:04PM +0100, Mark Rutland wrote:
>>> On Tue, Oct 15, 2019 at 04:21:09PM +0100, Dave Martin wrote:
>>>> On Fri, Oct 11, 2019 at 04:24:53PM +0100, Mark Rutland wrote:
>>>>> On Thu, Oct 10, 2019 at 07:44:37PM +0100, Dave Martin wrote:
>>>>>> Correct skipping of an instruction on AArch32 works a bit
>>>>>> differently from AArch64, mainly due to the different CPSR/PSTATE
>>>>>> semantics.
>>>>>>
>>>>>> There have been various attempts to get this right.  Currenty
>>>>>> arm64_skip_faulting_instruction() mostly does the right thing, but
>>>>>> does not advance the IT state machine for the AArch32 case.
>>>>>>
>>>>>> arm64_compat_skip_faulting_instruction() handles the IT state
>>>>>> machine but is local to traps.c, and porting other code to use it
>>>>>> will make a mess since there are some call sites that apply for
>>>>>> both the compat and native cases.
>>>>>>
>>>>>> Since manual instruction skipping implies a trap, it's a relatively
>>>>>> slow path.
>>>>>>
>>>>>> So, make arm64_skip_faulting_instruction() handle both compat and
>>>>>> native, and get rid of the arm64_compat_skip_faulting_instruction()
>>>>>> special case.
>>>>>>
>>>>>> Fixes: 32a3e635fb0e ("arm64: compat: Add CNTFRQ trap handler")
>>>>>> Fixes: 1f1c014035a8 ("arm64: compat: Add condition code checks and IT advance")
>>>>>> Fixes: 6436beeee572 ("arm64: Fix single stepping in kernel traps")
>>>>>> Fixes: bd35a4adc413 ("arm64: Port SWP/SWPB emulation support from arm")
>>>>>> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>>>>>> ---
>>>>>>   arch/arm64/kernel/traps.c | 18 ++++++++----------
>>>>>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>>>>
>>>>> This looks good to me; it's certainly easier to reason about.
>>>>>
>>>>> I couldn't spot a place where we do the wrong thing today, given AFAICT
>>>>> all the instances in arch/arm64/kernel/armv8_deprecated.c would be
>>>>> UNPREDICTABLE within an IT block.
>>>>>
>>>>> It might be worth calling out an example in the commit message to
>>>>> justify the fixes tags.
>>>>
>>>> IIRC I found no bug here; rather we have pointlessly fragmented code,
>>>> so I followed the "if fixing the same bug in multiple places, merge
>>>> those places so you need only fix it in one place next time" rule.
>>>
>>> Sure thing, that makes sense to me.
>>>
>>>> Since arm64_skip_faulting_instruction() is most of the way to being
>>>> generically usable anyway, this series merges all the special-case
>>>> handling into it.
>>>>
>>>> I could add something like
>>>>
>>>> --8<--
>>>>
>>>> This allows this fiddly operation to be maintained in a single
>>>> place rather than trying to maintain fragmented versions spread
>>>> around arch/arm64.
>>>>
>>>> -->8--
>>>>
>>>> Any good?
>>>
>>> My big concern is that the commit message reads as a fix, implying that
>>> there's an existing correctness bug. I think that simplifying it to make
>>> it clearer that it's a cleanup/improvement would be preferable.
>>>
>>> How about:
>>>
>>> | arm64: unify native/compat instruction skipping
>>> |
>>> | Skipping of an instruction on AArch32 works a bit differently from
>>> | AArch64, mainly due to the different CPSR/PSTATE semantics.
>>> |
>>> | Currently arm64_skip_faulting_instruction() is only suitable for
>>> | AArch64, and arm64_compat_skip_faulting_instruction() handles the IT
>>> | state machine but is local to traps.c.
>>> |
>>> | Since manual instruction skipping implies a trap, it's a relatively
>>> | slow path.
>>> |
>>> | So, make arm64_skip_faulting_instruction() handle both compat and
>>> | native, and get rid of the arm64_compat_skip_faulting_instruction()
>>> | special case.
>>> |
>>> | Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>>
>> And drop the Fixes tags.  Yes, I think that's reasonable.
>>
>> I think I was probably glossing over the fact that we don't need to get
>> the ITSTATE machine advance correct for the compat insn emulation; as
>> you say, I can't see what else this patch "fixes".
>>
>>> With that, feel free to add:
>>>
>>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>>
>> Thanks!
>>
>>> We could even point out that the armv8_deprecated cases are
>>> UNPREDICTABLE in an IT block, and correctly emulated either way.
>>
>> Yes, I'll add something along those lines.
> 
> Taking another look, I now can't track down where e.g., SWP in an IT
> block is specified to be UNPREDICTABLE.  I only see e.g.,
> ARM DDI 0487E.a Section 1.8.2 ("F1.8.2 Partial deprecation of IT"),
> which only deprecates the affected instructions.
> 
> The legacy AArch32 SWP{B} insn is obsoleted by ARMv8, but the whole
> point of the armv8_deprecated stuff is to provide some backwards
> compatiblity with v7.
> 
> 
> So, this needs a closer look.
> 
> I'll leave the Fixes tags for now, so that the archaeology doesn't need
> to redone if we decide that this patch does fix incorrect behaviour.

The Thumb encoding of SETEND is explicitly not allowed in an IT block, 
while a Thumb encoding of SWB{B} has never existed, so that's moot.

As far as I can see from DDI0406C.c, nothing prevents a Thumb MCR/MRC 
from being in an IT block (the ARM encodings are conditional, after all) 
so while they do fall under the performance deprecation, it would seem 
to be our bug if we don't already handle conditional CP15 barriers 
correctly.

Robin.