Re: [PATCH v26 22/30] x86/cet/shstk: Add user-mode shadow stack support

From: Yu, Yu-cheng
Date: Wed Apr 28 2021 - 14:39:08 EST

On 4/28/2021 10:52 AM, Borislav Petkov wrote:
On Tue, Apr 27, 2021 at 01:43:07PM -0700, Yu-cheng Yu wrote:
@@ -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.

+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!

+ 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.

+ 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@xxxxxxxxx

?


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

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

int r = vm_munmap...

+
+ /*
+ * 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.

+
+ 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.