| Message ID | 20240507105321.71524-7-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | BPF |
| Headers | show |
| Series | uprobe: uretprobe speed up | expand |
On Tue, 2024-05-07 at 12:53 +0200, Jiri Olsa wrote: > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index 81e6ee95784d..ae6c3458a675 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c > @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe) > * trampoline's ret instruction > */ > r11_cx_ax[2] = regs->ip; > + > + /* make the shadow stack follow that */ > + if (shstk_push_frame(regs->ip)) > + goto sigill; > + > regs->ip = ip; > Per the earlier discussion, this cannot be reached unless uretprobes are in use, which cannot happen without something with privileges taking an action. But are uretprobes ever used for monitoring applications where security is important? Or is it strictly a debug-time thing?
On Tue, May 07, 2024 at 05:35:54PM +0000, Edgecombe, Rick P wrote: > On Tue, 2024-05-07 at 12:53 +0200, Jiri Olsa wrote: > > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > > index 81e6ee95784d..ae6c3458a675 100644 > > --- a/arch/x86/kernel/uprobes.c > > +++ b/arch/x86/kernel/uprobes.c > > @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe) > > * trampoline's ret instruction > > */ > > r11_cx_ax[2] = regs->ip; > > + > > + /* make the shadow stack follow that */ > > + if (shstk_push_frame(regs->ip)) > > + goto sigill; > > + > > regs->ip = ip; > > > > Per the earlier discussion, this cannot be reached unless uretprobes are in use, > which cannot happen without something with privileges taking an action. But are > uretprobes ever used for monitoring applications where security is important? Or > is it strictly a debug-time thing? sorry, I don't have that level of detail, but we do have customers that use uprobes in general or want to use it and complain about the speed there are several tools in bcc [1] that use uretprobes in scripts, like: memleak, sslsniff, trace, bashreadline, gethostlatency, argdist, funclatency jirka [1] https://github.com/iovisor/bcc
On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote: > > Per the earlier discussion, this cannot be reached unless uretprobes are in > > use, > > which cannot happen without something with privileges taking an action. But > > are > > uretprobes ever used for monitoring applications where security is > > important? Or > > is it strictly a debug-time thing? > > sorry, I don't have that level of detail, but we do have customers > that use uprobes in general or want to use it and complain about > the speed > > there are several tools in bcc [1] that use uretprobes in scripts, > like: > memleak, sslsniff, trace, bashreadline, gethostlatency, argdist, > funclatency Is it possible to have shadow stack only use the non-syscall solution? It seems it exposes a more limited compatibility in that it only allows writing the specific trampoline address. (IIRC) Then shadow stack users could still use uretprobes, but just not the new optimized solution. There are already operations that are slower with shadow stack, like longjmp(), so this could be ok maybe.
On Thu, May 09, 2024 at 04:24:37PM +0000, Edgecombe, Rick P wrote: > On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote: > > > Per the earlier discussion, this cannot be reached unless uretprobes are in > > > use, > > > which cannot happen without something with privileges taking an action. But > > > are > > > uretprobes ever used for monitoring applications where security is > > > important? Or > > > is it strictly a debug-time thing? > > > > sorry, I don't have that level of detail, but we do have customers > > that use uprobes in general or want to use it and complain about > > the speed > > > > there are several tools in bcc [1] that use uretprobes in scripts, > > like: > > memleak, sslsniff, trace, bashreadline, gethostlatency, argdist, > > funclatency > > Is it possible to have shadow stack only use the non-syscall solution? It seems > it exposes a more limited compatibility in that it only allows writing the > specific trampoline address. (IIRC) Then shadow stack users could still use > uretprobes, but just not the new optimized solution. There are already > operations that are slower with shadow stack, like longjmp(), so this could be > ok maybe. I guess it's doable, we'd need to keep both trampolines around, because shadow stack is enabled by app dynamically and use one based on the state of shadow stack when uretprobe is installed so you're worried the optimized syscall path could be somehow exploited to add data on shadow stack? jirka
On Sat, 11 May 2024 15:09:48 -0600 Jiri Olsa <olsajiri@gmail.com> wrote: > On Thu, May 09, 2024 at 04:24:37PM +0000, Edgecombe, Rick P wrote: > > On Thu, 2024-05-09 at 10:30 +0200, Jiri Olsa wrote: > > > > Per the earlier discussion, this cannot be reached unless uretprobes are in > > > > use, > > > > which cannot happen without something with privileges taking an action. But > > > > are > > > > uretprobes ever used for monitoring applications where security is > > > > important? Or > > > > is it strictly a debug-time thing? > > > > > > sorry, I don't have that level of detail, but we do have customers > > > that use uprobes in general or want to use it and complain about > > > the speed > > > > > > there are several tools in bcc [1] that use uretprobes in scripts, > > > like: > > > memleak, sslsniff, trace, bashreadline, gethostlatency, argdist, > > > funclatency > > > > Is it possible to have shadow stack only use the non-syscall solution? It seems > > it exposes a more limited compatibility in that it only allows writing the > > specific trampoline address. (IIRC) Then shadow stack users could still use > > uretprobes, but just not the new optimized solution. There are already > > operations that are slower with shadow stack, like longjmp(), so this could be > > ok maybe. > > I guess it's doable, we'd need to keep both trampolines around, because > shadow stack is enabled by app dynamically and use one based on the > state of shadow stack when uretprobe is installed > > so you're worried the optimized syscall path could be somehow exploited > to add data on shadow stack? Good point. For the security concerning (e.g. leaking sensitive information from secure process which uses shadow stack), we need another limitation which prohibits probing such process even for debugging. But I think that needs another series of patches. We also need to discuss when it should be prohibited and how (e.g. audit interface? SELinux?). But I think this series is just optimizing currently available uprobes with a new syscall. I don't think it changes such security concerning. Thank you, > > jirka
On Mon, 2024-05-13 at 18:50 +0900, Masami Hiramatsu wrote: > > I guess it's doable, we'd need to keep both trampolines around, because > > shadow stack is enabled by app dynamically and use one based on the > > state of shadow stack when uretprobe is installed > > > > so you're worried the optimized syscall path could be somehow exploited > > to add data on shadow stack? Shadow stack allows for modification to the shadow stack only through a few limited ways (call, ret, etc). The kernel has the ability to write through shadow stack protections (for example when pushing and popping signal frames), but the ways in which it does this are limited in order to try to prevent providing extra capabilities to attackers wanting to craft their own shadow stacks. But the HW features have optional abilities to allow extra patterns of shadow stack modification for userspace as well. This can facilitate unusual patterns of stack modification (like in this series). For, x86 there is the ability to allow an instruction (called WRSS) such that userspace can also write arbitrary data to the shadow stack. Arm has something likes that, plus an instruction to push to the shadow stack. There was some debate about whether to use these features, as glibc could not perfectly match compatibility for features that play with the stack like longjmp(). As in, without using those extra HW capabilities, some apps would require modifications to work with shadow stack. There has been a lot of design tension between security, performance and compatibility in figuring out how to fit this feature into existing software. In the end the consensus was to not use these extra HW capabilities, and lean towards security in the implementation. To try to summarize the debate, this was because we could get pretty close to compatibility without enabling these extra features. So since this solution does something like enabling these extra capabilities in software that were purposely disabled in HW, it raises eyebrows. Glibc has some operations that now have extra steps because of shadow stack. So if we could do something that was still functional, but slower and more secure, then it seems roughly in line with the tradeoffs we have gone with so far. But shadow stack is not in widespread use yet, so whether we have the final tradeoffs settled is still open I think. For example, other libcs have expressed interest in using WRSS. I'm also not clear on the typical use of uretprobes (debugging vs production). And whether shadow stack + debugging + production will happen seems pretty unknown. > > Good point. For the security concerning (e.g. leaking sensitive information > from secure process which uses shadow stack), we need another limitation > which prohibits probing such process even for debugging. But I think that > needs another series of patches. We also need to discuss when it should be > prohibited and how (e.g. audit interface? SELinux?). > But I think this series is just optimizing currently available uprobes with > a new syscall. I don't think it changes such security concerning. Patch 6 adds support for shadow stack for uretprobes. Currently there is no support. Peterz had asked that the new solution consider shadow stack support, so I think that is how this series grew kind of two goals: new faster uretprobes and initial shadow stack support.
On Mon, May 13, 2024 at 05:12:31PM +0000, Edgecombe, Rick P wrote: > On Mon, 2024-05-13 at 18:50 +0900, Masami Hiramatsu wrote: > > > I guess it's doable, we'd need to keep both trampolines around, because > > > shadow stack is enabled by app dynamically and use one based on the > > > state of shadow stack when uretprobe is installed > > > > > > so you're worried the optimized syscall path could be somehow exploited > > > to add data on shadow stack? > > Shadow stack allows for modification to the shadow stack only through a few > limited ways (call, ret, etc). The kernel has the ability to write through > shadow stack protections (for example when pushing and popping signal frames), > but the ways in which it does this are limited in order to try to prevent > providing extra capabilities to attackers wanting to craft their own shadow > stacks. > > But the HW features have optional abilities to allow extra patterns of shadow > stack modification for userspace as well. This can facilitate unusual patterns > of stack modification (like in this series). For, x86 there is the ability to > allow an instruction (called WRSS) such that userspace can also write arbitrary > data to the shadow stack. Arm has something likes that, plus an instruction to > push to the shadow stack. > > There was some debate about whether to use these features, as glibc could not > perfectly match compatibility for features that play with the stack like > longjmp(). As in, without using those extra HW capabilities, some apps would > require modifications to work with shadow stack. > > There has been a lot of design tension between security, performance and > compatibility in figuring out how to fit this feature into existing software. In > the end the consensus was to not use these extra HW capabilities, and lean > towards security in the implementation. To try to summarize the debate, this was > because we could get pretty close to compatibility without enabling these extra > features. > > So since this solution does something like enabling these extra capabilities in > software that were purposely disabled in HW, it raises eyebrows. Glibc has some > operations that now have extra steps because of shadow stack. So if we could do > something that was still functional, but slower and more secure, then it seems > roughly in line with the tradeoffs we have gone with so far. so at the moment the patch 6 changes shadow stack for 1) current uretprobe which are not working at the moment and we change the top value of shadow stack with shstk_push_frame 2) optimized uretprobe which needs to push new frame on shadow stack with shstk_update_last_frame I think we should do 1) and have current uretprobe working with shadow stack, which is broken at the moment I'm ok with not using optimized uretprobe when shadow stack is detected as enabled and we go with current uretprobe in that case would this work for you? thanks, jirka > > But shadow stack is not in widespread use yet, so whether we have the final > tradeoffs settled is still open I think. For example, other libcs have expressed > interest in using WRSS. > > I'm also not clear on the typical use of uretprobes (debugging vs production). > And whether shadow stack + debugging + production will happen seems pretty > unknown. > > > > > Good point. For the security concerning (e.g. leaking sensitive information > > from secure process which uses shadow stack), we need another limitation > > which prohibits probing such process even for debugging. But I think that > > needs another series of patches. We also need to discuss when it should be > > prohibited and how (e.g. audit interface? SELinux?). > > But I think this series is just optimizing currently available uprobes with > > a new syscall. I don't think it changes such security concerning. > > Patch 6 adds support for shadow stack for uretprobes. Currently there is no > support. > > Peterz had asked that the new solution consider shadow stack support, so I think > that is how this series grew kind of two goals: new faster uretprobes and > initial shadow stack support. > >
On Mon, 2024-05-13 at 15:23 -0600, Jiri Olsa wrote: > so at the moment the patch 6 changes shadow stack for > > 1) current uretprobe which are not working at the moment and we change > the top value of shadow stack with shstk_push_frame > 2) optimized uretprobe which needs to push new frame on shadow stack > with shstk_update_last_frame > > I think we should do 1) and have current uretprobe working with shadow > stack, which is broken at the moment > > I'm ok with not using optimized uretprobe when shadow stack is detected > as enabled and we go with current uretprobe in that case > > would this work for you? Sorry for the delay. It seems reasonable to me due to 1 being at a fixed address where 2 was arbitrary address. But Peterz might have felt the opposite earlier. Not sure. I'd also love to get some second opinions from broonie (arm shadow stack) and Deepak (riscv shadow stack). Deepak, even if riscv has a special instruction that pushes to the shadow stack, will it be ok if there is a callable operation that does the same thing? Like, aren't you relying on endbranches or the compiler or something such that arbitrary data can't be pushed via that instruction? BTW Jiri, thanks for considering shadow stack in your work.
On Wed, May 15, 2024 at 01:10:03AM +0000, Edgecombe, Rick P wrote: >On Mon, 2024-05-13 at 15:23 -0600, Jiri Olsa wrote: >> so at the moment the patch 6 changes shadow stack for >> >> 1) current uretprobe which are not working at the moment and we change >> the top value of shadow stack with shstk_push_frame >> 2) optimized uretprobe which needs to push new frame on shadow stack >> with shstk_update_last_frame >> >> I think we should do 1) and have current uretprobe working with shadow >> stack, which is broken at the moment >> >> I'm ok with not using optimized uretprobe when shadow stack is detected >> as enabled and we go with current uretprobe in that case >> >> would this work for you? > >Sorry for the delay. It seems reasonable to me due to 1 being at a fixed address >where 2 was arbitrary address. But Peterz might have felt the opposite earlier. >Not sure. > >I'd also love to get some second opinions from broonie (arm shadow stack) and >Deepak (riscv shadow stack). > >Deepak, even if riscv has a special instruction that pushes to the shadow stack, >will it be ok if there is a callable operation that does the same thing? Like, >aren't you relying on endbranches or the compiler or something such that >arbitrary data can't be pushed via that instruction? Instruction is `sspush x1/ra`. It pushes contents of register return address (ra also called x1) onto shadow stack. `ra` is like arm's equivalent of link register. Prologue of function is supposed to have `sspush x1` to save it away. ISA doesn't allow encodings with register in risc-v GPRs (except register x5 because some embedded riscv space toolchains have used x5 as ra too). On question of callable operation, I think still need to fully understand who manages the probe and forward progress. Question, Is it kernel who is maintaining all return probes, meaning original return addresses are saved in kernel data structures on per task basis. Once uretprobe did its job then its kernel who is ensuring return to original return address ? > >BTW Jiri, thanks for considering shadow stack in your work.
Sorry for the late reply, I was on PTO. On 05/14, Deepak Gupta wrote: > > Question, > > Is it kernel who is maintaining all return probes, meaning original return addresses > are saved in kernel data structures on per task basis. Yes. task_struct->utask->return_instances See prepare_uretprobe() which inserts the new return_instance with ->orig_ret_vaddr = original return addresses when the tracee enters the ret-probed function. > Once uretprobe did its job then > its kernel who is ensuring return to original return address ? Yes. See instruction_pointer_set(regs, ri->orig_ret_vaddr) in handle_trampoline(). I know absolutely nothing about the shadow stacks, trying to read Documentation/arch/x86/shstk.rst but it doesn't tell me too much... Where can I find more documentation? I didn't try to google yet. Upon function return, the processor pops the shadow stack copy and compares it to the normal stack copy. If the two differ, the processor raises a control-protection fault. grep-grep-grep... exc_control_protection I guess. Let me ask a couple of really stupid questions. What if the shadow stack is "shorter" than the normal stack? I mean, enable_shstk() { prctl(ARCH_SHSTK_SHSTK); } what happens when enable_shstk() returns? And what is the purpose of fpregs_lock_and_load() ? Why do we need to fpregs_restore_userregs() in shstk_setup() and other places? Oleg.
Let me repeat I know nothing about shadow stacks, only tried to read Documentation/arch/x86/shstk.rst few minutes ago ;) On 05/13, Jiri Olsa wrote: > > 1) current uretprobe which are not working at the moment and we change > the top value of shadow stack with shstk_push_frame > 2) optimized uretprobe which needs to push new frame on shadow stack > with shstk_update_last_frame > > I think we should do 1) and have current uretprobe working with shadow > stack, which is broken at the moment Agreed, > I'm ok with not using optimized uretprobe when shadow stack is detected > as enabled and we go with current uretprobe in that case But how can we detect it? Again, suppose userspace does enable_shstk() { prctl(ARCH_SHSTK_SHSTK); } what if enable_shstk() is ret-probed ? Oleg.
On Wed, May 15, 2024 at 01:19:20PM +0200, Oleg Nesterov wrote: > Sorry for the late reply, I was on PTO. > > On 05/14, Deepak Gupta wrote: > > > > Question, > > > > Is it kernel who is maintaining all return probes, meaning original return addresses > > are saved in kernel data structures on per task basis. > > Yes. task_struct->utask->return_instances > > See prepare_uretprobe() which inserts the new return_instance with > ->orig_ret_vaddr = original return addresses > when the tracee enters the ret-probed function. > > > Once uretprobe did its job then > > its kernel who is ensuring return to original return address ? > > Yes. See instruction_pointer_set(regs, ri->orig_ret_vaddr) in > handle_trampoline(). > > > > I know absolutely nothing about the shadow stacks, trying to read > Documentation/arch/x86/shstk.rst but it doesn't tell me too much... > Where can I find more documentation? I didn't try to google yet. > > Upon function return, the processor pops the shadow stack copy > and compares it to the normal stack copy. If the two differ, the > processor raises a control-protection fault. > > grep-grep-grep... exc_control_protection I guess. > > Let me ask a couple of really stupid questions. What if the shadow stack > is "shorter" than the normal stack? I mean, > > enable_shstk() > { > prctl(ARCH_SHSTK_SHSTK); > } > > what happens when enable_shstk() returns? I think it will crash, there's explanation in the comment in tools/testing/selftests/x86/test_shadow_stack.c test that's why ARCH_PRCTL is using syscall instruction directly and not calling syscall function jirka > And what is the purpose of fpregs_lock_and_load() ? Why do we need to > fpregs_restore_userregs() in shstk_setup() and other places? > > Oleg. >
On Wed, 2024-05-15 at 13:35 +0200, Oleg Nesterov wrote: > Let me repeat I know nothing about shadow stacks, only tried to > read Documentation/arch/x86/shstk.rst few minutes ago ;) > > On 05/13, Jiri Olsa wrote: > > > > 1) current uretprobe which are not working at the moment and we change > > the top value of shadow stack with shstk_push_frame > > 2) optimized uretprobe which needs to push new frame on shadow stack > > with shstk_update_last_frame > > > > I think we should do 1) and have current uretprobe working with shadow > > stack, which is broken at the moment > > Agreed, > > > I'm ok with not using optimized uretprobe when shadow stack is detected > > as enabled and we go with current uretprobe in that case > > But how can we detect it? Again, suppose userspace does the rdssp instruction returns the value of the shadow stack pointer. On non- shadow stack it is a nop. So you could check if the SSP is non-zero to find if shadow stack is enabled. This would catch most cases, but I guess there is the possibility of it getting enabled in a signal that hit between checking and the rest of operation. Is this uretprobe stuff signal safe in general?
On Wed, 2024-05-15 at 08:36 -0600, Jiri Olsa wrote: > > > > Let me ask a couple of really stupid questions. What if the shadow stack > > is "shorter" than the normal stack? I mean, The shadow stack could overflow if it is not big enough. However since the normal stack has return addresses and data, and shadow stack has the smaller amount data of only return addresses, we can mostly avoid this by picking a large size for the shadow stack. For underflow, you can't return from the point where you enable shadow stack. Almost all uses will enable it very early. Glibc loader does it before main is reached, for example. The shadow stack selftest is not a typical usage in this respect.
On 05/15, Jiri Olsa wrote: > > On Wed, May 15, 2024 at 01:19:20PM +0200, Oleg Nesterov wrote: > > Let me ask a couple of really stupid questions. What if the shadow stack > > is "shorter" than the normal stack? I mean, > > > > enable_shstk() > > { > > prctl(ARCH_SHSTK_SHSTK); I meant ARCH_SHSTK_ENABLE, of course > > } > > > > what happens when enable_shstk() returns? > > I think it will crash, there's explanation in the comment in > tools/testing/selftests/x86/test_shadow_stack.c test OK, thanks... But test_shadow_stack.c doesn't do ARCH_PRCTL(ARCH_SHSTK_DISABLE) if all the tests succeed ? Confused but nevermind. > > And what is the purpose of fpregs_lock_and_load() ? Why do we need to > > fpregs_restore_userregs() in shstk_setup() and other places? > > > > Oleg. > > >
On Wed, 2024-05-15 at 17:26 +0200, Oleg Nesterov wrote: > > I think it will crash, there's explanation in the comment in > > tools/testing/selftests/x86/test_shadow_stack.c test > > OK, thanks... > > But test_shadow_stack.c doesn't do ARCH_PRCTL(ARCH_SHSTK_DISABLE) if > all the tests succeed ? Confused but nevermind. The last test disables shadow stack as part of the test. So if it succeeds it doesn't need to disable the shadow stack to prevent underflow.
On 05/15, Edgecombe, Rick P wrote: > > On Wed, 2024-05-15 at 13:35 +0200, Oleg Nesterov wrote: > > > > > I'm ok with not using optimized uretprobe when shadow stack is detected > > > as enabled and we go with current uretprobe in that case > > > > But how can we detect it? Again, suppose userspace does > > the rdssp instruction returns the value of the shadow stack pointer. On non- > shadow stack it is a nop. So you could check if the SSP is non-zero to find if > shadow stack is enabled. But again, the ret-probed function can enable it before it returns? And we need to check if it is enabled on the function entry if we want to avoid sys_uretprobe() in this case. Although I don't understand why we want to avoid it. > This would catch most cases, but I guess there is the > possibility of it getting enabled in a signal that hit between checking and the > rest of operation. Or from signal handler. > Is this uretprobe stuff signal safe in general? In what sense? I forgot everything about this code but I can't recall any problem with signals. Except it doesn't support sigaltstack() + siglongjmp(). Oleg.
On Wed, May 15, 2024 at 05:42:03PM +0200, Oleg Nesterov wrote: > On 05/15, Edgecombe, Rick P wrote: > > > > On Wed, 2024-05-15 at 13:35 +0200, Oleg Nesterov wrote: > > > > > > > I'm ok with not using optimized uretprobe when shadow stack is detected > > > > as enabled and we go with current uretprobe in that case > > > > > > But how can we detect it? Again, suppose userspace does > > > > the rdssp instruction returns the value of the shadow stack pointer. On non- > > shadow stack it is a nop. So you could check if the SSP is non-zero to find if > > shadow stack is enabled. > > But again, the ret-probed function can enable it before it returns? And we > need to check if it is enabled on the function entry if we want to avoid > sys_uretprobe() in this case. yea, that's another complexity > Although I don't understand why we want to > avoid it. AFAIU the problem is that by using uretprobe syscall we make the kernel to push data on shadow stack [1], which adds another way to modify shadow stack as pointed out by Rick in here [2]: > Shadow stack allows for modification to the shadow stack only through a few > limited ways (call, ret, etc). The kernel has the ability to write through > shadow stack protections (for example when pushing and popping signal frames), > but the ways in which it does this are limited in order to try to prevent > providing extra capabilities to attackers wanting to craft their own shadow > stacks. anyway I think we can fix that in another way by using the optimized trampoline, but returning to the user space through iret when shadow stack is detected (as I did in the first version, before you adjusted it to the sysret path). we need to update the return address on stack only when returning through the trampoline, but we can jump to original return address directly from syscall through iret.. which is slower, but with shadow stack we don't care basically the only change is adding the shstk_is_enabled check to the following condition in SYSCALL_DEFINE0(uretprobe): if (regs->sp != sp || shstk_is_enabled()) return regs->ax; I'm testing patch below and it looks good so far, I'll add test code to run existing tests on top of shadow stack as well (when detected). I'll send new version with that jirka --- diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h index 896909f306e3..4cb77e004615 100644 --- a/arch/x86/include/asm/shstk.h +++ b/arch/x86/include/asm/shstk.h @@ -22,6 +22,7 @@ void shstk_free(struct task_struct *p); int setup_signal_shadow_stack(struct ksignal *ksig); int restore_signal_shadow_stack(void); int shstk_update_last_frame(unsigned long val); +bool shstk_is_enabled(void); #else static inline long shstk_prctl(struct task_struct *task, int option, unsigned long arg2) { return -EINVAL; } @@ -33,6 +34,7 @@ static inline void shstk_free(struct task_struct *p) {} static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; } static inline int restore_signal_shadow_stack(void) { return 0; } static inline int shstk_update_last_frame(unsigned long val) { return 0; } +static inline bool shstk_is_enabled(void) { return false; } #endif /* CONFIG_X86_USER_SHADOW_STACK */ #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c index 9797d4cdb78a..059685612362 100644 --- a/arch/x86/kernel/shstk.c +++ b/arch/x86/kernel/shstk.c @@ -588,3 +588,8 @@ int shstk_update_last_frame(unsigned long val) ssp = get_user_shstk_addr(); return write_user_shstk_64((u64 __user *)ssp, (u64)val); } + +bool shstk_is_enabled(void) +{ + return features_enabled(ARCH_SHSTK_SHSTK); +} diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c index 6402fb3089d2..5a952c5ea66b 100644 --- a/arch/x86/kernel/uprobes.c +++ b/arch/x86/kernel/uprobes.c @@ -12,6 +12,7 @@ #include <linux/ptrace.h> #include <linux/uprobes.h> #include <linux/uaccess.h> +#include <linux/syscalls.h> #include <linux/kdebug.h> #include <asm/processor.h> @@ -308,6 +309,122 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool } #ifdef CONFIG_X86_64 + +asm ( + ".pushsection .rodata\n" + ".global uretprobe_trampoline_entry\n" + "uretprobe_trampoline_entry:\n" + "pushq %rax\n" + "pushq %rcx\n" + "pushq %r11\n" + "movq $" __stringify(__NR_uretprobe) ", %rax\n" + "syscall\n" + ".global uretprobe_syscall_check\n" + "uretprobe_syscall_check:\n" + "popq %r11\n" + "popq %rcx\n" + + /* The uretprobe syscall replaces stored %rax value with final + * return address, so we don't restore %rax in here and just + * call ret. + */ + "retq\n" + ".global uretprobe_trampoline_end\n" + "uretprobe_trampoline_end:\n" + ".popsection\n" +); + +extern u8 uretprobe_trampoline_entry[]; +extern u8 uretprobe_trampoline_end[]; +extern u8 uretprobe_syscall_check[]; + +void *arch_uprobe_trampoline(unsigned long *psize) +{ + static uprobe_opcode_t insn = UPROBE_SWBP_INSN; + struct pt_regs *regs = task_pt_regs(current); + + /* + * At the moment the uretprobe syscall trampoline is supported + * only for native 64-bit process, the compat process still uses + * standard breakpoint. + */ + if (user_64bit_mode(regs)) { + *psize = uretprobe_trampoline_end - uretprobe_trampoline_entry; + return uretprobe_trampoline_entry; + } + + *psize = UPROBE_SWBP_INSN_SIZE; + return &insn; +} + +static unsigned long trampoline_check_ip(void) +{ + unsigned long tramp = uprobe_get_trampoline_vaddr(); + + return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry); +} + +SYSCALL_DEFINE0(uretprobe) +{ + struct pt_regs *regs = task_pt_regs(current); + unsigned long err, ip, sp, r11_cx_ax[3]; + + if (regs->ip != trampoline_check_ip()) + goto sigill; + + err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax)); + if (err) + goto sigill; + + /* expose the "right" values of r11/cx/ax/sp to uprobe_consumer/s */ + regs->r11 = r11_cx_ax[0]; + regs->cx = r11_cx_ax[1]; + regs->ax = r11_cx_ax[2]; + regs->sp += sizeof(r11_cx_ax); + regs->orig_ax = -1; + + ip = regs->ip; + sp = regs->sp; + + uprobe_handle_trampoline(regs); + + /* + * Some of the uprobe consumers has changed sp, we can do nothing, + * just return via iret. + * .. or shadow stack is enabled, in which case we need to skip + * return through the user space stack address. + */ + if (regs->sp != sp || shstk_is_enabled()) + return regs->ax; + regs->sp -= sizeof(r11_cx_ax); + + /* for the case uprobe_consumer has changed r11/cx */ + r11_cx_ax[0] = regs->r11; + r11_cx_ax[1] = regs->cx; + + /* + * ax register is passed through as return value, so we can use + * its space on stack for ip value and jump to it through the + * trampoline's ret instruction + */ + r11_cx_ax[2] = regs->ip; + regs->ip = ip; + + err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax)); + if (err) + goto sigill; + + /* ensure sysret, see do_syscall_64() */ + regs->r11 = regs->flags; + regs->cx = regs->ip; + + return regs->ax; + +sigill: + force_sig(SIGILL); + return -1; +} + /* * If arch_uprobe->insn doesn't use rip-relative addressing, return * immediately. Otherwise, rewrite the instruction so that it accesses diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h index f46e0ca0169c..b503fafb7fb3 100644 --- a/include/linux/uprobes.h +++ b/include/linux/uprobes.h @@ -138,6 +138,9 @@ extern bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check c extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs); extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr, void *src, unsigned long len); +extern void uprobe_handle_trampoline(struct pt_regs *regs); +extern void *arch_uprobe_trampoline(unsigned long *psize); +extern unsigned long uprobe_get_trampoline_vaddr(void); #else /* !CONFIG_UPROBES */ struct uprobes_state { }; diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 2c83ba776fc7..2816e65729ac 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1474,11 +1474,20 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) return ret; } +void * __weak arch_uprobe_trampoline(unsigned long *psize) +{ + static uprobe_opcode_t insn = UPROBE_SWBP_INSN; + + *psize = UPROBE_SWBP_INSN_SIZE; + return &insn; +} + static struct xol_area *__create_xol_area(unsigned long vaddr) { struct mm_struct *mm = current->mm; - uprobe_opcode_t insn = UPROBE_SWBP_INSN; + unsigned long insns_size; struct xol_area *area; + void *insns; area = kmalloc(sizeof(*area), GFP_KERNEL); if (unlikely(!area)) @@ -1502,7 +1511,8 @@ static struct xol_area *__create_xol_area(unsigned long vaddr) /* Reserve the 1st slot for get_trampoline_vaddr() */ set_bit(0, area->bitmap); atomic_set(&area->slot_count, 1); - arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE); + insns = arch_uprobe_trampoline(&insns_size); + arch_uprobe_copy_ixol(area->pages[0], 0, insns, insns_size); if (!xol_add_vma(mm, area)) return area; @@ -1827,7 +1837,7 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags) * * Returns -1 in case the xol_area is not allocated. */ -static unsigned long get_trampoline_vaddr(void) +unsigned long uprobe_get_trampoline_vaddr(void) { struct xol_area *area; unsigned long trampoline_vaddr = -1; @@ -1878,7 +1888,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs) if (!ri) return; - trampoline_vaddr = get_trampoline_vaddr(); + trampoline_vaddr = uprobe_get_trampoline_vaddr(); orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs); if (orig_ret_vaddr == -1) goto fail; @@ -2123,7 +2133,7 @@ static struct return_instance *find_next_ret_chain(struct return_instance *ri) return ri; } -static void handle_trampoline(struct pt_regs *regs) +void uprobe_handle_trampoline(struct pt_regs *regs) { struct uprobe_task *utask; struct return_instance *ri, *next; @@ -2187,8 +2197,8 @@ static void handle_swbp(struct pt_regs *regs) int is_swbp; bp_vaddr = uprobe_get_swbp_addr(regs); - if (bp_vaddr == get_trampoline_vaddr()) - return handle_trampoline(regs); + if (bp_vaddr == uprobe_get_trampoline_vaddr()) + return uprobe_handle_trampoline(regs); uprobe = find_active_uprobe(bp_vaddr, &is_swbp); if (!uprobe) {
On Mon, 2024-05-20 at 00:18 +0200, Jiri Olsa wrote: > anyway I think we can fix that in another way by using the optimized > trampoline, > but returning to the user space through iret when shadow stack is detected > (as I did in the first version, before you adjusted it to the sysret path). > > we need to update the return address on stack only when returning through the > trampoline, but we can jump to original return address directly from syscall > through iret.. which is slower, but with shadow stack we don't care > > basically the only change is adding the shstk_is_enabled check to the > following condition in SYSCALL_DEFINE0(uretprobe): > > if (regs->sp != sp || shstk_is_enabled()) > return regs->ax; On the surface it sounds reasonable. Thanks. And then I guess if tradeoffs are seen differently in the future, and we want to enable the fast path for shadow stack we can go with your other solution. So this just simply fixes things functionally without much code.
On Tue, May 21, 2024 at 01:31:53AM +0000, Edgecombe, Rick P wrote: > On Mon, 2024-05-20 at 00:18 +0200, Jiri Olsa wrote: > > anyway I think we can fix that in another way by using the optimized > > trampoline, > > but returning to the user space through iret when shadow stack is detected > > (as I did in the first version, before you adjusted it to the sysret path). > > > > we need to update the return address on stack only when returning through the > > trampoline, but we can jump to original return address directly from syscall > > through iret.. which is slower, but with shadow stack we don't care > > > > basically the only change is adding the shstk_is_enabled check to the > > following condition in SYSCALL_DEFINE0(uretprobe): > > > > if (regs->sp != sp || shstk_is_enabled()) > > return regs->ax; > > On the surface it sounds reasonable. Thanks. > > And then I guess if tradeoffs are seen differently in the future, and we want to > enable the fast path for shadow stack we can go with your other solution. So > this just simply fixes things functionally without much code. yes, if we want to enable the fast path for shadow stack in future we'll need to remove that shstk_is_enabled and push extra frame on shadow stack jirka
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h index 42fee8959df7..2e1ddcf98242 100644 --- a/arch/x86/include/asm/shstk.h +++ b/arch/x86/include/asm/shstk.h @@ -21,6 +21,8 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clon void shstk_free(struct task_struct *p); int setup_signal_shadow_stack(struct ksignal *ksig); int restore_signal_shadow_stack(void); +int shstk_update_last_frame(unsigned long val); +int shstk_push_frame(unsigned long val); #else static inline long shstk_prctl(struct task_struct *task, int option, unsigned long arg2) { return -EINVAL; } @@ -31,6 +33,8 @@ static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p, static inline void shstk_free(struct task_struct *p) {} static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; } static inline int restore_signal_shadow_stack(void) { return 0; } +static inline int shstk_update_last_frame(unsigned long val) { return 0; } +static inline int shstk_push_frame(unsigned long val) { return 0; } #endif /* CONFIG_X86_USER_SHADOW_STACK */ #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c index 59e15dd8d0f8..66434dfde52e 100644 --- a/arch/x86/kernel/shstk.c +++ b/arch/x86/kernel/shstk.c @@ -577,3 +577,32 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2) return wrss_control(true); return -EINVAL; } + +int shstk_update_last_frame(unsigned long val) +{ + unsigned long ssp; + + if (!features_enabled(ARCH_SHSTK_SHSTK)) + return 0; + + ssp = get_user_shstk_addr(); + return write_user_shstk_64((u64 __user *)ssp, (u64)val); +} + +int shstk_push_frame(unsigned long val) +{ + unsigned long ssp; + + if (!features_enabled(ARCH_SHSTK_SHSTK)) + return 0; + + ssp = get_user_shstk_addr(); + ssp -= SS_FRAME_SIZE; + if (write_user_shstk_64((u64 __user *)ssp, (u64)val)) + return -EFAULT; + + fpregs_lock_and_load(); + wrmsrl(MSR_IA32_PL3_SSP, ssp); + fpregs_unlock(); + return 0; +} diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c index 81e6ee95784d..ae6c3458a675 100644 --- a/arch/x86/kernel/uprobes.c +++ b/arch/x86/kernel/uprobes.c @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe) * trampoline's ret instruction */ r11_cx_ax[2] = regs->ip; + + /* make the shadow stack follow that */ + if (shstk_push_frame(regs->ip)) + goto sigill; + regs->ip = ip; err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax)); @@ -1191,8 +1196,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs return orig_ret_vaddr; nleft = copy_to_user((void __user *)regs->sp, &trampoline_vaddr, rasize); - if (likely(!nleft)) + if (likely(!nleft)) { + if (shstk_update_last_frame(trampoline_vaddr)) { + force_sig(SIGSEGV); + return -1; + } return orig_ret_vaddr; + } if (nleft != rasize) { pr_err("return address clobbered: pid=%d, %%sp=%#lx, %%ip=%#lx\n",
Adding return uprobe support to work with enabled shadow stack. Currently the application with enabled shadow stack will crash if it sets up return uprobe. The reason is the uretprobe kernel code changes the user space task's stack, but does not update shadow stack accordingly. Adding new functions to update values on shadow stack and using them in uprobe code to keep shadow stack in sync with uretprobe changes to user stack. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- arch/x86/include/asm/shstk.h | 4 ++++ arch/x86/kernel/shstk.c | 29 +++++++++++++++++++++++++++++ arch/x86/kernel/uprobes.c | 12 +++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-)