Blocking CR and MSR writes via mem_access?

Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

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?

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.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Jan Beulich]

>>> On 02.10.14 at 12:49, <rcojocaru@bitdefender.com> wrote:
> 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.

So what do you expect the effect of denying the write to be?
Wouldn't crashing the guest explicitly have about the same effect?

> 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?

I don't think you should break existing behavior. If you need an
event before the setting gets carried out, just add another one.

Jan

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/02/14 14:39, Jan Beulich wrote:
>>>> On 02.10.14 at 12:49, <rcojocaru@bitdefender.com> wrote:
>> 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.
> 
> So what do you expect the effect of denying the write to be?
> Wouldn't crashing the guest explicitly have about the same effect?

Thanks for the quick reply!

Denying a normal, legitimate write, would indeed be a problem along the
lines of what you are describing, but the point would be to block
malicious writes that would modify the SYSCALL entry point, disable SMAP
/ SMEP, and so on.

>> 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?
> 
> I don't think you should break existing behavior. If you need an
> event before the setting gets carried out, just add another one.

Understood.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Andrew Cooper]

On 02/10/14 12:46, Razvan Cojocaru wrote:
> On 10/02/14 14:39, Jan Beulich wrote:
>>>>> On 02.10.14 at 12:49, <rcojocaru@bitdefender.com> wrote:
>>> 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.
>> So what do you expect the effect of denying the write to be?
>> Wouldn't crashing the guest explicitly have about the same effect?
> Thanks for the quick reply!
>
> Denying a normal, legitimate write, would indeed be a problem along the
> lines of what you are describing, but the point would be to block
> malicious writes that would modify the SYSCALL entry point, disable SMAP
> / SMEP, and so on.

So your use case is to protect a running VM which is under active attack
without crashing the domain wholesale?

I presume you then want to degrade the illegitimate writes to nops?

~Andrew

Re: Blocking CR and MSR writes via mem_access?

[Jan Beulich]

>>> On 02.10.14 at 13:46, <rcojocaru@bitdefender.com> wrote:
> On 10/02/14 14:39, Jan Beulich wrote:
>>>>> On 02.10.14 at 12:49, <rcojocaru@bitdefender.com> wrote:
>>> 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.
>> 
>> So what do you expect the effect of denying the write to be?
>> Wouldn't crashing the guest explicitly have about the same effect?
> 
> Thanks for the quick reply!
> 
> Denying a normal, legitimate write, would indeed be a problem along the
> lines of what you are describing, but the point would be to block
> malicious writes that would modify the SYSCALL entry point, disable SMAP
> / SMEP, and so on.

I'd be really curious to know how you envision telling a malicious from
a legitimate operation here.

Jan

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/02/14 14:51, Andrew Cooper wrote:
> On 02/10/14 12:46, Razvan Cojocaru wrote:
>> On 10/02/14 14:39, Jan Beulich wrote:
>>>>>> On 02.10.14 at 12:49, <rcojocaru@bitdefender.com> wrote:
>>>> 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.
>>> So what do you expect the effect of denying the write to be?
>>> Wouldn't crashing the guest explicitly have about the same effect?
>> Thanks for the quick reply!
>>
>> Denying a normal, legitimate write, would indeed be a problem along the
>> lines of what you are describing, but the point would be to block
>> malicious writes that would modify the SYSCALL entry point, disable SMAP
>> / SMEP, and so on.
> 
> So your use case is to protect a running VM which is under active attack
> without crashing the domain wholesale?
> 
> I presume you then want to degrade the illegitimate writes to nops?

