linux-kernel - x86/paravirt: Detect over-sized patching bugs in paravirt_patch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190425095039.GC115378@gmail.com>
Date:   Thu, 25 Apr 2019 11:50:39 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Juergen Gross <jgross@...e.com>,
        Andi Kleen <ak@...ux.intel.com>
Subject: x86/paravirt: Detect over-sized patching bugs in
 paravirt_patch_call()


* Peter Zijlstra <peterz@...radead.org> wrote:

> On Thu, Apr 25, 2019 at 11:17:17AM +0200, Ingo Molnar wrote:
> > It basically means that we silently won't do any patching and the kernel 
> > will crash later on in mysterious ways, because paravirt patching is 
> > usually relied on.
> 
> That's OK. The compiler emits an indirect CALL/JMP to the pv_ops
> structure contents. That _should_ stay valid and function correctly at
> all times.

It might result in a correctly executing kernel in terms of code 
generation, but it doesn't result in a viable kernel: some of the places 
rely on the patching going through and don't know what to do when it 
doesn't and misbehave or crash in interesting ways.

Guess how I know this. ;-)

> Not patching should at the very least cause a WARN with RETPOLINE 
> kernels though, we hard rely on the patching actually working and 
> writing at least a direct call.

We hard rely in other places too.

How about just BUG_ON()-ing in paravirt_patch_call() as well? It's not 
like these are *supposed* to fail, and if they do we want to know it, 
even if the outcome might be more benign in some cases?

I.e. how about the patch below? This not only makes it more apparent when 
patching fails, it also makes the kernel smaller and removes an #ifdef 
ugly.

I tried it with a richly paravirt-enabled kernel and no patching bugs 
were detected.

Thanks,

	Ingo

Signed-off-by: Ingo Molnar <mingo@...nel.org>

---
 arch/x86/kernel/paravirt.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 7f9121f2fdac..544d386ded45 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -73,21 +73,21 @@ struct branch {
 static unsigned paravirt_patch_call(void *insnbuf, const void *target,
 				    unsigned long addr, unsigned len)
 {
+	const int call_len = 5;
 	struct branch *b = insnbuf;
-	unsigned long delta = (unsigned long)target - (addr+5);
+	unsigned long delta = (unsigned long)target - (addr+call_len);
 
-	if (len < 5) {
-#ifdef CONFIG_RETPOLINE
-		WARN_ONCE(1, "Failing to patch indirect CALL in %ps\n", (void *)addr);
-#endif
-		return len;	/* call too long for patch site */
+	if (len < call_len) {
+		pr_warn("paravirt: Failed to patch indirect CALL at %ps\n", (void *)addr);
+		/* Kernel might not be viable if patching fails, bail out: */
+		BUG_ON(1);
 	}
 
 	b->opcode = 0xe8; /* call */
 	b->delta = delta;
-	BUILD_BUG_ON(sizeof(*b) != 5);
+	BUILD_BUG_ON(sizeof(*b) != call_len);
 
-	return 5;
+	return call_len;
 }
 
 #ifdef CONFIG_PARAVIRT_XXL