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 = ¤t->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 = ¤t->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.
