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.