linux-kernel - [PATCH] x86/paravirt: Detect oversized patching bugs as they happen and BUG

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: <20190425091717.GA72229@gmail.com>
Date:   Thu, 25 Apr 2019 11:17:17 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Juergen Gross <jgross@...e.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: [PATCH] x86/paravirt: Detect oversized patching bugs as they happen
 and BUG_ON() to avoid later crashes


* Ingo Molnar <mingo@...nel.org> wrote:

> Third, beyond readability there's another advantage of my suggested 
> approach as well: for example that way we could verify the passed in 
> length with the patchlet length. Right now it's completely unverified:
> 
>         case PARAVIRT_PATCH(ops.m):                                     \
>                 return PATCH(data, ops##_##m, ibuf, len)
> 
> right now we don't check whether the 'len' passed in by the usage site 
> matches the actual structure field length.
> 
> Although maybe we could do that with your C space structure as well.

So I was wrong here, got confused by the 'len' name which doesn't mean 
what it suggests: it's not the length of the patchlet, but the 
maximum/original length of the patch site - which we trim down in 
paravirt_patch_insns():

unsigned paravirt_patch_insns(void *insnbuf, unsigned len,
                              const char *start, const char *end)
{
        unsigned insn_len = end - start;

        if (insn_len > len || start == NULL)
                insn_len = len;
        else
                memcpy(insnbuf, start, insn_len);

        return insn_len;
}

What is the logic behind silently returning 'len' here and not copying 
anything?

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.

Instead I think we should BUG_ON() that condition with the patch below - 
there's no way to continue successfully at that point.

I've tested this patch, with the vanilla kernel check never triggers, and 
if I intentionally increase the size of one of the patch templates to a 
too high value the assert triggers:

[    0.164385] kernel BUG at arch/x86/kernel/paravirt.c:167!

Without this patch a broken kernel randomly crashes in later places, 
after the silent patching failure.

Thanks,

	Ingo

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

--- tip.orig/arch/x86/kernel/paravirt.c
+++ tip/arch/x86/kernel/paravirt.c
@@ -163,10 +163,10 @@ unsigned paravirt_patch_insns(void *insn
 {
 	unsigned insn_len = end - start;
 
-	if (insn_len > len || start == NULL)
-		insn_len = len;
-	else
-		memcpy(insnbuf, start, insn_len);
+	/* Alternative instruction is too large for the patch site and we cannot continue: */
+	BUG_ON(insn_len > len || start == NULL);
+
+	memcpy(insnbuf, start, insn_len);
 
 	return insn_len;
 }