linux-kernel - Re: [Xen-devel] [PATCH v4 0/5] [PATCH v3 0/5] Improve non-"safe" MSR access failure handling

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: <56E6CB6D.5080906@oracle.com>
Date:	Mon, 14 Mar 2016 10:32:13 -0400
From:	Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:	Andy Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>
Cc:	KVM list <kvm@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	xen-devel <Xen-devel@...ts.xen.org>,
	Borislav Petkov <bp@...en8.de>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Arjan van de Ven <arjan@...ux.intel.com>
Subject: Re: [Xen-devel] [PATCH v4 0/5] [PATCH v3 0/5] Improve non-"safe" MSR
 access failure handling

On 03/12/2016 01:08 PM, Andy Lutomirski wrote:
> Setting CONFIG_PARAVIRT=y has an unintended side effect: it silently
> turns all rdmsr and wrmsr operations into the safe variants without
> any checks that the operations actually succeed.
>
> With CONFIG_PARAVIRT=n, unchecked MSR failures OOPS and probably
> cause boot to fail if they happen before init starts.
>
> Neither behavior is very good, and it's particularly unfortunate that
> the behavior changes depending on CONFIG_PARAVIRT.
>
> In particular, KVM guests might be unwittingly depending on the
> PARAVIRT=y behavior because CONFIG_KVM_GUEST currently depends on
> CONFIG_PARAVIRT, and, because accesses in that case are completely
> unchecked, we wouldn't even see a warning.
>
> This series changes the native behavior, regardless of
> CONFIG_PARAVIRT.  A non-"safe" MSR failure will give an informative
> warning once and will be fixed up -- native_read_msr will return
> zero, and both reads and writes will continue where they left off.
>
> If panic_on_oops is set, they will still OOPS and panic.
>
> By using the shiny new custom exception handler infrastructure,
> there should be no overhead on the success paths.
>
> I didn't change the behavior on Xen, but, with this series applied,
> it would be straightforward for the Xen maintainers to make the
> corresponding change -- knowledge of whether the access is "safe" is
> now propagated into the pvops.
>
> Doing this is probably a prerequisite to sanely decoupling
> CONFIG_KVM_GUEST and CONFIG_PARAVIRT, which would probably make
> Arjan and the rest of the Clear Containers people happy :)
>
> There's also room to reduce the code size of the "safe" variants
> using custom exception handlers in the future.
>
> Changes from v3:
>   - WARN_ONCE instead of WARN (Ingo)
>   - In the warning text, s/unsafe/unchecked/ (Ingo, sort of)
>
> Changes from earlier versions: lots of changes!
>
> Andy Lutomirski (5):
>    x86/paravirt: Add _safe to the read_msr and write_msr PV hooks
>    x86/msr: Carry on after a non-"safe" MSR access fails without
>      !panic_on_oops
>    x86/paravirt: Add paravirt_{read,write}_msr
>    x86/paravirt: Make "unsafe" MSR accesses unsafe even if PARAVIRT=y
>    x86/msr: Set the return value to zero when native_rdmsr_safe fails
>
>   arch/x86/include/asm/msr.h            | 20 ++++++++++++----
>   arch/x86/include/asm/paravirt.h       | 45 +++++++++++++++++++++--------------
>   arch/x86/include/asm/paravirt_types.h | 14 +++++++----
>   arch/x86/kernel/paravirt.c            |  6 +++--
>   arch/x86/mm/extable.c                 | 33 +++++++++++++++++++++++++
>   arch/x86/xen/enlighten.c              | 27 +++++++++++++++++++--
>   6 files changed, 114 insertions(+), 31 deletions(-)
>

I don't see any issues as far as Xen is concerned but let me run this 
through our nightly. I'll wait for the next version (which I think you 
might have based on the comments). Please copy me.

-boris