On Fri, 2018-08-31 at 15:16 -0700, Andy Lutomirski wrote:

On Fri, Aug 31, 2018 at 2:49 PM, Yu-cheng Yu [off-list ref] wrote:

quoted

On Thu, 2018-08-30 at 09:22 -0700, Yu-cheng Yu wrote:

quoted

On Thu, 2018-08-30 at 08:55 -0700, Andy Lutomirski wrote:

quoted

On Thu, Aug 30, 2018 at 8:39 AM, Jann Horn [off-list ref]
wrote:

quoted

On Thu, Aug 30, 2018 at 4:44 PM Yu-cheng Yu <yu-cheng.yu@intel.c
om

quoted

wrote:


WRUSS is a new kernel-mode instruction but writes directly
to user shadow stack memory.  This is used to construct
a return address on the shadow stack for the signal
handler.

This instruction can fault if the user shadow stack is
invalid shadow stack memory.  In that case, the kernel does
fixup.

Signed-off-by: Yu-cheng Yu <redacted>

[...]

quoted

+static inline int write_user_shstk_64(unsigned long addr,
unsigned long val)
+{
+       int err = 0;
+
+       asm volatile("1: wrussq %1, (%0)\n"
+                    "2:\n"
+                    _ASM_EXTABLE_HANDLE(1b, 2b,
ex_handler_wruss)
+                    :
+                    : "r" (addr), "r" (val));
+
+       return err;
+}

What's up with "err"? You set it to zero, and then you return
it,
but
nothing can ever set it to non-zero, right?

quoted

+__visible bool ex_handler_wruss(const struct
exception_table_entry *fixup,
+                               struct pt_regs *regs, int
trapnr)
+{
+       regs->ip = ex_fixup_addr(fixup);
+       regs->ax = -1;
+       return true;
+}

And here you just write into regs->ax, but your "asm volatile"
doesn't
reserve that register. This looks wrong to me.

I think you probably want to add something like an explicit
`"+&a"(err)` output to the asm statements.

We require asm goto support these days.  How about using
that?  You
won't even need a special exception handler.

Maybe something like this?  It looks simple now.

static inline int write_user_shstk_64(unsigned long addr, unsigned
long val)
{
        asm_volatile_goto("wrussq %1, (%0)\n"
                     "jmp %l[ok]\n"
                     ".section .fixup,\"ax\"n"
                     "jmp %l[fail]\n"
                     ".previous\n"
                     :: "r" (addr), "r" (val)
                     :: ok, fail);
ok:
        return 0;
fail:
        return -1;
}

I think you can get rid of 'jmp %l[ok]' and the ok label and just fall
through.  And you don't need an explicit jmp to fail -- just set the
_EX_HANDLER entry to land on the fail label.

Thanks!  This now looks simple and much better.

Yu-cheng



+static inline int write_user_shstk_64(unsigned long addr, unsigned long val)
+{
+	asm_volatile_goto("1: wrussq %1, (%0)\n"
+			  _ASM_EXTABLE(1b, %l[fail])
+			  :: "r" (addr), "r" (val)
+			  :: fail);
+	return 0;
+fail:
+	return -1;
+}