On Tue, 2018-06-12 at 21:56 +1000, Balbir Singh wrote:

On 08/06/18 00:37, Yu-cheng Yu wrote:

quoted

This patch adds basic shadow stack enabling/disabling routines.
A task's shadow stack is allocated from memory with VM_SHSTK
flag set and read-only protection.  The shadow stack is
allocated to a fixed size and that can be changed by the system
admin.

I presume a read-only permission on the kernel side, but it
can be written by hardware?

Yes, the shadow stack is written by the processor when a call
instruction is executed.

...

quoted

diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
new file mode 100644
index 000000000000..9d5bc1efc9b7
--- /dev/null
+++ b/arch/x86/include/asm/cet.h

@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_CET_H
+#define _ASM_X86_CET_H
+
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+
+struct task_struct;
+/*
+ * Per-thread CET status
+ */
+struct cet_stat {

stat sounds like statistics, just expand out to status please

I will make it 'cet_status'.

quoted

+	unsigned long	shstk_base;
+	unsigned long	shstk_size;
+	unsigned int	shstk_enabled:1;
+};
+
+#ifdef CONFIG_X86_INTEL_CET
+unsigned long cet_get_shstk_ptr(void);

For the current task? Why does _ptr routine return an unsigned long?

What about cet_get_shstk_addr()?

...

quoted

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index fda2114197b3..428d13828ba9 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h

@@ -770,4 +770,18 @@
 #define MSR_VM_IGNNE                    0xc0010115
 #define MSR_VM_HSAVE_PA                 0xc0010117
 
+/* Control-flow Enforcement Technology MSRs */
+#define MSR_IA32_U_CET		0x6a0
+#define MSR_IA32_S_CET		0x6a2
+#define MSR_IA32_PL0_SSP	0x6a4
+#define MSR_IA32_PL3_SSP	0x6a7
+#define MSR_IA32_INT_SSP_TAB	0x6a8

some comments on the purpose of the MSR would be nice

Sure.

...

I think there was a comment about this being TASK_SIZE_MAX

quoted

+
+	rdmsrl(MSR_IA32_U_CET, r);
+	wrmsrl(MSR_IA32_U_CET, r | MSR_IA32_CET_SHSTK_EN);
+	wrmsrl(MSR_IA32_PL3_SSP, addr);

Should the enable happen before setting addr? I would expect to do this in the opposite order.

I will check.

quoted

+	return 0;
+}
+
+unsigned long cet_get_shstk_ptr(void)
+{
+	unsigned long ptr;
+
+	if (!current->thread.cet.shstk_enabled)
+		return 0;
+
+	rdmsrl(MSR_IA32_PL3_SSP, ptr);
+	return ptr;
+}
+
+static unsigned long shstk_mmap(unsigned long addr, unsigned long len)
+{
+	struct mm_struct *mm = current->mm;
+	unsigned long populate;
+
+	down_write(&mm->mmap_sem);
+	addr = do_mmap(NULL, addr, len, PROT_READ,
+		       MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK,
+		       0, &populate, NULL);
+	up_write(&mm->mmap_sem);

What happens if the mmap fails for any reason? I presume the caller disables shadow stack on this process?

This is from exec(), and that fails.

quoted

+
+	if (populate)
+		mm_populate(addr, populate);
+
+	return addr;
+}
+
+int cet_setup_shstk(void)
+{
+	unsigned long addr, size;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
+		return -EOPNOTSUPP;
+
+	size = SHSTK_SIZE;
+	addr = shstk_mmap(0, size);
+
+	if (addr >= TASK_SIZE)
+		return -ENOMEM;
+

TASK_SIZE_MAX?

Yes.

quoted

+	cet_set_shstk_ptr(addr + size - sizeof(void *));
+	current->thread.cet.shstk_base = addr;
+	current->thread.cet.shstk_size = size;
+	current->thread.cet.shstk_enabled = 1;
+	return 0;
+}
+
+void cet_disable_shstk(void)
+{
+	u64 r;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
+		return;
+
+	rdmsrl(MSR_IA32_U_CET, r);
+	r &= ~(MSR_IA32_CET_SHSTK_EN);
+	wrmsrl(MSR_IA32_U_CET, r);
+	wrmsrl(MSR_IA32_PL3_SSP, 0);

Again, I'd expect the order to be the reverse

quoted

+	current->thread.cet.shstk_enabled = 0;
+}
+
+void cet_disable_free_shstk(struct task_struct *tsk)
+{
+	if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
+	    !tsk->thread.cet.shstk_enabled)
+		return;
+
+	if (tsk == current)
+		cet_disable_shstk();
+
+	/*
+	 * Free only when tsk is current or shares mm
+	 * with current but has its own shstk.
+	 */
+	if (tsk->mm && (tsk->mm == current->mm) &&
+	    (tsk->thread.cet.shstk_base)) {

Does the caller hold a reference to tsk->mm?

If (tsk->mm == current->mm), i.e. it is current or it is a pthread of
current, then yes.

Yu-cheng

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html