[PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[Razvan Cojocaru]

It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear())
to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates
vcpu->arch.vm_event) has been called, so return an error from the
XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/include/asm-x86/monitor.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0954b59..0544836 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
     switch ( mop->op )
     {
     case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
+    {
+        bool_t value = !!mop->event;
         domain_pause(d);
-        d->arch.mem_access_emulate_each_rep = !!mop->event;
+        /*
+         * Enabling emulate_each_rep without a vm_event subscriber
+         * is meaningless.
+         */
+        if ( !d->vcpu || !d->vcpu[0]->arch.vm_event )
+        {
+            domain_unpause(d);
+            return -EINVAL;
+        }
+        d->arch.mem_access_emulate_each_rep = value;
         domain_unpause(d);
         break;
-
+    }
     default:
         return -EOPNOTSUPP;
     }
-- 
1.9.1


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

Re: [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[Razvan Cojocaru]

On 04/08/16 10:10, Razvan Cojocaru wrote:
> It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear())
> to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates
> vcpu->arch.vm_event) has been called, so return an error from the
> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/include/asm-x86/monitor.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 0954b59..0544836 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>      switch ( mop->op )
>      {
>      case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
> +    {
> +        bool_t value = !!mop->event;
>          domain_pause(d);
> -        d->arch.mem_access_emulate_each_rep = !!mop->event;
> +        /*
> +         * Enabling emulate_each_rep without a vm_event subscriber
> +         * is meaningless.
> +         */
> +        if ( !d->vcpu || !d->vcpu[0]->arch.vm_event )

Sorry, this is a bit convoluted, and wrong: if d->vcpu != NULL it will
return -EINVAL even if d->vcpu[0]->arch.vm_event is NULL. I'll rework
it. Apologies for the noise.


Thanks,
Razvan

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

Re: [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[Andrew Cooper]

On 08/04/2016 08:16, Razvan Cojocaru wrote:
> On 04/08/16 10:10, Razvan Cojocaru wrote:
>> It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear())
>> to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates
>> vcpu->arch.vm_event) has been called, so return an error from the
>> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> ---
>>  xen/include/asm-x86/monitor.h | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
>> index 0954b59..0544836 100644
>> --- a/xen/include/asm-x86/monitor.h
>> +++ b/xen/include/asm-x86/monitor.h
>> @@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>>      switch ( mop->op )
>>      {
>>      case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
>> +    {
>> +        bool_t value = !!mop->event;

While you are at at, one extra newline here please.

>>          domain_pause(d);
>> -        d->arch.mem_access_emulate_each_rep = !!mop->event;
>> +        /*
>> +         * Enabling emulate_each_rep without a vm_event subscriber
>> +         * is meaningless.
>> +         */
>> +        if ( !d->vcpu || !d->vcpu[0]->arch.vm_event )
> Sorry, this is a bit convoluted, and wrong: if d->vcpu != NULL it will
> return -EINVAL even if d->vcpu[0]->arch.vm_event is NULL. I'll rework
> it. Apologies for the noise.

The issue comes about because d->arch.mem_access_emulate_each_rep is a
per-domain variable, whereas vm_event listeners are per-vcpu.

In principle, it would be legitimate for an introspection engine to only
want to introspect a single vcpu.

Either we need to enforce that every vcpu has vm_event infrastructure
set up (and allow for delivery of events is disabled on a per-vcpu
basis), or settings such as emulate_each_rep needs to be made per-vcpu.

~Andrew

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

Re: [PATCH] x86/monitor: Disallow setting mem_access_emulate_each_rep when vm_event is NULL

[Razvan Cojocaru]

On 04/08/16 11:17, Andrew Cooper wrote:
> On 08/04/2016 08:16, Razvan Cojocaru wrote:
>> On 04/08/16 10:10, Razvan Cojocaru wrote:
>>> It is meaningless (and potentially dangerous - see hvmemul_virtual_to_linear())
>>> to set mem_access_emulate_each_rep before xc_monitor_enable() (which allocates
>>> vcpu->arch.vm_event) has been called, so return an error from the
>>> XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP hypercall when that is the case.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> ---
>>>  xen/include/asm-x86/monitor.h | 15 +++++++++++++--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
>>> index 0954b59..0544836 100644
>>> --- a/xen/include/asm-x86/monitor.h
>>> +++ b/xen/include/asm-x86/monitor.h
>>> @@ -35,11 +35,22 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>>>      switch ( mop->op )
>>>      {
>>>      case XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP:
>>> +    {
>>> +        bool_t value = !!mop->event;
> 
> While you are at at, one extra newline here please.
> 
>>>          domain_pause(d);
>>> -        d->arch.mem_access_emulate_each_rep = !!mop->event;
>>> +        /*
>>> +         * Enabling emulate_each_rep without a vm_event subscriber
>>> +         * is meaningless.
>>> +         */
>>> +        if ( !d->vcpu || !d->vcpu[0]->arch.vm_event )
>> Sorry, this is a bit convoluted, and wrong: if d->vcpu != NULL it will
>> return -EINVAL even if d->vcpu[0]->arch.vm_event is NULL. I'll rework
>> it. Apologies for the noise.
> 
> The issue comes about because d->arch.mem_access_emulate_each_rep is a
> per-domain variable, whereas vm_event listeners are per-vcpu.
> 
> In principle, it would be legitimate for an introspection engine to only
> want to introspect a single vcpu.
> 
> Either we need to enforce that every vcpu has vm_event infrastructure
> set up (and allow for delivery of events is disabled on a per-vcpu
> basis), or settings such as emulate_each_rep needs to be made per-vcpu.

Xc_monitor_enable() does set vm_event up for all of the domain's VCPUs,
so at least for now it is a fair assumption that if it is set for the
first one, it's set for all of them. In other words, your first
requirement is currently being implicitly enforced.

Introspecting a single VCPU is an interesting idea, but not terribly
useful at this point.


Thanks,
Razvan

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