Re: [Xen-devel] [PATCH 0/3] x86/paravirt: Fix baremetal paravirt MSR ops

[Andy Lutomirski <luto@xxxxxxxxxxxxxx>]

On Thu, Sep 17, 2015 at 12:19 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Andy Lutomirski <luto@xxxxxxxxxx> 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.
>>
>> This is IMO awful: it papers over bugs.  In particular, KVM gueests
>> might be unwittingly depending on this behavior because
>> CONFIG_KVM_GUEST currently depends on CONFIG_PARAVIRT.  I'm not
>> aware of any such problems, but applying this series would be a good
>> way to shake them out.
>>
>> Fix it so that the MSR operations work the same on CONFIG_PARAVIRT=n
>> and CONFIG_PARAVIRT=y as long as Xen isn't being used.  The Xen
>> maintainers are welcome to make a similar change on top of this.
>>
>> Since there's plenty of time before the next merge window, I think
>> we should apply and fix anything that breaks.
>
> No, I think we should at most generate a warning instead, and not crash the 
> kernel
> via rdmsr()!
>
> Most big distro kernels on bare metal have CONFIG_PARAVIRT=y (I checked 
> Ubuntu and
> Fedora), so we are potentially exposing a lot of users to problems.
>
> Crashing the bootup on an unknown MSR is bad. Many MSR reads and writes are
> non-critical and returning the 'safe' result is much better than crashing or
> hanging the bootup.
>

Should we do that for CONFIG_PARAVIRT=n, too?

It would be straightforward to rig this up (temporarily?) on top of
these patches.  To keep bloat down, we might want to implement it in
do_general_protection rather than sticking it in native_read_msr.

wrmsr is a different beast, since we can fail due to writing the wrong
value to an otherwise valid MSR.  Given that MSR screwups can very
easily be security holes, I'm not sure that warning and blindly
continuing on an unchecked failed wrmsr is a good idea.

In any event, I think it's nuts that CONFIG_PARAVIRT changes this
behavior.  We should pick something sane and stick with it.
CONFIG_PARAVIRT=n appears to work just fine on bare metal in lots of
deployments, especially pre-KVM.  CONFIG_PARAVIRT=n also appears to
work fine under KVM, and I use it frequently.

> ( We should double check that rdmsr()/wrmsr() results are never left
>   uninitialized, but are set to zero or so, for cases where the return code 
> is not
>   checked. )

It sure looks like native_read_msr_safe doesn't clear the output if
the rdmsr fails.

--Andy

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel