Re: [PATCH 4/4] x86,module: Detect CRn and DRn manipulation

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/04/2020 22:22, Nadav Amit wrote:
>> On Apr 7, 2020, at 1:50 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Tue, Apr 07, 2020 at 01:27:45PM -0700, Nadav Amit wrote:
>>>> On Apr 7, 2020, at 12:38 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>>>
>>>> On Tue, Apr 07, 2020 at 11:55:21AM -0700, Nadav Amit wrote:
>>>>>> On Apr 7, 2020, at 4:02 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>>>>>
>>>>>> Since we now have infrastructure to analyze module text, disallow
>>>>>> modules that write to CRn and DRn registers.
>>>>> Assuming the kernel is built without CONFIG_PARAVIRT, what is the right way
>>>>> for out-of-tree modules to write to CRs? Let’s say CR2?
>>>> Most of them there is no real justification for ever writing to. CR2 I
>>>> suppose we can have an exception for given a sane rationale for why
>>>> you'd need to rewrite the fault address.
>>> For the same reason that KVM writes to CR2 - to restore CR2 before entering
>>> a guest, since CR2 not architecturally loaded from the VMCS. I suspect there
>>> are additional use-cases which are not covered by the kernel interfaces.
>> So I'm not much of a virt guy (clearly), and *groan*, that's horrible.
>> I'll go make an exception for CR2.
> Clearly you are not a virt guy if you think that this is the horrible part
> in x86 virtualization ;-)

Very definitely seconded :)

> Anyhow, I do not think it is the only use-case which is not covered by your
> patches (even considering CRs/DRs alone). For example, there is no kernel
> function to turn on CR4.VMXE, which is required to run hypervisors on x86.

How about taking this opportunity to see if there is a way to improve on
the status quo for co-existing hypervisor modules?

ISTR there are a large number of hoops to jump through if you're not
sure if anything else in the system is using VMX, going as far as
needing to do a full VMXON/VMXOFF cycle each context.

Enabling CR4.VMXE might want to be done proactively by the kernel. 
Amongst other things, it gives protection against stray INIT IPIs in the
system, although the interaction with SMX (tboot / trenchboot) wants
considering carefully.

~Andrew