Thread (62 messages) 62 messages, 8 authors, 2016-03-08

Re: [PATCH v2] sparc64: Add support for Application Data Integrity (ADI)

From: Julian Calaby <hidden>
Date: 2016-03-02 23:08:49
Also in: linux-api, linux-mm, lkml, sparclinux

Hi Khalid,

On Thu, Mar 3, 2016 at 7:39 AM, Khalid Aziz [off-list ref] wrote:
Enable Application Data Integrity (ADI) support in the sparc
kernel for applications to use ADI in userspace. ADI is a new
feature supported on sparc M7 and newer processors. ADI is supported
for data fetches only and not instruction fetches. This patch adds
prctl commands to enable and disable ADI (TSTATE.mcde), return ADI
parameters to userspace, enable/disable MCD (Memory Corruption
Detection) on selected memory ranges and enable TTE.mcd in PTEs. It
also adds handlers for all traps related to MCD. ADI is not enabled
by default for any task and a task must explicitly enable ADI
(TSTATE.mcde), turn MCD on on a memory range and set version tag
for ADI to be effective for the task. This patch adds support for
ADI for hugepages only. Addresses passed into system calls must be
non-ADI tagged addresses.
I can't comment on the actual functionality here, but I do see a few
minor style issues in your patch.

My big concern is that you're defining a lot of new code that is ADI
specific but isn't inside a CONFIG_SPARC_ADI ifdef. (That said,
handling ADI specific traps if ADI isn't enabled looks like a good
idea to me, however most of the other stuff is just dead code if
CONFIG_SPARC_ADI isn't enabled.)
Signed-off-by: Khalid Aziz <redacted>
---
NOTES: ADI is a new feature added to M7 processor to allow hardware
        to catch rogue accesses to memory. An app can enable ADI on
        its data pages, set version tags on them and use versioned
        addresses (bits 63-60 of the address contain a version tag)
        to access the data pages. If a rogue app attempts to access
        ADI enabled data pages, its access is blocked and processor
        generates an exception. Enabling this functionality for all
        data pages of an app requires adding infrastructure to save
        version tags for any data pages that get swapped out and
        restoring those tags when pages are swapped back in. In this
        first implementation I am enabling ADI for hugepages only
        since these pages are locked in memory and hence avoid the
        issue of saving and restoring tags. Once this core functionality
        is stable, ADI for other memory pages can be enabled more
        easily.

v2:
        - Fixed a build error

 Documentation/prctl/sparc_adi.txt     |  62 ++++++++++
 Documentation/sparc/adi.txt           | 206 +++++++++++++++++++++++++++++++
 arch/sparc/Kconfig                    |  12 ++
 arch/sparc/include/asm/hugetlb.h      |  14 +++
 arch/sparc/include/asm/hypervisor.h   |   2 +
 arch/sparc/include/asm/mmu_64.h       |   1 +
 arch/sparc/include/asm/pgtable_64.h   |  15 +++
 arch/sparc/include/asm/processor_64.h |  19 +++
 arch/sparc/include/asm/ttable.h       |  10 ++
 arch/sparc/include/uapi/asm/asi.h     |   3 +
 arch/sparc/include/uapi/asm/pstate.h  |  10 ++
 arch/sparc/kernel/entry.h             |   3 +
 arch/sparc/kernel/head_64.S           |   1 +
 arch/sparc/kernel/mdesc.c             |  81 +++++++++++++
 arch/sparc/kernel/process_64.c        | 222 ++++++++++++++++++++++++++++++++++
 arch/sparc/kernel/sun4v_mcd.S         |  16 +++
 arch/sparc/kernel/traps_64.c          |  96 ++++++++++++++-
 arch/sparc/kernel/ttable_64.S         |   6 +-
 include/linux/mm.h                    |   2 +
 include/uapi/asm-generic/siginfo.h    |   5 +-
 include/uapi/linux/prctl.h            |  16 +++
 kernel/sys.c                          |  30 +++++
 22 files changed, 826 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/prctl/sparc_adi.txt
 create mode 100644 Documentation/sparc/adi.txt
 create mode 100644 arch/sparc/kernel/sun4v_mcd.S
I must admit that I'm slightly impressed that the documentation is
over a quarter of the lines added. =)
quoted hunk ↗ jump to hunk
diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 56442d2..0aac0ae 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -80,6 +80,7 @@ config SPARC64
        select NO_BOOTMEM
        select HAVE_ARCH_AUDITSYSCALL
        select ARCH_SUPPORTS_ATOMIC_RMW
