On Tue, Aug 20, 2024 at 09:36:46PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2024-08-19 at 20:24 +0100, Mark Brown wrote:
> > + /* This should really be an atomic cmpxchg. It is not. */
> > + copy_from_user_page(vma, page, addr, &val, maddr + offset,
> > + sizeof(val));
> Were so close to the real cmpxchg at this point. I took a shot at it with the
> diff at the end. I'm not sure if it might need some of the instrumentation
> calls.
Great - I hadn't been sure if there was any fun with access from kernel
mode on x86. I can't get that patch to apply cleanly FWIW:
patching file arch/x86/kernel/shstk.c
Hunk #1 FAILED at 204.
patch: **** malformed patch at line 24: offset),
I think I got everything integrated correctly, I should have a version
with that folded in out tomorrow.
> > +
> > + if (val != expected)
> > + return false;
> Return false for an int will be 0 (i.e. success). I think it might be covering
> up a bug. The gup happens to args->shadow_stack + args->shadow_stack_size - 1
> (the size inclusive). But the copy happens at the size exclusive.
Ah, yeah, thanks for noticing - that's cut'n'paste from the arm64 code
where the token check is in a separate function.
> > + if (!(vma->vm_flags & VM_SHADOW_STACK)) {
> Can we check VM_WRITE here too? At least on x86, shadow stacks can be
> mprotect()ed as read-only. The reason for this before I think fell out of the
> implementation details, but all the same it would be nice be consistent. Then it
> should behave identically to a real shadow stack access.
Seems reasonable.
> > + if (kargs->shadow_stack_size < SHADOW_STACK_SIZE_MIN)
> > + return false;
> > + if (kargs->shadow_stack_size > rlimit(RLIMIT_STACK))
> > + return false;
> At the risk of asking a stupid question or one that I should have asked a long
> time ago...
>
> Why do we need both shadow_stack and shadow_stack_size? We are basically asking
> it to consume a token at a pointer and have userspace manage the shadow stack
> itself. So why does the kernel care what size it is? Couldn't we just have
> 'shadow_stack' have that mean consume a token here.
I was doing things this way for symmetry with how we specify the normal
stack. That's a bit different since the kernel will actually use the
size for the normal stack but it felt nicer to keep things looking
consistent, it saves users wondering why they work differently. It's
also a bit of a help with portability given that arm64 expects to have a
top of stack marker above the token by default while x86 doesn't support
that.
Attachment:
signature.asc
Description: PGP signature
