linux-kernel - Re: 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: <20190425105745.GA29840@gmail.com>
Date:   Thu, 25 Apr 2019 12:57:45 +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: Re: x86/paravirt: Detect over-sized patching bugs in
 paravirt_patch_call()


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

> On Thu, Apr 25, 2019 at 11:50:39AM +0200, Ingo Molnar wrote:
> > 
> > * 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. ;-)
> 
> What sites would that be? It really should work AFAIK.

So for example I tried to increasing the size of one of the struct 
patch_xxl members:

--- a/arch/x86/kernel/paravirt_patch.c
+++ b/arch/x86/kernel/paravirt_patch.c
@@ -28,7 +28,7 @@ struct patch_xxl {
 	const unsigned char	irq_restore_fl[2];
 # ifdef CONFIG_X86_64
 	const unsigned char	cpu_wbinvd[2];
-	const unsigned char	cpu_usergs_sysret64[6];
+	const unsigned char	cpu_usergs_sysret64[60];
 	const unsigned char	cpu_swapgs[3];
 	const unsigned char	mov64[3];
 # else

Which with the vanilla kernel crashes on boot much, much later:

[    2.478026] PANIC: double fault, error_code: 0x0

But in any case, even if many of the others will work if the patching 
fails silently, is there any case where we'd treat patching failure as an 
acceptable case?

BUG_ON() in paravirt kernels is an easily debuggable condition and beats 
the above kinds of symptoms. But I can turn it into a WARN_ON_ONCE() if 
you think that's better?

Thanks,

	Ingo