[PATCH] x86/emulate: Check current->arch.vm_event in hvmemul_virtual_to_linear()

[PATCH] x86/emulate: Check current->arch.vm_event in hvmemul_virtual_to_linear()

[Razvan Cojocaru]

Theoretically it is possible for mem_access_emulate_each_rep to be
true even when current->arch.vm_event == NULL, so add an extra
check to hvmemul_virtual_to_linear().

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/hvm/emulate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index f5ab5bc..91413d2 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -514,7 +514,7 @@ static int hvmemul_virtual_to_linear(
      * vm_event being triggered for repeated writes to a whole page.
      */
     if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) &&
-         current->arch.vm_event->emulate_flags != 0 )
+         current->arch.vm_event && current->arch.vm_event->emulate_flags != 0 )
        max_reps = 1;
 
     /*
-- 
1.9.1


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

Re: [PATCH] x86/emulate: Check current->arch.vm_event in hvmemul_virtual_to_linear()

[Jan Beulich]

>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote:
> Theoretically it is possible for mem_access_emulate_each_rep to be
> true even when current->arch.vm_event == NULL, so add an extra
> check to hvmemul_virtual_to_linear().

Mind saying what those theoretical conditions are when this might
happen?

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -514,7 +514,7 @@ static int hvmemul_virtual_to_linear(
>       * vm_event being triggered for repeated writes to a whole page.
>       */
>      if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) &&
> -         current->arch.vm_event->emulate_flags != 0 )
> +         current->arch.vm_event && current->arch.vm_event->emulate_flags != 0 )

That's then the third instance of "current" here - this needs
latching into a local variable.

Jan


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

Re: [PATCH] x86/emulate: Check current->arch.vm_event in hvmemul_virtual_to_linear()

[Razvan Cojocaru]

On 04/07/16 20:27, Jan Beulich wrote:
>>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote:
>> Theoretically it is possible for mem_access_emulate_each_rep to be
>> true even when current->arch.vm_event == NULL, so add an extra
>> check to hvmemul_virtual_to_linear().
> 
> Mind saying what those theoretical conditions are when this might
> happen?

This could happen if someone were to call xc_monitor_emulate_each_rep(),
but not xc_monitor_enable() (when current->arch.vm_event gets
allocated), or after someone called both, but afterwards called
xc_monitor_disable() (when current->arch.vm_event gets freed).

>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -514,7 +514,7 @@ static int hvmemul_virtual_to_linear(
>>       * vm_event being triggered for repeated writes to a whole page.
>>       */
>>      if ( unlikely(current->domain->arch.mem_access_emulate_each_rep) &&
>> -         current->arch.vm_event->emulate_flags != 0 )
>> +         current->arch.vm_event && current->arch.vm_event->emulate_flags != 0 )
> 
> That's then the third instance of "current" here - this needs
> latching into a local variable.

No problem.


Thanks,
Razvan


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

Re: [PATCH] x86/emulate: Check current->arch.vm_event in hvmemul_virtual_to_linear()

[Razvan Cojocaru]

On 04/07/16 20:54, Razvan Cojocaru wrote:
> On 04/07/16 20:27, Jan Beulich wrote:
>>>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote:
>>> Theoretically it is possible for mem_access_emulate_each_rep to be
>>> true even when current->arch.vm_event == NULL, so add an extra
>>> check to hvmemul_virtual_to_linear().
>>
>> Mind saying what those theoretical conditions are when this might
>> happen?
> 
> This could happen if someone were to call xc_monitor_emulate_each_rep(),
> but not xc_monitor_enable() (when current->arch.vm_event gets
> allocated), or after someone called both, but afterwards called
> xc_monitor_disable() (when current->arch.vm_event gets freed).

Actually, I need to correct myself here: only the first case applies. I
was looking at older Xen source code, but there's a
"d->arch.mem_access_emulate_each_rep = 0;" statement in
vm_event_cleanup_domain() now, so calling xc_monitor_disable() would not
cause a problem.


Thanks,
Razvan

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

Re: [PATCH] x86/emulate: Check current->arch.vm_event in hvmemul_virtual_to_linear()

[Jan Beulich]

>>> On 07.04.16 at 19:54, <rcojocaru@bitdefender.com> wrote:
> On 04/07/16 20:27, Jan Beulich wrote:
>>>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote:
>>> Theoretically it is possible for mem_access_emulate_each_rep to be
>>> true even when current->arch.vm_event == NULL, so add an extra
>>> check to hvmemul_virtual_to_linear().
>> 
>> Mind saying what those theoretical conditions are when this might
>> happen?
> 
> This could happen if someone were to call xc_monitor_emulate_each_rep(),
> but not xc_monitor_enable() (when current->arch.vm_event gets
> allocated), or after someone called both, but afterwards called
> xc_monitor_disable() (when current->arch.vm_event gets freed).

Then wouldn't the correct action be to fail
xc_monitor_emulate_each_rep() (i.e. whatever hypercall this
resolves to) when the monitor is not enabled? (You did already
clarify that the other variant isn't applicable)?

Jan


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

Re: [PATCH] x86/emulate: Check current->arch.vm_event in hvmemul_virtual_to_linear()

[Razvan Cojocaru]

On 04/08/16 00:17, Jan Beulich wrote:
>>>> On 07.04.16 at 19:54, <rcojocaru@bitdefender.com> wrote:
>> On 04/07/16 20:27, Jan Beulich wrote:
>>>>>> On 07.04.16 at 10:39, <rcojocaru@bitdefender.com> wrote:
>>>> Theoretically it is possible for mem_access_emulate_each_rep to be
>>>> true even when current->arch.vm_event == NULL, so add an extra
>>>> check to hvmemul_virtual_to_linear().
>>>
>>> Mind saying what those theoretical conditions are when this might
>>> happen?
>>
>> This could happen if someone were to call xc_monitor_emulate_each_rep(),
>> but not xc_monitor_enable() (when current->arch.vm_event gets
>> allocated), or after someone called both, but afterwards called
>> xc_monitor_disable() (when current->arch.vm_event gets freed).
> 
> Then wouldn't the correct action be to fail
> xc_monitor_emulate_each_rep() (i.e. whatever hypercall this
> resolves to) when the monitor is not enabled? (You did already
> clarify that the other variant isn't applicable)?

Yes, I think that's better. I'll do that instead.


Thanks,
Razvan

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