+       select SPARC_ADI
This doesn't look right.
quoted hunk ↗ jump to hunk
 config ARCH_DEFCONFIG
        string
@@ -314,6 +315,17 @@ if SPARC64
 source "kernel/power/Kconfig"
 endif

+config SPARC_ADI
+       bool "Application Data Integrity support"
+       def_bool y if SPARC64
def_bool is for config options without names (i.e. "this is a boolean
value and it's default is...")

So if you want people to be able to disable this option, then you
should remove the select above and just have:

bool "Application Data Integrity support"
default y if SPARC64

If you don't want people disabling it, then there's no point in having
a separate Kconfig symbol.
+       help
+         Support for Application Data Integrity (ADI). ADI feature allows
+         a process to tag memory blocks with version tags. Once ADI is
+         enabled and version tag is set on a memory block, any access to
+         it is allowed only if the correct version tag is presented by
+         a process. This feature is meant to help catch rogue accesses
+         to memory.
+
You should probably mention that it's only available on newer
processors and recommend that it's enabled on them.

This code won't break anything on older processors, right? I haven't
looked very closely, but I don't see anything that specifically
disables the code if it's run on, say, a UltraSparc I.
quoted hunk ↗ jump to hunk
 config SCHED_SMT
        bool "SMT (Hyperthreading) scheduler support"
        depends on SPARC64 && SMP
diff --git a/arch/sparc/include/asm/processor_64.h b/arch/sparc/include/asm/processor_64.h
index 6924bde..9a71701 100644
--- a/arch/sparc/include/asm/processor_64.h
+++ b/arch/sparc/include/asm/processor_64.h
@@ -97,6 +97,25 @@ struct thread_struct {
 struct task_struct;
 unsigned long thread_saved_pc(struct task_struct *);

+#ifdef CONFIG_SPARC_ADI
+extern struct adi_caps *get_adi_caps(void);
+extern long get_sparc_adicaps(unsigned long);
+extern long set_sparc_pstate_mcde(unsigned long);
+extern long enable_sparc_adi(unsigned long, unsigned long);
+extern long disable_sparc_adi(unsigned long, unsigned long);
+extern long get_sparc_adi_status(unsigned long);
+extern bool adi_capable(void);
+
+#define GET_SPARC_ADICAPS(a)   get_sparc_adicaps(a)
+#define SET_SPARC_MCDE(a)      set_sparc_pstate_mcde(a)
+#define ENABLE_SPARC_ADI(a, b) enable_sparc_adi(a, b)
+#define DISABLE_SPARC_ADI(a, b)        disable_sparc_adi(a, b)
+#define GET_SPARC_ADI_STATUS(a)        get_sparc_adi_status(a)
+#define ADI_CAPABLE()          adi_capable()
Get rid of the ADI_CAPABLE macro, the usual pattern here is to define
a static inline function for the entire API when the symbol is
disabled, i.e.

#ifdef CONFIG_SPARC_ADI
...
extern bool adi_capable(void);
#else
...
static inline bool adi_capable(void) {
    return false;
}
#endif

That way you get type checking on the arguments even if the option is
disabled and modern compilers are smart enough to optimise all the
no-op code away. (Not that the type checking is needed here.)

Also, in all but one place you use the ADI_CAPABLE() macro when the
adi_capable() function is defined and available.
quoted hunk ↗ jump to hunk
+#else
+#define ADI_CAPABLE()          0
+#endif
+
 /* On Uniprocessor, even in RMO processes see TSO semantics */
 #ifdef CONFIG_SMP
 #define TSTATE_INITIAL_MM      TSTATE_TSO
diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index 6f80936..79f981c 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -1007,6 +1013,80 @@ static int mdesc_open(struct inode *inode, struct file *file)
        return 0;
 }

+bool adi_capable(void)
+{
+       return adi_state.enabled;
+}
+
+struct adi_caps *get_adi_caps(void)
+{
+       return &adi_state.caps;
+}
+
+void __init
+init_adi(void)
+{
+       struct mdesc_handle *hp = mdesc_grab();
+       const char *prop;
+       u64 pn, *val;
+       int len;
+
+       adi_state.enabled = false;
+
+       if (!hp)
+               return;
+
+       pn = mdesc_node_by_name(hp, MDESC_NODE_NULL, "cpu");
+       if (pn == MDESC_NODE_NULL)
+               goto out;
+
+       prop = mdesc_get_property(hp, pn, "hwcap-list", &len);
+       if (!prop)
+               goto out;
+
+       /*
+        * Look for "adp" keyword in hwcap-list which would indicate
+        * ADI support
+        */
+       while (len) {
+               int plen;
+
+               if (!strcmp(prop, "adp")) {
+                       adi_state.enabled = true;
+                       break;
+               }
+
+               plen = strlen(prop) + 1;
+               prop += plen;
+               len -= plen;
+       }
+
+       if (!adi_state.enabled)
+               goto out;
+
+       pn = mdesc_node_by_name(hp, MDESC_NODE_NULL, "platform");
+       if (pn == MDESC_NODE_NULL)
+               goto out;
+
+       val = (u64 *) mdesc_get_property(hp, pn, "adp-blksz", &len);
+       if (!val)
+               goto out;
+       adi_state.caps.blksz = *val;
+
+       val = (u64 *) mdesc_get_property(hp, pn, "adp-nbits", &len);
+       if (!val)
+               goto out;
+       adi_state.caps.nbits = *val;
+
+       val = (u64 *) mdesc_get_property(hp, pn, "ue-on-adp", &len);
+       if (!val)
+               goto out;
+       adi_state.caps.ue_on_adi = *val;
+
+out:
+       mdesc_release(hp);
+}
+
Should all the ADI related functions above be within a #ifdef CONFIG_SPARC_ADI?
quoted hunk ↗ jump to hunk
 static ssize_t mdesc_read(struct file *file, char __user *buf,
                          size_t len, loff_t *offp)
 {
diff --git a/arch/sparc/kernel/traps_64.c b/arch/sparc/kernel/traps_64.c
index d21cd62..29db583 100644
--- a/arch/sparc/kernel/traps_64.c
+++ b/arch/sparc/kernel/traps_64.c
@@ -2531,6 +2589,38 @@ void sun4v_do_mna(struct pt_regs *regs, unsigned long addr, unsigned long type_c
        force_sig_info(SIGBUS, &info, current);
 }

+void sun4v_mem_corrupt_detect_precise(struct pt_regs *regs, unsigned long addr,
+                                     unsigned long context)
+{
+       siginfo_t info;
+
+       if (!ADI_CAPABLE()) {
+               bad_trap(regs, 0x1a);
+               return;
+       }
+
+       if (notify_die(DIE_TRAP, "memory corruption precise exception", regs,
+                      0, 0x8, SIGSEGV) == NOTIFY_STOP)
+               return;
+
+       if (regs->tstate & TSTATE_PRIV) {
+               pr_emerg("sun4v_mem_corrupt_detect_precise: ADDR[%016lx] "
+                       "CTX[%lx], going.\n", addr, context);
+               die_if_kernel("MCD precise", regs);
+       }
+
+       if (test_thread_flag(TIF_32BIT)) {
+               regs->tpc &= 0xffffffff;
+               regs->tnpc &= 0xffffffff;
+       }
+       info.si_signo = SIGSEGV;
+       info.si_code = SEGV_ADIPERR;
+       info.si_errno = 0;
+       info.si_addr = (void __user *) addr;
+       info.si_trapno = 0;
+       force_sig_info(SIGSEGV, &info, current);
+}
+
Should this be ifdef'd too?
quoted hunk ↗ jump to hunk
 void do_privop(struct pt_regs *regs)
 {
        enum ctx_state prev_state = exception_enter();
diff --git a/kernel/sys.c b/kernel/sys.c
index 6af9212..fa7b5d9 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -103,6 +103,21 @@
 #ifndef SET_FP_MODE
 # define SET_FP_MODE(a,b)      (-EINVAL)
 #endif
+#ifndef GET_SPARC_ADICAPS
+# define GET_SPARC_ADICAPS(a)          (-EINVAL)
+#endif
+#ifndef SET_SPARC_MCDE
+# define SET_SPARC_MCDE(a)             (-EINVAL)
+#endif
+#ifndef ENABLE_SPARC_ADI
+# define ENABLE_SPARC_ADI(a, b)                (-EINVAL)
+#endif
+#ifndef DISABLE_SPARC_ADI
+# define DISABLE_SPARC_ADI(a, b)       (-EINVAL)
+#endif
+#ifndef GET_SPARC_ADI_STATUS
+# define GET_SPARC_ADI_STATUS(a)       (-EINVAL)
+#endif
Ah, I was wondering why you were defining macros in processor_64.h.
 /*
  * this is where the system-wide overflow UID and GID are defined, for
I've got a couple more comments, I'll send another email with them shortly.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help