Yes, pretty much.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/02/14 14:51, Jan Beulich wrote:
>>>> On 02.10.14 at 13:46, <rcojocaru@bitdefender.com> wrote:
>> On 10/02/14 14:39, Jan Beulich wrote:
>>>>>> On 02.10.14 at 12:49, <rcojocaru@bitdefender.com> wrote:
>>>> 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.
>>>
>>> So what do you expect the effect of denying the write to be?
>>> Wouldn't crashing the guest explicitly have about the same effect?
>>
>> Thanks for the quick reply!
>>
>> Denying a normal, legitimate write, would indeed be a problem along the
>> lines of what you are describing, but the point would be to block
>> malicious writes that would modify the SYSCALL entry point, disable SMAP
>> / SMEP, and so on.
> 
> I'd be really curious to know how you envision telling a malicious from
> a legitimate operation here.

As part of the security logic, SMEP / SMAP shouldn't be disabled and the
SYSCALL entry point should not be modified. PatchGuard watches for this,
but when it is somehow compromised, or otherwise unavailable, the guest
would still be protected.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2038 bytes --]

On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru <rcojocaru@bitdefender.com>
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


>
> Thanks,
> Razvan Cojocaru
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2896 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2410 bytes --]

On 03/10/14 13:32, Tamas K Lengyel wrote:
>
> On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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.

Having a "pre-write" event hook which the listener can register for
(instead of the post-write hook) sounds like a plausible plan, where the
result of the event can be Yes/No/"Do this in stead".

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 4355 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/03/14 15:32, Tamas K Lengyel wrote:
> 
> On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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.

Hello Tamas, thanks for the reply! I was hoping to hear from a fellow
mem_event user. :)

