Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Jul 2023 18:16:43 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc:     Petr Pavlu <petr.pavlu@suse.com>, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, samitolvanen@google.com, x86@kernel.org,
        linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] x86/kprobes: Prohibit probing on compiler
 generated CFI checking code
Message-ID: <20230710161643.GB3040258@hirez.programming.kicks-ass.net>
References: <168899125356.80889.17967397360941194229.stgit@devnote2>
 <168899127520.80889.15418363018799407058.stgit@devnote2>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <168899127520.80889.15418363018799407058.stgit@devnote2>
Precedence: bulk

On Mon, Jul 10, 2023 at 09:14:35PM +0900, Masami Hiramatsu (Google) wrote:
> +	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;

Notably, the JE will already be avoided?

> +
> +		/* This movl/addl is used for decoding CFI. */
> +		if (is_cfi_trap(addr + offset))
> +			return 0;
> +	}
>  
> +out:
>  	return (addr == paddr);
>  }

Hmm, so I was thinking something like the below, which also catches
things when we rewrite kCFI to FineIBT, except I don't think we care if
the FineIBT callsite gets re-written. FineIBT only relies on the __cfi_
symbol not getting poked at (which the previous patches should ensure).

Additionally is_cfi_trap() is horrifically slow -- it's a full linear
search of the entire kcfi_traps array, it doesn't have any smarts on
(#UD can hardly be considered a fast path).

So I tihnk I'm ok with the above, just adding the below for reference
(completely untested and everything).


Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>


---
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index f7f6042eb7e6..b812dee76909 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -293,6 +293,11 @@ static int can_probe(unsigned long paddr)
 #endif
 		addr += insn.length;
 	}
+	/*
+	 * Don't allow poking the kCFI/FineIBT callsites.
+	 */
+	if (IS_ENABLED(CONFIG_CFI_CLANG) && cfi_call_site(addr))
+		return 0;
 
 	return (addr == paddr);
 }
diff --git a/kernel/cfi.c b/kernel/cfi.c
index 08caad776717..2656e6ffa013 100644
--- a/kernel/cfi.c
+++ b/kernel/cfi.c
@@ -31,16 +31,22 @@ static inline unsigned long trap_address(s32 *p)
 	return (unsigned long)((long)p + (long)*p);
 }
 
-static bool is_trap(unsigned long addr, s32 *start, s32 *end)
+static long cfi_trap_distance(unsigned long addr, s32 *start, s32 *end)
 {
+	long dist = LONG_MAX;
 	s32 *p;
 
 	for (p = start; p < end; ++p) {
-		if (trap_address(p) == addr)
-			return true;
+		long d = trap_address(p) - addr;
+
+		if (abs(dist) < abs(d)) {
+			dist = d;
+			if (dist == 0)
+				return 0;
+		}
 	}
 
-	return false;
+	return dist;
 }
 
 #ifdef CONFIG_MODULES
@@ -66,25 +72,25 @@ void module_cfi_finalize(const Elf_Ehdr *hdr, const Elf_Shdr *sechdrs,
 	}
 }
 
-static bool is_module_cfi_trap(unsigned long addr)
+static long module_cfi_trap_distance(unsigned long addr)
 {
 	struct module *mod;
-	bool found = false;
+	long dist = LONG_MAX;
 
 	rcu_read_lock_sched_notrace();
 
 	mod = __module_address(addr);
 	if (mod)
-		found = is_trap(addr, mod->kcfi_traps, mod->kcfi_traps_end);
+		dist = cfi_trap_distance(addr, mod->kcfi_traps, mod->kcfi_traps_end);
 
 	rcu_read_unlock_sched_notrace();
 
 	return found;
 }
 #else /* CONFIG_MODULES */
-static inline bool is_module_cfi_trap(unsigned long addr)
+static long module_cfi_trap_distance(unsigned long addr)
 {
-	return false;
+	return LONG_MAX;
 }
 #endif /* CONFIG_MODULES */
 
@@ -93,9 +99,24 @@ extern s32 __stop___kcfi_traps[];
 
 bool is_cfi_trap(unsigned long addr)
 {
-	if (is_trap(addr, __start___kcfi_traps, __stop___kcfi_traps))
+	long dist = cfi_trap_distance(addr, __start___kcfi_traps, __stop___kcfi_traps);
+	if (!dist)
+		return true;
+
+	return module_cfi_trap_distance(addr) == 0;
+}
+
+bool cfi_call_site(unsigned long addr)
+{
+	long dist = cfi_trap_distance(addr, __start___kcfi_traps, __stop___kcfi_traps);
+	if (dist >= -12 && dist <= 0)
+		return true;
+
+	dist = module_cfi_trap_distance(addr);
+	if (dist >= -12 && dist <= 0)
 		return true;
 
-	return is_module_cfi_trap(addr);
+	return false;
 }
+
 #endif /* CONFIG_ARCH_USES_CFI_TRAPS */