[PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Kees Cook]

From: Marios Pomonis <pomonis@google.com>

Fix a bug in the ORC unwinder when kretprobes has replaced a return
address with the address of `kretprobes_trampoline'. ORC mistakenly
assumes that the address in the stack is a return address and decrements
it by 1 in order to find the proper depth of the next frame.

This issue was discovered while testing the FG-KASLR series[0][1] and
running the live patching test[2] that was originally failing[3].

[0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
[1] https://github.com/KSPP/linux/issues/132
[2] https://github.com/lpechacek/qa_test_klp
[3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/

Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
Signed-off-by: Marios Pomonis <pomonis@google.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/kernel/unwind_orc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index a1202536fc57..8c5038b3b707 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -7,6 +7,7 @@
 #include <asm/unwind.h>
 #include <asm/orc_types.h>
 #include <asm/orc_lookup.h>
+#include <asm/kprobes.h>
 
 #define orc_warn(fmt, ...) \
 	printk_deferred_once(KERN_WARNING "WARNING: " fmt, ##__VA_ARGS__)
@@ -414,6 +415,15 @@ static bool get_reg(struct unwind_state *state, unsigned int reg_off,
 	return false;
 }
 
+static bool is_kretprobe_trampoline(unsigned long ip)
+{
+#ifdef	CONFIG_KRETPROBES
+	if (ip == (unsigned long)&kretprobe_trampoline)
+		return true;
+#endif
+	return false;
+}
+
 bool unwind_next_frame(struct unwind_state *state)
 {
 	unsigned long ip_p, sp, tmp, orig_ip = state->ip, prev_sp = state->sp;
@@ -540,7 +550,7 @@ bool unwind_next_frame(struct unwind_state *state)
 		state->sp = sp;
 		state->regs = NULL;
 		state->prev_regs = NULL;
-		state->signal = false;
+		state->signal = is_kretprobe_trampoline(state->ip);
 		break;
 
 	case UNWIND_HINT_TYPE_REGS:
-- 
2.30.2

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Josh Poimboeuf]

On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> From: Marios Pomonis <pomonis@google.com>
> 
> Fix a bug in the ORC unwinder when kretprobes has replaced a return
> address with the address of `kretprobes_trampoline'. ORC mistakenly
> assumes that the address in the stack is a return address and decrements
> it by 1 in order to find the proper depth of the next frame.
> 
> This issue was discovered while testing the FG-KASLR series[0][1] and
> running the live patching test[2] that was originally failing[3].
> 
> [0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
> [1] https://github.com/KSPP/linux/issues/132
> [2] https://github.com/lpechacek/qa_test_klp
> [3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/
> 
> Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
> Signed-off-by: Marios Pomonis <pomonis@google.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>

I suspect this is fixed by:

  https://lkml.kernel.org/r/162756755600.301564.4957591913842010341.stgit@devnote2


> ---
>  arch/x86/kernel/unwind_orc.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index a1202536fc57..8c5038b3b707 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -7,6 +7,7 @@
>  #include <asm/unwind.h>
>  #include <asm/orc_types.h>
>  #include <asm/orc_lookup.h>
> +#include <asm/kprobes.h>
>  
>  #define orc_warn(fmt, ...) \
>  	printk_deferred_once(KERN_WARNING "WARNING: " fmt, ##__VA_ARGS__)
> @@ -414,6 +415,15 @@ static bool get_reg(struct unwind_state *state, unsigned int reg_off,
>  	return false;
>  }
>  
> +static bool is_kretprobe_trampoline(unsigned long ip)
> +{
> +#ifdef	CONFIG_KRETPROBES
> +	if (ip == (unsigned long)&kretprobe_trampoline)
> +		return true;
> +#endif
> +	return false;
> +}
> +
>  bool unwind_next_frame(struct unwind_state *state)
>  {
>  	unsigned long ip_p, sp, tmp, orig_ip = state->ip, prev_sp = state->sp;
> @@ -540,7 +550,7 @@ bool unwind_next_frame(struct unwind_state *state)
>  		state->sp = sp;
>  		state->regs = NULL;
>  		state->prev_regs = NULL;
> -		state->signal = false;
> +		state->signal = is_kretprobe_trampoline(state->ip);
>  		break;
>  
>  	case UNWIND_HINT_TYPE_REGS:
> -- 
> 2.30.2
> 

-- 
Josh

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Masami Hiramatsu]

On Sat, 4 Sep 2021 10:55:11 -0700
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> > From: Marios Pomonis <pomonis@google.com>
> > 
> > Fix a bug in the ORC unwinder when kretprobes has replaced a return
> > address with the address of `kretprobes_trampoline'. ORC mistakenly
> > assumes that the address in the stack is a return address and decrements
> > it by 1 in order to find the proper depth of the next frame.
> > 
> > This issue was discovered while testing the FG-KASLR series[0][1] and
> > running the live patching test[2] that was originally failing[3].
> > 
> > [0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
> > [1] https://github.com/KSPP/linux/issues/132
> > [2] https://github.com/lpechacek/qa_test_klp
> > [3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/
> > 
> > Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
> > Signed-off-by: Marios Pomonis <pomonis@google.com>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> I suspect this is fixed by:
> 
>   https://lkml.kernel.org/r/162756755600.301564.4957591913842010341.stgit@devnote2

I think this can be a bit different issue. As far as I ran my test code
(same one in the above cover mail) with this fix, the stacktrace wasn't
fixed.

ffffffff812b7c80  r  vfs_read+0x0    [FTRACE]
ffffffff813b4cc0  r  full_proxy_read+0x0    [FTRACE]
# tracer: nop
#
# entries-in-buffer/entries-written: 3/3   #P:8
#
#                                _-----=> irqs-off
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| /     delay
#           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
#              | |         |   ||||      |         |
             cat-138     [002] ...1     9.488727: r_full_proxy_read_0: (vfs_read+0x99/0x190 <- full_proxy_read)
             cat-138     [002] ...1     9.488732: <stack trace>
 => kretprobe_trace_func+0x209/0x300
 => kretprobe_dispatcher+0x9d/0xb0
 => __kretprobe_trampoline_handler+0xc5/0x160
 => trampoline_handler+0x44/0x60
 => kretprobe_trampoline+0x25/0x50
             cat-138     [002] ...1     9.488733: r_vfs_read_0: (ksys_read+0x68/0xe0 <- vfs_read)

Kees, can you also try to test with my series?
It should be able to be checked out with;

git clone git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git -b kprobes/kretprobe-stackfix-v10

Thank you,
> 
> 
> > ---
> >  arch/x86/kernel/unwind_orc.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> > index a1202536fc57..8c5038b3b707 100644
> > --- a/arch/x86/kernel/unwind_orc.c
> > +++ b/arch/x86/kernel/unwind_orc.c
> > @@ -7,6 +7,7 @@
> >  #include <asm/unwind.h>
> >  #include <asm/orc_types.h>
> >  #include <asm/orc_lookup.h>
> > +#include <asm/kprobes.h>
> >  
> >  #define orc_warn(fmt, ...) \
> >  	printk_deferred_once(KERN_WARNING "WARNING: " fmt, ##__VA_ARGS__)
> > @@ -414,6 +415,15 @@ static bool get_reg(struct unwind_state *state, unsigned int reg_off,
> >  	return false;
> >  }
> >  
> > +static bool is_kretprobe_trampoline(unsigned long ip)
> > +{
> > +#ifdef	CONFIG_KRETPROBES
> > +	if (ip == (unsigned long)&kretprobe_trampoline)
> > +		return true;
> > +#endif
> > +	return false;
> > +}
> > +
> >  bool unwind_next_frame(struct unwind_state *state)
> >  {
> >  	unsigned long ip_p, sp, tmp, orig_ip = state->ip, prev_sp = state->sp;
> > @@ -540,7 +550,7 @@ bool unwind_next_frame(struct unwind_state *state)
> >  		state->sp = sp;
> >  		state->regs = NULL;
> >  		state->prev_regs = NULL;
> > -		state->signal = false;
> > +		state->signal = is_kretprobe_trampoline(state->ip);
> >  		break;
> >  
> >  	case UNWIND_HINT_TYPE_REGS:
> > -- 
> > 2.30.2
> > 
> 
> -- 
> Josh
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Marios Pomonis]

On Sun, Sep 5, 2021 at 12:57 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Sat, 4 Sep 2021 10:55:11 -0700
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> > On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> > > From: Marios Pomonis <pomonis@google.com>
> > >
> > > Fix a bug in the ORC unwinder when kretprobes has replaced a return
> > > address with the address of `kretprobes_trampoline'. ORC mistakenly
> > > assumes that the address in the stack is a return address and decrements
> > > it by 1 in order to find the proper depth of the next frame.
> > >
> > > This issue was discovered while testing the FG-KASLR series[0][1] and
> > > running the live patching test[2] that was originally failing[3].
> > >
> > > [0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
> > > [1] https://github.com/KSPP/linux/issues/132
> > > [2] https://github.com/lpechacek/qa_test_klp
> > > [3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/
> > >
> > > Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
> > > Signed-off-by: Marios Pomonis <pomonis@google.com>
> > > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> > > Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
> > > Cc: Sami Tolvanen <samitolvanen@google.com>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > I suspect this is fixed by:
> >
> >   https://lkml.kernel.org/r/162756755600.301564.4957591913842010341.stgit@devnote2
>
> I think this can be a bit different issue. As far as I ran my test code
> (same one in the above cover mail) with this fix, the stacktrace wasn't
> fixed.
>
> ffffffff812b7c80  r  vfs_read+0x0    [FTRACE]
> ffffffff813b4cc0  r  full_proxy_read+0x0    [FTRACE]
> # tracer: nop
> #
> # entries-in-buffer/entries-written: 3/3   #P:8
> #
> #                                _-----=> irqs-off
> #                               / _----=> need-resched
> #                              | / _---=> hardirq/softirq
> #                              || / _--=> preempt-depth
> #                              ||| /     delay
> #           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
> #              | |         |   ||||      |         |
>              cat-138     [002] ...1     9.488727: r_full_proxy_read_0: (vfs_read+0x99/0x190 <- full_proxy_read)
>              cat-138     [002] ...1     9.488732: <stack trace>
>  => kretprobe_trace_func+0x209/0x300
>  => kretprobe_dispatcher+0x9d/0xb0
>  => __kretprobe_trampoline_handler+0xc5/0x160
>  => trampoline_handler+0x44/0x60
>  => kretprobe_trampoline+0x25/0x50
>              cat-138     [002] ...1     9.488733: r_vfs_read_0: (ksys_read+0x68/0xe0 <- vfs_read)
>
> Kees, can you also try to test with my series?
> It should be able to be checked out with;
>
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git -b kprobes/kretprobe-stackfix-v10
>
> Thank you,

I tested this series in conjunction with FG-KASLR and klp_tc_12 fails.
Therefore the patch of the cover mail fixes a different issue than the
one of this series.

Thanks,
Marios
> >
> >
> > > ---
> > >  arch/x86/kernel/unwind_orc.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> > > index a1202536fc57..8c5038b3b707 100644
> > > --- a/arch/x86/kernel/unwind_orc.c
> > > +++ b/arch/x86/kernel/unwind_orc.c
> > > @@ -7,6 +7,7 @@
> > >  #include <asm/unwind.h>
> > >  #include <asm/orc_types.h>
> > >  #include <asm/orc_lookup.h>
> > > +#include <asm/kprobes.h>
> > >
> > >  #define orc_warn(fmt, ...) \
> > >     printk_deferred_once(KERN_WARNING "WARNING: " fmt, ##__VA_ARGS__)
> > > @@ -414,6 +415,15 @@ static bool get_reg(struct unwind_state *state, unsigned int reg_off,
> > >     return false;
> > >  }
> > >
> > > +static bool is_kretprobe_trampoline(unsigned long ip)
> > > +{
> > > +#ifdef     CONFIG_KRETPROBES
> > > +   if (ip == (unsigned long)&kretprobe_trampoline)
> > > +           return true;
> > > +#endif
> > > +   return false;
> > > +}
> > > +
> > >  bool unwind_next_frame(struct unwind_state *state)
> > >  {
> > >     unsigned long ip_p, sp, tmp, orig_ip = state->ip, prev_sp = state->sp;
> > > @@ -540,7 +550,7 @@ bool unwind_next_frame(struct unwind_state *state)
> > >             state->sp = sp;
> > >             state->regs = NULL;
> > >             state->prev_regs = NULL;
> > > -           state->signal = false;
> > > +           state->signal = is_kretprobe_trampoline(state->ip);
> > >             break;
> > >
> > >     case UNWIND_HINT_TYPE_REGS:
> > > --
> > > 2.30.2
> > >
> >
> > --
> > Josh
> >
>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Kees Cook]

On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> From: Marios Pomonis <pomonis@google.com>
> 
> Fix a bug in the ORC unwinder when kretprobes has replaced a return
> address with the address of `kretprobes_trampoline'. ORC mistakenly
> assumes that the address in the stack is a return address and decrements
> it by 1 in order to find the proper depth of the next frame.
> 
> This issue was discovered while testing the FG-KASLR series[0][1] and
> running the live patching test[2] that was originally failing[3].
> 
> [0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
> [1] https://github.com/KSPP/linux/issues/132
> [2] https://github.com/lpechacek/qa_test_klp
> [3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/
> 
> Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
> Signed-off-by: Marios Pomonis <pomonis@google.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Ping again; Josh can you take this please?

-Kees

> ---
>  arch/x86/kernel/unwind_orc.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
> index a1202536fc57..8c5038b3b707 100644
> --- a/arch/x86/kernel/unwind_orc.c
> +++ b/arch/x86/kernel/unwind_orc.c
> @@ -7,6 +7,7 @@
>  #include <asm/unwind.h>
>  #include <asm/orc_types.h>
>  #include <asm/orc_lookup.h>
> +#include <asm/kprobes.h>
>  
>  #define orc_warn(fmt, ...) \
>  	printk_deferred_once(KERN_WARNING "WARNING: " fmt, ##__VA_ARGS__)
> @@ -414,6 +415,15 @@ static bool get_reg(struct unwind_state *state, unsigned int reg_off,
>  	return false;
>  }
>  
> +static bool is_kretprobe_trampoline(unsigned long ip)
> +{
> +#ifdef	CONFIG_KRETPROBES
> +	if (ip == (unsigned long)&kretprobe_trampoline)
> +		return true;
> +#endif
> +	return false;
> +}
> +
>  bool unwind_next_frame(struct unwind_state *state)
>  {
>  	unsigned long ip_p, sp, tmp, orig_ip = state->ip, prev_sp = state->sp;
> @@ -540,7 +550,7 @@ bool unwind_next_frame(struct unwind_state *state)
>  		state->sp = sp;
>  		state->regs = NULL;
>  		state->prev_regs = NULL;
> -		state->signal = false;
> +		state->signal = is_kretprobe_trampoline(state->ip);
>  		break;
>  
>  	case UNWIND_HINT_TYPE_REGS:
> -- 
> 2.30.2
> 

-- 
Kees Cook

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Josh Poimboeuf]

On Mon, Oct 11, 2021 at 02:03:26PM -0700, Kees Cook wrote:
> On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> > From: Marios Pomonis <pomonis@google.com>
> > 
> > Fix a bug in the ORC unwinder when kretprobes has replaced a return
> > address with the address of `kretprobes_trampoline'. ORC mistakenly
> > assumes that the address in the stack is a return address and decrements
> > it by 1 in order to find the proper depth of the next frame.
> > 
> > This issue was discovered while testing the FG-KASLR series[0][1] and
> > running the live patching test[2] that was originally failing[3].
> > 
> > [0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
> > [1] https://github.com/KSPP/linux/issues/132
> > [2] https://github.com/lpechacek/qa_test_klp
> > [3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/
> > 
> > Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
> > Signed-off-by: Marios Pomonis <pomonis@google.com>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> > Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
> > Cc: Sami Tolvanen <samitolvanen@google.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Ping again; Josh can you take this please?

I'm confused how this still fixes anything after Masami's patch set,
which is now in linux-next.

After those patches, for a CALL-type ORC entry, the unwinder sets
state->ip to the address returned by unwind_recover_ret_addr().  In the
case of a kretprobe, that means that state->ip will no longer point to
kretprobes_trampoline() -- making the above patch description incorrect.

Instead, state->ip will then contain the original call return address
which was replaced by kretpobes.  So it looks to the unwinder like a
normal call return address, and 'state->signal' should remain false.

Am I missing something?

-- 
Josh

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Kees Cook]

On Wed, Oct 13, 2021 at 06:41:01PM -0700, Josh Poimboeuf wrote:
> On Mon, Oct 11, 2021 at 02:03:26PM -0700, Kees Cook wrote:
> > On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> > > From: Marios Pomonis <pomonis@google.com>
> > > 
> > > Fix a bug in the ORC unwinder when kretprobes has replaced a return
> > > address with the address of `kretprobes_trampoline'. ORC mistakenly
> > > assumes that the address in the stack is a return address and decrements
> > > it by 1 in order to find the proper depth of the next frame.
> > > 
> > > This issue was discovered while testing the FG-KASLR series[0][1] and
> > > running the live patching test[2] that was originally failing[3].
> > > 
> > > [0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
> > > [1] https://github.com/KSPP/linux/issues/132
> > > [2] https://github.com/lpechacek/qa_test_klp
> > > [3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/
> > > 
> > > Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
> > > Signed-off-by: Marios Pomonis <pomonis@google.com>
> > > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> > > Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
> > > Cc: Sami Tolvanen <samitolvanen@google.com>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > 
> > Ping again; Josh can you take this please?
> 
> I'm confused how this still fixes anything after Masami's patch set,
> which is now in linux-next.
> 
> After those patches, for a CALL-type ORC entry, the unwinder sets
> state->ip to the address returned by unwind_recover_ret_addr().  In the
> case of a kretprobe, that means that state->ip will no longer point to
> kretprobes_trampoline() -- making the above patch description incorrect.
> 
> Instead, state->ip will then contain the original call return address
> which was replaced by kretpobes.  So it looks to the unwinder like a
> normal call return address, and 'state->signal' should remain false.
> 
> Am I missing something?

I'll let Marios answer in more detail, but my understanding is that
Masami's patch set didn't solve the FGKASLR-vs-kretprobes issue[1].
I don't understand _why_ yet, though.

-Kees

[1] https://lore.kernel.org/all/CAKXAmdgS3SL_qyjzjY32_DXe3WVTN+O=wYwJ9vkUXKhjmt87fA@mail.gmail.com/

-- 
Kees Cook

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Masami Hiramatsu]

On Wed, 13 Oct 2021 21:52:36 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Wed, Oct 13, 2021 at 06:41:01PM -0700, Josh Poimboeuf wrote:
> > On Mon, Oct 11, 2021 at 02:03:26PM -0700, Kees Cook wrote:
> > > On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> > > > From: Marios Pomonis <pomonis@google.com>
> > > > 
> > > > Fix a bug in the ORC unwinder when kretprobes has replaced a return
> > > > address with the address of `kretprobes_trampoline'. ORC mistakenly
> > > > assumes that the address in the stack is a return address and decrements
> > > > it by 1 in order to find the proper depth of the next frame.

Hmm, with my fixes[1], the kretprobe_trampoline address in the stack will be
replaced with the correct return address. In that case, that behavior
sounds correct.

[1] https://lore.kernel.org/all/163163030719.489837.2236069935502195491.stgit@devnote2/

> > > > 
> > > > This issue was discovered while testing the FG-KASLR series[0][1] and
> > > > running the live patching test[2] that was originally failing[3].
> > > > 
> > > > [0] https://lore.kernel.org/kernel-hardening/20200923173905.11219-1-kristen@linux.intel.com/
> > > > [1] https://github.com/KSPP/linux/issues/132
> > > > [2] https://github.com/lpechacek/qa_test_klp
> > > > [3] https://lore.kernel.org/lkml/alpine.LSU.2.21.2009251450260.13615@pobox.suse.cz/
> > > > 
> > > > Fixes: ee9f8fce9964 ("x86/unwind: Add the ORC unwinder")
> > > > Signed-off-by: Marios Pomonis <pomonis@google.com>
> > > > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > > > Cc: Alexander Lobakin <alexandr.lobakin@intel.com>
> > > > Cc: Kristen C Accardi <kristen.c.accardi@intel.com>
> > > > Cc: Sami Tolvanen <samitolvanen@google.com>
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > 
> > > Ping again; Josh can you take this please?
> > 
> > I'm confused how this still fixes anything after Masami's patch set,
> > which is now in linux-next.
> > 
> > After those patches, for a CALL-type ORC entry, the unwinder sets
> > state->ip to the address returned by unwind_recover_ret_addr().  In the
> > case of a kretprobe, that means that state->ip will no longer point to
> > kretprobes_trampoline() -- making the above patch description incorrect.
> > 
> > Instead, state->ip will then contain the original call return address
> > which was replaced by kretpobes.  So it looks to the unwinder like a
> > normal call return address, and 'state->signal' should remain false.
> > 
> > Am I missing something?
> 
> I'll let Marios answer in more detail, but my understanding is that
> Masami's patch set didn't solve the FGKASLR-vs-kretprobes issue[1].
> I don't understand _why_ yet, though.

My another question is that this fix still works correctly on my series,
because it's already merged via Steve's tree. Isn't this break anything?

I also need to know how (from where) the failed test uses stacktrace.
Does it call stacktrace from outside of kretprobe?

Thank you,

> 
> -Kees
> 
> [1] https://lore.kernel.org/all/CAKXAmdgS3SL_qyjzjY32_DXe3WVTN+O=wYwJ9vkUXKhjmt87fA@mail.gmail.com/
> 
> -- 
> Kees Cook


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Masami Hiramatsu]

On Thu, 14 Oct 2021 19:16:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 13 Oct 2021 21:52:36 -0700
> Kees Cook <keescook@chromium.org> wrote:
> 
> > On Wed, Oct 13, 2021 at 06:41:01PM -0700, Josh Poimboeuf wrote:
> > > On Mon, Oct 11, 2021 at 02:03:26PM -0700, Kees Cook wrote:
> > > > On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> > > > > From: Marios Pomonis <pomonis@google.com>
> > > > > 
> > > > > Fix a bug in the ORC unwinder when kretprobes has replaced a return
> > > > > address with the address of `kretprobes_trampoline'. ORC mistakenly
> > > > > assumes that the address in the stack is a return address and decrements
> > > > > it by 1 in order to find the proper depth of the next frame.
> 
> Hmm, with my fixes[1], the kretprobe_trampoline address in the stack will be
> replaced with the correct return address. In that case, that behavior
> sounds correct.
> 
> [1] https://lore.kernel.org/all/163163030719.489837.2236069935502195491.stgit@devnote2/

Here is the code which I applied this on my series.

        /* Find IP, SP and possibly regs: */
        switch (orc->type) {
        case UNWIND_HINT_TYPE_CALL:
                ip_p = sp - sizeof(long);

                if (!deref_stack_reg(state, ip_p, &state->ip))
                        goto err;

                state->ip = unwind_recover_ret_addr(state, state->ip,
                                                    (unsigned long *)ip_p);
                state->sp = sp;
                state->regs = NULL;
                state->prev_regs = NULL;
                state->signal = is_kretprobe_trampoline(state->ip);
                break;

Actually, this cause a build issue because I introduced more generic is_kretprobe_trampoline().
Anyway, after calling unwind_recover_ret_addr(), the state->ip should be fixed.
This means that the is_kretprobe_trampoline(state->ip) always be false, and
that is correct because state->ip is recovered with the correct return address
which is call instruction + 5.

So this patch seems not needed, hmm...

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/unwind/orc: Handle kretprobes_trampoline

[Marios Pomonis]

On Thu, Oct 21, 2021 at 8:13 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Thu, 14 Oct 2021 19:16:43 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > On Wed, 13 Oct 2021 21:52:36 -0700
> > Kees Cook <keescook@chromium.org> wrote:
> >
> > > On Wed, Oct 13, 2021 at 06:41:01PM -0700, Josh Poimboeuf wrote:
> > > > On Mon, Oct 11, 2021 at 02:03:26PM -0700, Kees Cook wrote:
> > > > > On Thu, Sep 02, 2021 at 07:13:26PM -0700, Kees Cook wrote:
> > > > > > From: Marios Pomonis <pomonis@google.com>
> > > > > >
> > > > > > Fix a bug in the ORC unwinder when kretprobes has replaced a return
> > > > > > address with the address of `kretprobes_trampoline'. ORC mistakenly
> > > > > > assumes that the address in the stack is a return address and decrements
> > > > > > it by 1 in order to find the proper depth of the next frame.
> >
> > Hmm, with my fixes[1], the kretprobe_trampoline address in the stack will be
> > replaced with the correct return address. In that case, that behavior
> > sounds correct.
> >
> > [1] https://lore.kernel.org/all/163163030719.489837.2236069935502195491.stgit@devnote2/
>
> Here is the code which I applied this on my series.
>
>         /* Find IP, SP and possibly regs: */
>         switch (orc->type) {
>         case UNWIND_HINT_TYPE_CALL:
>                 ip_p = sp - sizeof(long);
>
>                 if (!deref_stack_reg(state, ip_p, &state->ip))
>                         goto err;
>
>                 state->ip = unwind_recover_ret_addr(state, state->ip,
>                                                     (unsigned long *)ip_p);
>                 state->sp = sp;
>                 state->regs = NULL;
>                 state->prev_regs = NULL;
>                 state->signal = is_kretprobe_trampoline(state->ip);
>                 break;
>
> Actually, this cause a build issue because I introduced more generic is_kretprobe_trampoline().
> Anyway, after calling unwind_recover_ret_addr(), the state->ip should be fixed.
> This means that the is_kretprobe_trampoline(state->ip) always be false, and
> that is correct because state->ip is recovered with the correct return address
> which is call instruction + 5.
>
> So this patch seems not needed, hmm...
>
> Thank you,
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>

You're right, I made a mistake when testing this code; this is what
happens when you create patches with debugging changes and then forget
to remove them. I re-checked and your patch does solve the issue, so
the cover mail fix is not needed (I had created it against the
then-linux-next branch which didn't include your patch).

Thanks for catching this and I apologize for the (very) late response!