linux-kernel - Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue

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: <20070818213141.GW3672@sequoia.sous-sol.org>
Date:	Sat, 18 Aug 2007 14:31:41 -0700
From:	Chris Wright <chrisw@...s-sol.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Chris Wright <chrisw@...s-sol.org>, Andi Kleen <ak@...e.de>,
	Jeremy Fitzhardinge <jeremy@...p.org>, patches@...-64.org,
	linux-kernel@...r.kernel.org, Rusty Russell <rusty@...tcorp.com.au>
Subject: Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt
	issue

* Linus Torvalds (torvalds@...ux-foundation.org) wrote:
> On Sat, 18 Aug 2007, Chris Wright wrote:
> > > 
> > > Check the latest git head. Does it still break?
> > 
> > Yeah, this is the latest git.  The broken commit is Rusty's patch which,
> > after Linus reverted the write-protected remap changes, is no longer
> > necessary.  AFAICT patching is writing garbage into the insn stream.
> > I suspect it's copying an uninitialized temp buffer.
> 
> Can you send me the revert patch that is verified to work?

Now that I understand the problem, I do have a very simple (slightly
overkill) fix for paravirt patching.  This can be cleaned up to avoid
the copies when they aren't needed, but that will take a little more
auditing of the various patchers.  If you still prefer a revert I've
got one handy, and we can re-visit this all post .23.

thanks,
-chris
--

Subject: [PATCH] x86: properly initialize temp insn buffer for paravirt patching 
From: Chris Wright <chrisw@...s-sol.org>

With commit ab144f5ec64c42218a555ec1dbde6b60cf2982d6 the patching code
now collects the complete new instruction stream into a temp buffer
before finally patching in the new insns.  In some cases the paravirt
patchers will choose to leave the patch site unpatched (length mismatch,
clobbers mismatch, etc).  This causes the new patching code to copy an
uninitialized temp buffer, i.e. garbage, to the callsite.  Simply make
sure to always initialize the buffer with the original instruction stream.
A better fix is to audit all the patchers and return proper length so that
apply_paravirt() can skip copies when we leave the patch site untouched.

Signed-off-by: Chris Wright <chrisw@...s-sol.org>
---
 arch/i386/kernel/alternative.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/i386/kernel/alternative.c b/arch/i386/kernel/alternative.c
index 1b66d5c..9f4ac8b 100644
--- a/arch/i386/kernel/alternative.c
+++ b/arch/i386/kernel/alternative.c
@@ -366,6 +366,8 @@ void apply_paravirt(struct paravirt_patch_site *start,
 		unsigned int used;

 		BUG_ON(p->len > MAX_PATCH_LEN);
+		/* prep the buffer with the original instructions */
+		memcpy(insnbuf, p->instr, p->len);
 		used = paravirt_ops.patch(p->instrtype, p->clobbers, insnbuf,
 					  (unsigned long)p->instr, p->len);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/