On 4/28/2021 10:52 AM, Borislav Petkov wrote:

On Tue, Apr 27, 2021 at 01:43:07PM -0700, Yu-cheng Yu wrote:

quoted

@@ -535,6 +536,10 @@ struct thread_struct {
  
  	unsigned int		sig_on_uaccess_err:1;
  
+#ifdef CONFIG_X86_SHADOW_STACK
+	struct cet_status	cet;

A couple of versions ago I said:

"	struct shstk_desc       shstk;

or so"

but no movement here. That thing is still called cet_status even though
there's nothing status-related with it.

So what's up?

Sorry about that.  After that email thread, we went ahead to separate 
shadow stack and ibt into different files.  I thought about the struct, 
the file names cet.h, etc.  The struct still needs to include ibt 
status, and if it is shstk_desc, the name is not entirely true.  One 
possible approach is, we don't make it a struct here, and put every item 
directly in thread_struct.  However, the benefit of putting all in a 
struct is understandable (you might argue the opposite :-)).  Please 
make the call, and I will do the change.

quoted

+static unsigned long alloc_shstk(unsigned long size)
+{
+	struct mm_struct *mm = current->mm;
+	unsigned long addr, populate;
+	int flags = MAP_ANONYMOUS | MAP_PRIVATE;

The tip-tree preferred ordering of variable declarations at the
beginning of a function is reverse fir tree order::

	struct long_struct_name *descriptive_name;
	unsigned long foo, bar;
	unsigned int tmp;
	int ret;

The above is faster to parse than the reverse ordering::

	int ret;
	unsigned int tmp;
	unsigned long foo, bar;
	struct long_struct_name *descriptive_name;

And even more so than random ordering::

	unsigned long foo, bar;
	int ret;
	struct long_struct_name *descriptive_name;
	unsigned int tmp;

Please fix it up everywhere.

Ok!

quoted

+	mmap_write_lock(mm);
+	addr = do_mmap(NULL, 0, size, PROT_READ, flags, VM_SHADOW_STACK, 0,
+		       &populate, NULL);
+	mmap_write_unlock(mm);
+
+	return addr;
+}
+
+int shstk_setup(void)
+{
+	unsigned long addr, size;
+	struct cet_status *cet = &current->thread.cet;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
+		return -EOPNOTSUPP;
+
+	size = round_up(min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G), PAGE_SIZE);
+	addr = alloc_shstk(size);
+	if (IS_ERR_VALUE(addr))
+		return PTR_ERR((void *)addr);
+
+	cet->shstk_base = addr;
+	cet->shstk_size = size;
+
+	start_update_msrs();
+	wrmsrl(MSR_IA32_PL3_SSP, addr + size);
+	wrmsrl(MSR_IA32_U_CET, CET_SHSTK_EN);
+	end_update_msrs();

<---- newline here.

quoted

+	return 0;
+}
+
+void shstk_free(struct task_struct *tsk)
+{
+	struct cet_status *cet = &tsk->thread.cet;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
+	    !cet->shstk_size ||
+	    !cet->shstk_base)
+		return;
+
+	if (!tsk->mm)
+		return;

Where are the comments you said you wanna add:

https://lkml.kernel.org/r/b05ee7eb-1b5d-f84f-c8f3-bfe9426e8a7d@intel.com

?

Yes, the comments are in patch #23: Handle thread shadow stack.  I 
wanted to add that in the patch that takes the path.

quoted

+
+	while (1) {
+		int r;
+
+		r = vm_munmap(cet->shstk_base, cet->shstk_size);

		int r = vm_munmap...

quoted

+
+		/*
+		 * vm_munmap() returns -EINTR when mmap_lock is held by
+		 * something else, and that lock should not be held for a
+		 * long time.  Retry it for the case.
+		 */
+		if (r == -EINTR) {
+			cond_resched();
+			continue;
+		}
+		break;
+	}

vm_munmap() can return other negative error values, where are you
handling those?

For other error values, the loop stops.

quoted

+
+	cet->shstk_base = 0;
+	cet->shstk_size = 0;
+}
+
+void shstk_disable(void)
+{
+	struct cet_status *cet = &current->thread.cet;

Same question as before: what guarantees that current doesn't change
from under you here?

The actual reading/writing MSRs are protected by fpregs_lock().

One of the worst thing to do is to ignore review comments. I'd strongly
suggest you pay more attention and avoid that in the future.

Thx.