This is called from deep entry ASM in a situation where instrumentation
will cause more harm than providing useful information.
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/traps.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -621,7 +621,7 @@ struct bad_iret_stack {
struct pt_regs regs;
};
-asmlinkage __visible notrace
+asmlinkage __visible noinstr
struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
{
/*
@@ -632,19 +632,21 @@ struct bad_iret_stack *fixup_bad_iret(st
* just below the IRET frame) and we want to pretend that the
* exception came from the IRET target.
*/
- struct bad_iret_stack *new_stack =
- (struct bad_iret_stack *)this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
+ struct bad_iret_stack tmp, *new_stack =
+ (struct bad_iret_stack *)__this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
- /* Copy the IRET target to the new stack. */
- memmove(&new_stack->regs.ip, (void *)s->regs.sp, 5*8);
+ /* Copy the IRET target to the temporary storage. */
+ memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
/* Copy the remainder of the stack from the current stack. */
- memmove(new_stack, s, offsetof(struct bad_iret_stack, regs.ip));
+ memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
+
+ /* Update the entry stack */
+ memcpy(new_stack, &tmp, sizeof(tmp));
BUG_ON(!user_mode(&new_stack->regs));
return new_stack;
}
-NOKPROBE_SYMBOL(fixup_bad_iret);
#endif
static bool is_sysenter_singlestep(struct pt_regs *regs)
On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>
> This is called from deep entry ASM in a situation where instrumentation
> will cause more harm than providing useful information.
>
Acked-by: Andy Lutomirski <[email protected]>
Maybe add to changelog:
Switch from memmove() to memcpy() because memmove() can't be called
from noinstr code.
On Tue, 05 May 2020 15:43:55 +0200
Thomas Gleixner <[email protected]> wrote:
> This is called from deep entry ASM in a situation where instrumentation
> will cause more harm than providing useful information.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
Looks good to me.
Reviewed-by: Masami Hiramatsu <[email protected]>
Thank you,
> ---
> arch/x86/kernel/traps.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -621,7 +621,7 @@ struct bad_iret_stack {
> struct pt_regs regs;
> };
>
> -asmlinkage __visible notrace
> +asmlinkage __visible noinstr
> struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
> {
> /*
> @@ -632,19 +632,21 @@ struct bad_iret_stack *fixup_bad_iret(st
> * just below the IRET frame) and we want to pretend that the
> * exception came from the IRET target.
> */
> - struct bad_iret_stack *new_stack =
> - (struct bad_iret_stack *)this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
> + struct bad_iret_stack tmp, *new_stack =
> + (struct bad_iret_stack *)__this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
>
> - /* Copy the IRET target to the new stack. */
> - memmove(&new_stack->regs.ip, (void *)s->regs.sp, 5*8);
> + /* Copy the IRET target to the temporary storage. */
> + memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
>
> /* Copy the remainder of the stack from the current stack. */
> - memmove(new_stack, s, offsetof(struct bad_iret_stack, regs.ip));
> + memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
> +
> + /* Update the entry stack */
> + memcpy(new_stack, &tmp, sizeof(tmp));
>
> BUG_ON(!user_mode(&new_stack->regs));
> return new_stack;
> }
> -NOKPROBE_SYMBOL(fixup_bad_iret);
> #endif
>
> static bool is_sysenter_singlestep(struct pt_regs *regs)
>
--
Masami Hiramatsu <[email protected]>
On Fri, 8 May 2020 17:39:00 -0700
Andy Lutomirski <[email protected]> wrote:
> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
> >
> > This is called from deep entry ASM in a situation where instrumentation
> > will cause more harm than providing useful information.
> >
>
> Acked-by: Andy Lutomirski <[email protected]>
>
> Maybe add to changelog:
>
> Switch from memmove() to memcpy() because memmove() can't be called
> from noinstr code.
Yes please, because I was about to say that there was changes that
didn't seem to fit the change log.
I would also add a comment in the code saying that we need the temp
variable to use memcpy as memmove can't be used in noinstr code.
-- Steve
----- On May 12, 2020, at 9:51 PM, rostedt [email protected] wrote:
> On Fri, 8 May 2020 17:39:00 -0700
> Andy Lutomirski <[email protected]> wrote:
>
>> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>> >
>> > This is called from deep entry ASM in a situation where instrumentation
>> > will cause more harm than providing useful information.
>> >
>>
>> Acked-by: Andy Lutomirski <[email protected]>
>>
>> Maybe add to changelog:
>>
>> Switch from memmove() to memcpy() because memmove() can't be called
>> from noinstr code.
>
> Yes please, because I was about to say that there was changes that
> didn't seem to fit the change log.
>
> I would also add a comment in the code saying that we need the temp
> variable to use memcpy as memmove can't be used in noinstr code.
Looking at an updated version of the tree, I see the acked-by from Andy,
but not comment about switching from memmove to memcpy.
Also, I notice a significant undocumented change in this patch: it changes
a this_cpu_read() (which presumes preemption is enabled) to a __this_cpu_read().
So the 100$ question: is preemption enabled or not in fixup_bad_iret() ? And of
course that change should be documented in the commit message.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
> On May 13, 2020, at 5:41 PM, Mathieu Desnoyers <[email protected]> wrote:
>
> ----- On May 12, 2020, at 9:51 PM, rostedt [email protected] wrote:
>
>>> On Fri, 8 May 2020 17:39:00 -0700
>>> Andy Lutomirski <[email protected]> wrote:
>>>
>>> On Tue, May 5, 2020 at 7:15 AM Thomas Gleixner <[email protected]> wrote:
>>>>
>>>> This is called from deep entry ASM in a situation where instrumentation
>>>> will cause more harm than providing useful information.
>>>>
>>>
>>> Acked-by: Andy Lutomirski <[email protected]>
>>>
>>> Maybe add to changelog:
>>>
>>> Switch from memmove() to memcpy() because memmove() can't be called
>>> from noinstr code.
>>
>> Yes please, because I was about to say that there was changes that
>> didn't seem to fit the change log.
>>
>> I would also add a comment in the code saying that we need the temp
>> variable to use memcpy as memmove can't be used in noinstr code.
>
> Looking at an updated version of the tree, I see the acked-by from Andy,
> but not comment about switching from memmove to memcpy.
>
> Also, I notice a significant undocumented change in this patch: it changes
> a this_cpu_read() (which presumes preemption is enabled) to a __this_cpu_read().
>
> So the 100$ question: is preemption enabled or not in fixup_bad_iret() ? And of
> course that change should be documented in the commit message.
>
IRQs are off, and, if they were on, the lack of a warning from the percpu access would be the least of our concerns here.
> Thanks,
>
> Mathieu
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
The following commit has been merged into the x86/entry branch of tip:
Commit-ID: 68a05c6247d2aa67f5ada1009ffd19758e5914ea
Gitweb: https://git.kernel.org/tip/68a05c6247d2aa67f5ada1009ffd19758e5914ea
Author: Thomas Gleixner <[email protected]>
AuthorDate: Wed, 25 Mar 2020 19:53:38 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Tue, 19 May 2020 16:03:52 +02:00
x86/traps: Mark fixup_bad_iret() noinstr
This is called from deep entry ASM in a situation where instrumentation
will cause more harm than providing useful information.
Switch from memmove() to memcpy() because memmove() can't be called
from noinstr code.
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Alexandre Chartre <[email protected]>
Reviewed-by: Masami Hiramatsu <[email protected]>
Acked-by: Peter Zijlstra <[email protected]>
Acked-by: Andy Lutomirski <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/traps.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 48468f6..b2b3656 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -578,7 +578,7 @@ struct bad_iret_stack {
struct pt_regs regs;
};
-asmlinkage __visible notrace
+asmlinkage __visible noinstr
struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
{
/*
@@ -589,19 +589,21 @@ struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
* just below the IRET frame) and we want to pretend that the
* exception came from the IRET target.
*/
- struct bad_iret_stack *new_stack =
- (struct bad_iret_stack *)this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
+ struct bad_iret_stack tmp, *new_stack =
+ (struct bad_iret_stack *)__this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
- /* Copy the IRET target to the new stack. */
- memmove(&new_stack->regs.ip, (void *)s->regs.sp, 5*8);
+ /* Copy the IRET target to the temporary storage. */
+ memcpy(&tmp.regs.ip, (void *)s->regs.sp, 5*8);
/* Copy the remainder of the stack from the current stack. */
- memmove(new_stack, s, offsetof(struct bad_iret_stack, regs.ip));
+ memcpy(&tmp, s, offsetof(struct bad_iret_stack, regs.ip));
+
+ /* Update the entry stack */
+ memcpy(new_stack, &tmp, sizeof(tmp));
BUG_ON(!user_mode(&new_stack->regs));
return new_stack;
}
-NOKPROBE_SYMBOL(fixup_bad_iret);
#endif
static bool is_sysenter_singlestep(struct pt_regs *regs)