Noted, as per your (and Jan's) suggestion, I won't touch the existing CR
events.

>     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.

Indeed, thanks for the idea! I was thinking doing that (rather than than
just rejecting a pre-write event) might impact performance, but for one
your solution is more elegant (doesn't duplicate CR events), and I don't
think there would be many instances of when the value needs to be
changed back anyway.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/03/14 15:37, Andrew Cooper wrote:
> On 03/10/14 13:32, Tamas K Lengyel wrote:
>>
>> On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
>> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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.
> 
> Having a "pre-write" event hook which the listener can register for
> (instead of the post-write hook) sounds like a plausible plan, where the
> result of the event can be Yes/No/"Do this in stead".

The way I've been thinking about that was to add new mem_event flags
(for example, MEM_EVENT_FLAG_SET_MSR), which could be set by the dom0
monitoring application if the MSR write is allowed to happen, then in
p2m_mem_access_resume() check for the flag and set some per-VCPU data to
signal that a MSR write should happen, for this MSR and that value.

A bool_t parameter could be added to hvm_msr_write_intercept(), let's
call it "mem_event", and if mem_event is true (and mem_access is
enabled, MSR events are requested, etc.), return X86EMUL_OKAY right
after the hvm_memory_event_msr(msr, msr_content) call (i.e. send out the
MSR mem_event and do nothing else). Calling it with mem_event == 1 would
be the default for all the code calling hvm_msr_write_intercept() now.

Then, somewhere (where?) similar to vmx_vmenter_helper() in purpose,
check the data and, if necessary, call hvm_msr_write_intercept() with
mem_event == false, which would trigger the actual write (with _no_ MSR
mem_event sent).

Would this be likely to blow up anything? If not, could you recommend a
candidate function to place the MSR setting call? Can't do it in
p2m_mem_access_resume(), since "current" is not right there.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2908 bytes --]

On Fri, Oct 3, 2014 at 2:37 PM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

>  On 03/10/14 13:32, Tamas K Lengyel wrote:
>
>
>  On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru <
> rcojocaru@bitdefender.com> 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.


>
> Having a "pre-write" event hook which the listener can register for
> (instead of the post-write hook) sounds like a plausible plan, where the
> result of the event can be Yes/No/"Do this in stead".
>

I think that's an overkill and wouldn't really get you much extra. But if
its decided to be added, it sounds just fine to me.

Tamas


>
> ~Andrew
>

[-- Attachment #1.2: Type: text/html, Size: 5539 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/03/14 19:22, Tamas K Lengyel wrote:
> 
> 
> On Fri, Oct 3, 2014 at 2:37 PM, Andrew Cooper <andrew.cooper3@citrix.com
> <mailto:andrew.cooper3@citrix.com>> wrote:
> 
>     On 03/10/14 13:32, Tamas K Lengyel wrote:
>>
>>     On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
>>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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.

It is a reasonable assumption in our case too, so at least for now
Tamas' suggestion should work for CR post-write events.

It would indeed be better if the previous value would be sent for MSR
events, the difference being that the MSR event is being sent in
hvm_msr_write_intercept() where the old value is not immediately
available (this is why I've implemented this event as not honouring
HVMPME_onchangeonly when I've initially submitted the patch a few years
ago).


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/03/2014 07:22 PM, Tamas K Lengyel wrote:
> 
> 
> On Fri, Oct 3, 2014 at 2:37 PM, Andrew Cooper <andrew.cooper3@citrix.com
> <mailto:andrew.cooper3@citrix.com>> wrote:
> 
>     On 03/10/14 13:32, Tamas K Lengyel wrote:
>>
>>     On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
>>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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?


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 4955 bytes --]

On Mon, Oct 6, 2014 at 4:25 PM, Razvan Cojocaru <rcojocaru@bitdefender.com>
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@citrix.com
> > <mailto:andrew.cooper3@citrix.com>> wrote:
> >
> >     On 03/10/14 13:32, Tamas K Lengyel wrote:
> >>
> >>     On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
> >>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>>
> 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.

Tamas


>
>
> Thanks,
> Razvan Cojocaru
>

[-- Attachment #1.2: Type: text/html, Size: 6546 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/07/2014 11:59 AM, Tamas K Lengyel wrote:
> 
> 
> On Mon, Oct 6, 2014 at 4:25 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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@citrix.com <mailto:andrew.cooper3@citrix.com>
>     > <mailto:andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>>> wrote:
>     >
>     >     On 03/10/14 13:32, Tamas K Lengyel wrote:
>     >>
>     >>     On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
>     >>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>     <mailto:rcojocaru@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>> 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

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/07/2014 01:21 PM, Razvan Cojocaru wrote:
> On 10/07/2014 11:59 AM, Tamas K Lengyel wrote:
>>
>>
>> On Mon, Oct 6, 2014 at 4:25 PM, Razvan Cojocaru
>> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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@citrix.com <mailto:andrew.cooper3@citrix.com>
>>     > <mailto:andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>>> wrote:
>>     >
>>     >     On 03/10/14 13:32, Tamas K Lengyel wrote:
>>     >>
>>     >>     On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
>>     >>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>>     <mailto:rcojocaru@bitdefender.com
>>     <mailto:rcojocaru@bitdefender.com>>> 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.

Sorry, meant to say "if we put the old value in .offset, it will be a
special case [...]".


Thanks,
Razvan

Re: Blocking CR and MSR writes via mem_access?

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 7653 bytes --]

On Tue, Oct 7, 2014 at 12:48 PM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 10/07/2014 01:21 PM, Razvan Cojocaru wrote:
> > On 10/07/2014 11:59 AM, Tamas K Lengyel wrote:
> >>
> >>
> >> On Mon, Oct 6, 2014 at 4:25 PM, Razvan Cojocaru
> >> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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@citrix.com <mailto:andrew.cooper3@citrix.com>
> >>     > <mailto:andrew.cooper3@citrix.com <mailto:
> andrew.cooper3@citrix.com>>> wrote:
> >>     >
> >>     >     On 03/10/14 13:32, Tamas K Lengyel wrote:
> >>     >>
> >>     >>     On Thu, Oct 2, 2014 at 12:49 PM, Razvan Cojocaru
> >>     >>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com
> >
> >>     <mailto:rcojocaru@bitdefender.com
> >>     <mailto:rcojocaru@bitdefender.com>>> 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.
>
> Sorry, meant to say "if we put the old value in .offset, it will be a
> special case [...]".
>
>
> Thanks,
> Razvan
>

IMHO that entire struct is in dire need of a cleanup. It is really hacky to
have fields like gla and gfn transfer values that mean different things
under different event type that don't have anything to do with gla/gfn.
It's sort of just a legacy struct from the time when we only had EPT events
and everything else just got hacked on top. I would be in favor of having
the struct as a union of substructs that nicely define all the values that
are transferred in the given context, with meaningful struct member names.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 10368 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Jan Beulich]

