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

* Linus Torvalds (torvalds@xxxxxxxxxxxxxxxxxxxx) 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@xxxxxxxxxxxx>

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@xxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: Russ Coopers AusCERT Presentation on MS Security Bulletins
    ... but this gal in SBSland thinks that non-patching is NOT the ... I'll take a Security hotfix anyday, thank you, ... feel that I get 100% in my lan of patching. ... there on XP sp2 RC, firewall in place AND the Sasser patch in place, I ...
    (NT-Bugtraq)
  • Re: Patch Management on Critical Servers (Healthcare)
    ... *nix servers patch management is handled at two levels. ... meeting and approved, especially patching. ... change meetings for the hospitals and dates set. ...
    (Focus-Microsoft)
  • Re: [Full-Disclosure] RE: Linux (in)security
    ... > There's a vast difference in having to backout patches in complex ... And I don't recall the last time that we had to back out a patch in an over ... There isn't a vast difference between patching ... Windows and patching *nix. ...
    (Full-Disclosure)
  • Re: change page protection, how to
    ... I have a very small stub of code ... not possible to circumwent it from user mode. ... patch an application. ... For patching dlls, ...
    (comp.lang.asm.x86)
  • Re: Java - JRE, SDK Java Web Start
    ... vendors in a propriety path i.e. program ... held accountable for the patching of java when they ... it doesn't matter whether you install in your ... All that matters is you patch it, ...
    (Vuln-Dev)