[PATCH v2 0/2] x86: kprobes: Fix CFI_CLANG related issues

[PATCH v2 0/2] x86: kprobes: Fix CFI_CLANG related issues

[Masami Hiramatsu (Google)]

Hi,

Here is the 2nd version of the kprobe patches for kernel CFI.
Previous version is here;

https://lore.kernel.org/all/168899125356.80889.17967397360941194229.stgit@devnote2/

In this version "__pfx_" prefix symbols also are prohibited and that check
is done unconditionally [1/2].

- Prohibit probing on __cfi_* and __pfx_* preamble symbols, which have CFI info.
- Prohibit probing on compiler generated movl/addl which is used for
  detecting typeid on x86.

Thank you,

---

Masami Hiramatsu (Google) (2):
      kprobes: Prohibit probing on CFI preamble symbol
      x86/kprobes: Prohibit probing on compiler generated CFI checking code


 arch/x86/kernel/kprobes/core.c |   34 ++++++++++++++++++++++++++++++++++
 kernel/kprobes.c               |   14 +++++++++++++-
 2 files changed, 47 insertions(+), 1 deletion(-)

--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH v2 1/2] kprobes: Prohibit probing on CFI preamble symbol

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Do not allow to probe on "__cfi_" or "__pfx_" started symbol, because those
are used for CFI and not executed. Probing it will break the CFI.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Check "__pfx_" prefix functions too.
  - Make the check unconditional.
---
 kernel/kprobes.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 00e177de91cc..3da9726232ff 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1545,6 +1545,17 @@ static int check_ftrace_location(struct kprobe *p)
 	return 0;
 }
 
+static bool is_cfi_preamble_symbol(unsigned long addr)
+{
+	char symbuf[KSYM_NAME_LEN];
+
+	if (lookup_symbol_name(addr, symbuf))
+		return false;
+
+	return str_has_prefix("__cfi_", symbuf) ||
+		str_has_prefix("__pfx_", symbuf);
+}
+
 static int check_kprobe_address_safe(struct kprobe *p,
 				     struct module **probed_mod)
 {
@@ -1563,7 +1574,8 @@ static int check_kprobe_address_safe(struct kprobe *p,
 	    within_kprobe_blacklist((unsigned long) p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr) ||
 	    static_call_text_reserved(p->addr, p->addr) ||
-	    find_bug((unsigned long)p->addr)) {
+	    find_bug((unsigned long)p->addr) ||
+	    is_cfi_preamble_symbol((unsigned long)p->addr)) {
 		ret = -EINVAL;
 		goto out;
 	}

Re: [PATCH v2 1/2] kprobes: Prohibit probing on CFI preamble symbol

[Steven Rostedt]

On Tue, 11 Jul 2023 10:50:47 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Do not allow to probe on "__cfi_" or "__pfx_" started symbol, because those
> are used for CFI and not executed. Probing it will break the CFI.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

> ---
>  Changes in v2:
>   - Check "__pfx_" prefix functions too.
>   - Make the check unconditional.
> ---
>  kernel/kprobes.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

Re: [PATCH v2 1/2] kprobes: Prohibit probing on CFI preamble symbol

[Masami Hiramatsu]

On Fri, 28 Jul 2023 18:49:13 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 11 Jul 2023 10:50:47 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Do not allow to probe on "__cfi_" or "__pfx_" started symbol, because those
> > are used for CFI and not executed. Probing it will break the CFI.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thanks! I will pick this for probes/fixes.


> 
> -- Steve
> 
> > ---
> >  Changes in v2:
> >   - Check "__pfx_" prefix functions too.
> >   - Make the check unconditional.
> > ---
> >  kernel/kprobes.c |   14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH v2 2/2] x86/kprobes: Prohibit probing on compiler generated CFI checking code

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Prohibit probing on the compiler generated CFI typeid checking code
because it is used for decoding typeid when CFI error happens.

The compiler generates the following instruction sequence for indirect
call checks on x86;

   movl    -<id>, %r10d       ; 6 bytes
   addl    -4(%reg), %r10d    ; 4 bytes
   je      .Ltmp1             ; 2 bytes
   ud2                        ; <- regs->ip

And handle_cfi_failure() decodes these instructions (movl and addl)
for the typeid and the target address. Thus if we put a kprobe on
those instructions, the decode will fail and report a wrong typeid
and target address.


Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/kprobes/core.c |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index f7f6042eb7e6..fa8c2b41cbaf 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -54,6 +54,7 @@
 #include <asm/insn.h>
 #include <asm/debugreg.h>
 #include <asm/ibt.h>
+#include <asm/cfi.h>
 
 #include "common.h"
 
@@ -293,7 +294,40 @@ static int can_probe(unsigned long paddr)
 #endif
 		addr += insn.length;
 	}
+	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
+		/*
+		 * The compiler generates the following instruction sequence
+		 * for indirect call checks and cfi.c decodes this;
+		 *
+		 *   movl    -<id>, %r10d       ; 6 bytes
+		 *   addl    -4(%reg), %r10d    ; 4 bytes
+		 *   je      .Ltmp1             ; 2 bytes
+		 *   ud2                        ; <- regs->ip
+		 *   .Ltmp1:
+		 *
+		 * Also, these movl and addl are used for showing expected
+		 * type. So those must not be touched.
+		 */
+		__addr = recover_probed_instruction(buf, addr);
+		if (!__addr)
+			return 0;
+
+		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
+			return 0;
+
+		if (insn.opcode.value == 0xBA)
+			offset = 12;
+		else if (insn.opcode.value == 0x3)
+			offset = 6;
+		else
+			goto out;
+
+		/* This movl/addl is used for decoding CFI. */
+		if (is_cfi_trap(addr + offset))
+			return 0;
+	}
 
