Re: [Xen-devel] Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/07/2014 11:59 AM, Tamas K Lengyel wrote:
> 
> 
> On Mon, Oct 6, 2014 at 4:25 PM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx <mailto:rcojocaru@xxxxxxxxxxxxxxx>> wrote:
> 
>     On 10/03/2014 07:22 PM, Tamas K Lengyel wrote:
>     >
>     >
>     > On Fri, Oct 3, 2014 at 2:37 PM, Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx <mailto:andrew.cooper3@xxxxxxxxxx>
>     > <mailto:andrew.cooper3@xxxxxxxxxx <mailto:andrew.cooper3@xxxxxxxxxx>>> 
> wrote:
>     >
>     >     On 03/10/14 13:32, Tamas K Lengyel wrote:
>     >>
>     >>     On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
>     >>     <rcojocaru@xxxxxxxxxxxxxxx <mailto:rcojocaru@xxxxxxxxxxxxxxx>
>     <mailto:rcojocaru@xxxxxxxxxxxxxxx
>     <mailto:rcojocaru@xxxxxxxxxxxxxxx>>> wrote:
>     >>
>     >>         Hello,
>     >>
>     >>         Currently hvm_memory_event_cr3() and the other
>     >>         hvm_memory_event_*()
>     >>         functions in hvm.c can pause the VCPU and send a mem_event
>     >>         with the new
>     >>         value of the respective register, but especially in the
>     case of CR
>     >>         events (as opposed to MSR events), this is done _after_ the
>     >>         value is set
>     >>         (please see hvm_set_cr3() in hvm.c).
>     >>
>     >>         It would be interesting from a memory introspection
>     >>         application's point
>     >>         of view to be able to receive a mem_event _before_ the value
>     >>         is set, and
>     >>         important to be able to veto the change.
>     >>
>     >>         A few questions:
>     >>
>     >>         1. Would it be acceptable to move the CR3 event sending code
>     >>         so that a
>     >>         mem_access client would receive the event _before_ the
>     write takes
>     >>         place? Is this likely to break other mem_event clients that
>     >>         might rely
>     >>         on the event being received _after_ the value has been set?
>     >>
>     >>
>     >>     Yes, it would break existing applications.
>     >>
>     >>
>     >>         2. I see that mem_event responses from all these cases (EPT
>     >>         violations,
>     >>         CR, MSR) are handled in p2m.c's p2m_mem_access_resume()
>     (seems
>     >>         to be
>     >>         confirmed by testing). Is this correct?
>     >>
>     >>         3. What would be the sanest, most elegant way to modify
>     Xen so
>     >>         that
>     >>         after a mem_event reply is being received for one of these
>     >>         cases (CR,
>     >>         MSR), the write will then be rejected? I'm asking because, as
>     >>         always,
>     >>         ideally this would also benefit other Xen users and an
>     elegant
>     >>         patch is
>     >>         always more likely to find its way into mainline than a quick
>     >>         hack.
>     >>
>     >>
>     >>     You can already block such writes with the existing post-write
>     >>     event delivery. If you are continuously watching for writes, you
>     >>     know what the previous value was (for CR events it is actually
>     >>     delivered to you by Xen as well as per my recent patch). If you
>     >>     don't like a particular new value that was set, just reset it to
>     >>     the value you had / want.
>     >>
>     >>     Tamas
>     >
>     >     That doesn't work if you join an event listener between the
>     previous
>     >     MSR write and one you wish to veto.
>     >
>     >
>     > Yes, that's correct. That's why I said it works if you continuously
>     > monitor for writes. I think that's a reasonable assumption. We could
>     > also make the MSR write events deliver the previous value as well
>     > similar to how the CR events do it. Anyway, AFAIU the hardware traps
>     > always happen before the write so technically both approaches are
>     > pre-write from the guest's perspective.
> 
>     I've actually been looking at this for a bit, and while it's true that
>     it might work for CR events, it's less clear how that would work for
>     MSRs.
> 
>     The CR part might be done in the following fashion:
> 
>     vcpu_guest_context_any_t ctx;
> 
>     if (xc_vcpu_getcontext(xch, domain, req.vcpu_id, &ctx) == 0) {
>             ctx.c.ctrlreg[crNumber] = req.gla; /* old value */
>             xc_vcpu_setcontext(xch, domain, req.vcpu_id, &ctx);
>     }
> 
>     However, only a very small number of MSRs are being offered for
>     manipulation from dom0 via libxc
>     (xen/include/public/arch-x86/hvm/save.h):
> 
>     struct hvm_hw_cpu {
>     /* ... */
>         /* msr content saved/restored. */
>         uint64_t msr_flags;
>         uint64_t msr_lstar;
>         uint64_t msr_star;
>         uint64_t msr_cstar;
>         uint64_t msr_syscall_mask;
>         uint64_t msr_efer;
>         uint64_t msr_tsc_aux;
>     /* ... */
>     };
> 
>     So this would either require expanding the vcpu_guest_context_any_t
>     information, or adding a new hypercall that sets MSRs (which would wrap
>     / ultimately end up calling msr_write_intercept(msr, value)).
> 
>     Am I missing something obvious here, or were you explictly only talking
>     about CRs?
> 
> 
> TBH I haven't looked at MSR events in detail. Conceptually I would
> expect them to be similar to CR events, so I would expect the
> write-blocking your outline above to be doable for MSRs as well (if the
> API gives you the required info). I think expanding the structure
> information to return all MSRs of interest sounds more reasonable then
> adding a new hypercall but I don't really know the details to be the
> judge of it.

The MSR part is actually more finicky than CRs.

For one, with CR events you can send the new value in .gfn and the old
value in .gla, but the way MSR events are being sent now, .gfn has the
MSR address and .gla the value to be written. So
hvm_memory_event_traps() would need to be modified to maybe use .offset
too for MSR events, but that doesn't look very clean: if we try to
modify the MSR events to match the Xen 4.5 CR events, we need to use
.gla and .gfn for the old and new values, and .offset for MSR address -
but that breaks compatibility with current MSR event consumers. And if
we put the MSR address in .offset, it will be a special case in
hvm_memory_event_traps(), and that's inelegant to say the least.

Then there's the other issue of adding .msr[] to ctx.c in the example
code above, similar to .ctrlreg[], but as far as I can tell, there's
quite a large number of total possible MSRs that would then have to end
up in the vcpucontext of all VCPUs.


Thanks,
Razvan

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