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:   Wed, 21 Jun 2023 20:33:56 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     x86@kernel.org, alyssa.milburn@linux.intel.com,
        linux-kernel@vger.kernel.org, samitolvanen@google.com,
        jpoimboe@kernel.org, joao@overdrivepizza.com,
        tim.c.chen@linux.intel.com
Subject: Re: [PATCH 1/2] x86/cfi: Fix ret_from_fork indirect calls
Message-ID: <20230621183356.GP38236@hirez.programming.kicks-ass.net>
References: <20230615193546.949657149@infradead.org>
 <20230615193722.127844423@infradead.org>
 <202306201455.AF16F617A@keescook>
 <20230621085217.GI2046280@hirez.programming.kicks-ass.net>
 <20230621092759.GJ2046280@hirez.programming.kicks-ass.net>
 <202306211107.4D529222C@keescook>
 <20230621181659.GB4253@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230621181659.GB4253@hirez.programming.kicks-ass.net>
Precedence: bulk

On Wed, Jun 21, 2023 at 08:16:59PM +0200, Peter Zijlstra wrote:
> On Wed, Jun 21, 2023 at 11:08:46AM -0700, Kees Cook wrote:
> 
> > Ah yeah, it should be direct-called only. I keep forgetting about the
> > endbr removal pass.
> > 
> > > I can't seem to manage to have it clobber it's __cfi hash value. Ideally
> > > we'd have an attribute to force the thing to 0 or something.
> > 
> > Doesn't objtool have logic to figure out this is only ever
> > direct-called?
> 
> It does; let me also use that same thing to clobber the kCFI hashes for
> these functions.

Completely untested... gotta go put the kids to bed. I'll try later.

--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -778,6 +778,8 @@ void __init_or_module noinline apply_ret
 
 #ifdef CONFIG_X86_KERNEL_IBT
 
+static void poison_hash(void *addr);
+
 static void __init_or_module poison_endbr(void *addr, bool warn)
 {
 	u32 endbr, poison = gen_endbr_poison();
@@ -802,6 +804,9 @@ static void __init_or_module poison_endb
 
 /*
  * Generated by: objtool --ibt
+ *
+ * Seal the functions for indirect calls by clobbering the ENDBR instructions
+ * and the kCFI hash value.
  */
 void __init_or_module noinline apply_ibt_endbr(s32 *start, s32 *end)
 {
@@ -812,7 +817,7 @@ void __init_or_module noinline apply_ibt
 
 		poison_endbr(addr, true);
 		if (IS_ENABLED(CONFIG_FINEIBT))
-			poison_endbr(addr - 16, false);
+			poison_hash(addr - 16);
 	}
 }
 
@@ -1193,6 +1198,38 @@ static void __apply_fineibt(s32 *start_r
 	pr_err("Something went horribly wrong trying to rewrite the CFI implementation.\n");
 }
 
+static inline void __poison_hash(void *addr)
+{
+	*(u32 *)hash = 0;
+}
+
+static void poison_hash(void *addr)
+{
+	switch (cfi_mode) {
+	case CFI_FINEIBT:
+		/*
+		 * __cfi_\func:
+		 *	osp nopl (%rax)
+		 *	subl	$0, %r10d
+		 *	jz	1f
+		 *	ud2
+		 * 1:	nop
+		 */
+		poison_endbr(addr, false);
+		__poison_hash(addr + 7);
+		break;
+
+	case CFI_KCFI:
+		/*
+		 * __cfi_\func:
+		 *	movl	$0, %eax
+		 *	.skip	11, 0x90
+		 */
+		__poison_hash(addr + 1);
+		break;
+	}
+}
+
 #else
 
 static void __apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,
@@ -1200,6 +1237,8 @@ static void __apply_fineibt(s32 *start_r
 {
 }
 
+static void poison_hash(void *addr) { }
+
 #endif
 
 void apply_fineibt(s32 *start_retpoline, s32 *end_retpoline,