>>> On 07.10.14 at 14:30, <tamas.lengyel@zentific.com> wrote:
> IMHO that entire struct is in dire need of a cleanup. It is really hacky to
> have fields like gla and gfn transfer values that mean different things
> under different event type that don't have anything to do with gla/gfn.
> It's sort of just a legacy struct from the time when we only had EPT events
> and everything else just got hacked on top. I would be in favor of having
> the struct as a union of substructs that nicely define all the values that
> are transferred in the given context, with meaningful struct member names.

And the whole thing isn't just "mem-event" anymore either ...

Jan

Re: Blocking CR and MSR writes via mem_access?

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 989 bytes --]

On Tue, Oct 7, 2014 at 2:40 PM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 07.10.14 at 14:30, <tamas.lengyel@zentific.com> wrote:
> > IMHO that entire struct is in dire need of a cleanup. It is really hacky
> to
> > have fields like gla and gfn transfer values that mean different things
> > under different event type that don't have anything to do with gla/gfn.
> > It's sort of just a legacy struct from the time when we only had EPT
> events
> > and everything else just got hacked on top. I would be in favor of having
> > the struct as a union of substructs that nicely define all the values
> that
> > are transferred in the given context, with meaningful struct member
> names.
>
> And the whole thing isn't just "mem-event" anymore either ...
>
> Jan
>

And that as well =) The whole mem_access and mem_event distinction is also
pretty blurry. Might worth coming up with a better name while also
combining the two into something more consistent.. xen-events perhaps?

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1543 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/07/2014 03:40 PM, Jan Beulich wrote:
>>>> On 07.10.14 at 14:30, <tamas.lengyel@zentific.com> wrote:
>> IMHO that entire struct is in dire need of a cleanup. It is really hacky to
>> have fields like gla and gfn transfer values that mean different things
>> under different event type that don't have anything to do with gla/gfn.
>> It's sort of just a legacy struct from the time when we only had EPT events
>> and everything else just got hacked on top. I would be in favor of having
>> the struct as a union of substructs that nicely define all the values that
>> are transferred in the given context, with meaningful struct member names.
> 
> And the whole thing isn't just "mem-event" anymore either ...

I agree with the union improvement, and indeed, we might need a more
appropriate name for the whole thing (xen-event?).

I guess one question would be how this would impact existing clients,
but there probably aren't that many, and the changes required to replace
the .gla / .gfn logic should be minimal.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1292 bytes --]

On 07/10/14 13:46, Tamas K Lengyel wrote:
>
>
> On Tue, Oct 7, 2014 at 2:40 PM, Jan Beulich <JBeulich@suse.com
> <mailto:JBeulich@suse.com>> wrote:
>
>     >>> On 07.10.14 at 14:30, <tamas.lengyel@zentific.com <mailto:tamas.lengyel@zentific.com>> wrote:
>     > IMHO that entire struct is in dire need of a cleanup. It is
>     really hacky to
>     > have fields like gla and gfn transfer values that mean different
>     things
>     > under different event type that don't have anything to do with
>     gla/gfn.
>     > It's sort of just a legacy struct from the time when we only had
>     EPT events
>     > and everything else just got hacked on top. I would be in favor
>     of having
>     > the struct as a union of substructs that nicely define all the
>     values that
>     > are transferred in the given context, with meaningful struct
>     member names.
>
>     And the whole thing isn't just "mem-event" anymore either ...
>
>     Jan
>
>
> And that as well =) The whole mem_access and mem_event distinction is
> also pretty blurry. Might worth coming up with a better name while
> also combining the two into something more consistent.. xen-events
> perhaps?
>
> Tamas
>