+out:
 	return (addr == paddr);
 }
 

Re: [PATCH v2 2/2] x86/kprobes: Prohibit probing on compiler generated CFI checking code

[Masami Hiramatsu]

On Tue, 11 Jul 2023 10:50:58 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Prohibit probing on the compiler generated CFI typeid checking code
> because it is used for decoding typeid when CFI error happens.
> 
> The compiler generates the following instruction sequence for indirect
> call checks on x86;
> 
>    movl    -<id>, %r10d       ; 6 bytes
>    addl    -4(%reg), %r10d    ; 4 bytes
>    je      .Ltmp1             ; 2 bytes
>    ud2                        ; <- regs->ip
> 
> And handle_cfi_failure() decodes these instructions (movl and addl)
> for the typeid and the target address. Thus if we put a kprobe on
> those instructions, the decode will fail and report a wrong typeid
> and target address.
> 
> 

Hi Peter,

Can I pick this to probes/fixes branch ?

Thank you,

> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kernel/kprobes/core.c |   34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index f7f6042eb7e6..fa8c2b41cbaf 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -54,6 +54,7 @@
>  #include <asm/insn.h>
>  #include <asm/debugreg.h>
>  #include <asm/ibt.h>
> +#include <asm/cfi.h>
>  
>  #include "common.h"
>  
> @@ -293,7 +294,40 @@ static int can_probe(unsigned long paddr)
>  #endif
>  		addr += insn.length;
>  	}
> +	if (IS_ENABLED(CONFIG_CFI_CLANG)) {
> +		/*
> +		 * The compiler generates the following instruction sequence
> +		 * for indirect call checks and cfi.c decodes this;
> +		 *
> +		 *   movl    -<id>, %r10d       ; 6 bytes
> +		 *   addl    -4(%reg), %r10d    ; 4 bytes
> +		 *   je      .Ltmp1             ; 2 bytes
> +		 *   ud2                        ; <- regs->ip
> +		 *   .Ltmp1:
> +		 *
> +		 * Also, these movl and addl are used for showing expected
> +		 * type. So those must not be touched.
> +		 */
> +		__addr = recover_probed_instruction(buf, addr);
> +		if (!__addr)
> +			return 0;
> +
> +		if (insn_decode_kernel(&insn, (void *)__addr) < 0)
> +			return 0;
> +
> +		if (insn.opcode.value == 0xBA)
> +			offset = 12;
> +		else if (insn.opcode.value == 0x3)
> +			offset = 6;
> +		else
> +			goto out;
> +
> +		/* This movl/addl is used for decoding CFI. */
> +		if (is_cfi_trap(addr + offset))
> +			return 0;
> +	}
>  
> +out:
>  	return (addr == paddr);
>  }
>  
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 2/2] x86/kprobes: Prohibit probing on compiler generated CFI checking code

[Peter Zijlstra]

On Wed, Jul 26, 2023 at 12:23:17PM +0900, Masami Hiramatsu wrote:
> On Tue, 11 Jul 2023 10:50:58 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Prohibit probing on the compiler generated CFI typeid checking code
> > because it is used for decoding typeid when CFI error happens.
> > 
> > The compiler generates the following instruction sequence for indirect
> > call checks on x86;
> > 
> >    movl    -<id>, %r10d       ; 6 bytes
> >    addl    -4(%reg), %r10d    ; 4 bytes
> >    je      .Ltmp1             ; 2 bytes
> >    ud2                        ; <- regs->ip
> > 
> > And handle_cfi_failure() decodes these instructions (movl and addl)
> > for the typeid and the target address. Thus if we put a kprobe on
> > those instructions, the decode will fail and report a wrong typeid
> > and target address.
> > 
> > 
> 
> Hi Peter,
> 
> Can I pick this to probes/fixes branch ?

I'll stick them in tip/x86/core, that ok?

Re: [PATCH v2 2/2] x86/kprobes: Prohibit probing on compiler generated CFI checking code

[Masami Hiramatsu]

On Wed, 26 Jul 2023 11:29:17 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Jul 26, 2023 at 12:23:17PM +0900, Masami Hiramatsu wrote:
> > On Tue, 11 Jul 2023 10:50:58 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> > 
> > > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > > 
> > > Prohibit probing on the compiler generated CFI typeid checking code
> > > because it is used for decoding typeid when CFI error happens.
> > > 
> > > The compiler generates the following instruction sequence for indirect
> > > call checks on x86;
> > > 
> > >    movl    -<id>, %r10d       ; 6 bytes
> > >    addl    -4(%reg), %r10d    ; 4 bytes
> > >    je      .Ltmp1             ; 2 bytes
> > >    ud2                        ; <- regs->ip
> > > 
> > > And handle_cfi_failure() decodes these instructions (movl and addl)
> > > for the typeid and the target address. Thus if we put a kprobe on
> > > those instructions, the decode will fail and report a wrong typeid
> > > and target address.
> > > 
> > > 
> > 
> > Hi Peter,
> > 
> > Can I pick this to probes/fixes branch ?
> 
> I'll stick them in tip/x86/core, that ok?

Yes, since it is for CFI change. 

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>