The "xen" ought to be explicit given the prefix on the hypercalls. 
"vm-events" as a name?

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 2831 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/07/2014 03:49 PM, Andrew Cooper wrote:
> The "xen" ought to be explicit given the prefix on the hypercalls. 
> "vm-events" as a name?

It's a good name.


Thanks,
Razvan

Re: Blocking CR and MSR writes via mem_access?

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 591 bytes --]

On Tue, Oct 7, 2014 at 2:55 PM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 10/07/2014 03:49 PM, Andrew Cooper wrote:
> > The "xen" ought to be explicit given the prefix on the hypercalls.
> > "vm-events" as a name?
>
> It's a good name.
>
>
> Thanks,
> Razvan
>

Provided that in the future we might want to include events generated by
hypervisor emulation / memory accesses into this event delivery system it
might be a bit misleading. Perhaps just events or monitor-events? Either
way, vm-events is already an improvement and for now it  certainly sounds
appropriate.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1087 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

On 10/07/2014 03:58 PM, Tamas K Lengyel wrote:
> 
> 
> On Tue, Oct 7, 2014 at 2:55 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 10/07/2014 03:49 PM, Andrew Cooper wrote:
>     > The "xen" ought to be explicit given the prefix on the hypercalls.
>     > "vm-events" as a name?
> 
>     It's a good name.
> 
> 
>     Thanks,
>     Razvan
> 
> 
> Provided that in the future we might want to include events generated by
> hypervisor emulation / memory accesses into this event delivery system
> it might be a bit misleading. Perhaps just events or monitor-events?
> Either way, vm-events is already an improvement and for now it 
> certainly sounds appropriate.

I suppose that it might be possible to send non-VM related events in the
future (unless we count dom0 as a VM and any event not related to
another VM a dom0-related event?). I'd be happy with any of the choices.


Thanks,
Razvan Cojocaru

Re: Blocking CR and MSR writes via mem_access?

[Razvan Cojocaru]

> 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);
> }

Coming back to this, testing showed that the values were indeed _not_
written. Looking at the code, we end up in arch_set_info_guest() in
xen/arch/x86/domain.c, which does a few things and then, for HVM guests,
calls hvm_set_info_guest() and pretty much exits:

 741     if ( is_hvm_vcpu(v) )
 742     {
 743         hvm_set_info_guest(v);
 744         goto out;
 745     }

In the VMX case, this doesn't do much (it definitely doesn't set any CR
registers):

1542 static void vmx_set_info_guest(struct vcpu *v)
1543 {
1544     unsigned long intr_shadow;
1545
1546     vmx_vmcs_enter(v);
1547
1548     __vmwrite(GUEST_DR7, v->arch.debugreg[7]);
1549
1550     /*
1551      * If the interruptibility-state field indicates blocking by STI,
1552      * setting the TF flag in the EFLAGS may cause VM entry to fail
1553      * and crash the guest. See SDM 3B 22.3.1.5.
1554      * Resetting the VMX_INTR_SHADOW_STI flag looks hackish but
1555      * to set the GUEST_PENDING_DBG_EXCEPTIONS.BS here incurs
1556      * immediately vmexit and hence make no progress.
1557      */
1558     __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_shadow);
1559     if ( v->domain->debugger_attached &&
1560          (v->arch.user_regs.eflags & X86_EFLAGS_TF) &&
1561          (intr_shadow & VMX_INTR_SHADOW_STI) )
1562     {
1563         intr_shadow &= ~VMX_INTR_SHADOW_STI;
1564         __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
1565     }
1566
1567     vmx_vmcs_exit(v);
1568 }

Hope this helps others considering going down this road.


Regards,
Razvan