Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-15 Thread George Dunlap 
On 14/09/16 16:11, Tamas K Lengyel wrote:
> On Wed, Sep 14, 2016 at 7:38 AM, George Dunlap  
> wrote:
>> On 13/09/16 19:12, Tamas K Lengyel wrote:
>>> Setting response flags in vm_event are only ever safe if the vCPUs are 
>>> paused.
>>> To reflect this we move all checks within the if block that already checks
>>> whether this is the case. Checks that are only supported on one architecture
>>> we relocate the bitmask operations to the arch-specific handlers to avoid
>>> the overhead on architectures that don't support it.
>>>
>>> Furthermore, we clean-up the emulation checks so it more clearly represents 
>>> the
>>> decision-logic when emulation should take place. As part of this we also
>>> set the stage to allow emulation in response to other types of events, not 
>>> just
>>> mem_access violations.
>>>
>>> Signed-off-by: Tamas K Lengyel 
>>
>> Tamas,
>>
>> Would you like a detailed review from me for this?  I'm happy to ack the
>> p2m bits on the basis that they're only touching mem_access code.  A
>> full review may get stuck behind a pretty long review backlog. :-(
>>
>>  -George
> 
> Hi George,
> acking just the p2m bits should suffice!

I should have given that in the first e-mail really. :-)

p2m bits:

Acked-by: George Dunlap 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-14 Thread Tamas K Lengyel 
On Wed, Sep 14, 2016 at 9:15 AM, Julien Grall  wrote:
>
>
> On 14/09/16 16:14, Tamas K Lengyel wrote:
>>
>> On Wed, Sep 14, 2016 at 3:33 AM, Julien Grall 
>> wrote:
>>>
>>> Hello Tamas,
>>>
>>> On 13/09/16 19:12, Tamas K Lengyel wrote:


 diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
 index 53c4d78..5e9bc54 100644
 --- a/xen/include/asm-arm/p2m.h
 +++ b/xen/include/asm-arm/p2m.h
 @@ -121,10 +121,10 @@ typedef enum {
   p2m_to_mask(p2m_map_foreign)))

  static inline
 -void p2m_mem_access_emulate_check(struct vcpu *v,
 +bool_t p2m_mem_access_emulate_check(struct vcpu *v,
const vm_event_response_t *rsp)
>>>
>>>
>>>
>>> s/bool_t/bool/ and please indent properly the second line.
>>
>>
>> Fine by me but bool_t is used throughout p2m.h and I see no use of
>> just bool. Is there actually any difference between the two to warrant
>> enforcing one over the other?
>
>
> bool_t has been turned into an alias to bool recently. Moving all the source
> code from one to another is a long task (very similar to mfn/gfn typesafe).
>
> However, new code should use bool and not bool_t.

Ack.

Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-14 Thread Julien Grall 



On 14/09/16 16:14, Tamas K Lengyel wrote:

On Wed, Sep 14, 2016 at 3:33 AM, Julien Grall  wrote:

Hello Tamas,

On 13/09/16 19:12, Tamas K Lengyel wrote:


diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 53c4d78..5e9bc54 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -121,10 +121,10 @@ typedef enum {
  p2m_to_mask(p2m_map_foreign)))

 static inline
-void p2m_mem_access_emulate_check(struct vcpu *v,
+bool_t p2m_mem_access_emulate_check(struct vcpu *v,
   const vm_event_response_t *rsp)



s/bool_t/bool/ and please indent properly the second line.


Fine by me but bool_t is used throughout p2m.h and I see no use of
just bool. Is there actually any difference between the two to warrant
enforcing one over the other?


bool_t has been turned into an alias to bool recently. Moving all the 
source code from one to another is a long task (very similar to mfn/gfn 
typesafe).


However, new code should use bool and not bool_t.

Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-14 Thread Tamas K Lengyel 
On Wed, Sep 14, 2016 at 3:33 AM, Julien Grall  wrote:
> Hello Tamas,
>
> On 13/09/16 19:12, Tamas K Lengyel wrote:
>>
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 53c4d78..5e9bc54 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -121,10 +121,10 @@ typedef enum {
>>   p2m_to_mask(p2m_map_foreign)))
>>
>>  static inline
>> -void p2m_mem_access_emulate_check(struct vcpu *v,
>> +bool_t p2m_mem_access_emulate_check(struct vcpu *v,
>>const vm_event_response_t *rsp)
>
>
> s/bool_t/bool/ and please indent properly the second line.

Fine by me but bool_t is used throughout p2m.h and I see no use of
just bool. Is there actually any difference between the two to warrant
enforcing one over the other?

>
> Please indent properly the second line.

Ack.

>
> Regards,
>
> --
> Julien Grall

Thanks,
Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-14 Thread Tamas K Lengyel 
On Wed, Sep 14, 2016 at 7:38 AM, George Dunlap  wrote:
> On 13/09/16 19:12, Tamas K Lengyel wrote:
>> Setting response flags in vm_event are only ever safe if the vCPUs are 
>> paused.
>> To reflect this we move all checks within the if block that already checks
>> whether this is the case. Checks that are only supported on one architecture
>> we relocate the bitmask operations to the arch-specific handlers to avoid
>> the overhead on architectures that don't support it.
>>
>> Furthermore, we clean-up the emulation checks so it more clearly represents 
>> the
>> decision-logic when emulation should take place. As part of this we also
>> set the stage to allow emulation in response to other types of events, not 
>> just
>> mem_access violations.
>>
>> Signed-off-by: Tamas K Lengyel 
>
> Tamas,
>
> Would you like a detailed review from me for this?  I'm happy to ack the
> p2m bits on the basis that they're only touching mem_access code.  A
> full review may get stuck behind a pretty long review backlog. :-(
>
>  -George

Hi George,
acking just the p2m bits should suffice!

Thanks,
Tamas

>> ---
>> Cc: George Dunlap 
>> Cc: Jan Beulich 
>> Cc: Andrew Cooper 
>> Cc: Razvan Cojocaru 
>> Cc: Stefano Stabellini 
>> Cc: Julien Grall 
>> ---
>>  xen/arch/x86/mm/p2m.c  | 81 
>> +++---
>>  xen/arch/x86/vm_event.c| 35 +-
>>  xen/common/vm_event.c  | 53 ++-
>>  xen/include/asm-arm/p2m.h  |  4 +--
>>  xen/include/asm-arm/vm_event.h |  9 -
>>  xen/include/asm-x86/p2m.h  |  4 +--
>>  xen/include/asm-x86/vm_event.h |  5 ++-
>>  xen/include/xen/mem_access.h   | 12 ---
>>  8 files changed, 113 insertions(+), 90 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 7d14c3b..6c01868 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1588,62 +1588,55 @@ void p2m_mem_paging_resume(struct domain *d, 
>> vm_event_response_t *rsp)
>>  }
>>  }
>>
>> -void p2m_mem_access_emulate_check(struct vcpu *v,
>> -  const vm_event_response_t *rsp)
>> +bool_t p2m_mem_access_emulate_check(struct vcpu *v,
>> +const vm_event_response_t *rsp)
>>  {
>> -/* Mark vcpu for skipping one instruction upon rescheduling. */
>> -if ( rsp->flags & VM_EVENT_FLAG_EMULATE )
>> -{
>> -xenmem_access_t access;
>> -bool_t violation = 1;
>> -const struct vm_event_mem_access *data = &rsp->u.mem_access;
>> +xenmem_access_t access;
>> +bool_t violation = 1;
>> +const struct vm_event_mem_access *data = &rsp->u.mem_access;
>>
>> -if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
>> +if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
>> +{
>> +switch ( access )
>>  {
>> -switch ( access )
>> -{
>> -case XENMEM_access_n:
>> -case XENMEM_access_n2rwx:
>> -default:
>> -violation = data->flags & MEM_ACCESS_RWX;
>> -break;
>> +case XENMEM_access_n:
>> +case XENMEM_access_n2rwx:
>> +default:
>> +violation = data->flags & MEM_ACCESS_RWX;
>> +break;
>>
>> -case XENMEM_access_r:
>> -violation = data->flags & MEM_ACCESS_WX;
>> -break;
>> +case XENMEM_access_r:
>> +violation = data->flags & MEM_ACCESS_WX;
>> +break;
>>
>> -case XENMEM_access_w:
>> -violation = data->flags & MEM_ACCESS_RX;
>> -break;
>> +case XENMEM_access_w:
>> +violation = data->flags & MEM_ACCESS_RX;
>> +break;
>>
>> -case XENMEM_access_x:
>> -violation = data->flags & MEM_ACCESS_RW;
>> -break;
>> +case XENMEM_access_x:
>> +violation = data->flags & MEM_ACCESS_RW;
>> +break;
>>
>> -case XENMEM_access_rx:
>> -case XENMEM_access_rx2rw:
>> -violation = data->flags & MEM_ACCESS_W;
>> -break;
>> +case XENMEM_access_rx:
>> +case XENMEM_access_rx2rw:
>> +violation = data->flags & MEM_ACCESS_W;
>> +break;
>>
>> -case XENMEM_access_wx:
>> -violation = data->flags & MEM_ACCESS_R;
>> -break;
>> +case XENMEM_access_wx:
>> +violation = data->flags & MEM_ACCESS_R;
>> +break;
>>
>> -case XENMEM_access_rw:
>> -violation = data->flags & MEM_ACCESS_X;
>> -break;
>> +case XENMEM_access_rw:
>> +violation = data->flags & MEM_ACCESS_X;
>> +break;
>>
>> -case XENMEM_access_rwx:
>> -violation = 0;
>> -break;
>> -}
>> +

Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-14 Thread George Dunlap 
On 13/09/16 19:12, Tamas K Lengyel wrote:
> Setting response flags in vm_event are only ever safe if the vCPUs are paused.
> To reflect this we move all checks within the if block that already checks
> whether this is the case. Checks that are only supported on one architecture
> we relocate the bitmask operations to the arch-specific handlers to avoid
> the overhead on architectures that don't support it.
> 
> Furthermore, we clean-up the emulation checks so it more clearly represents 
> the
> decision-logic when emulation should take place. As part of this we also
> set the stage to allow emulation in response to other types of events, not 
> just
> mem_access violations.
> 
> Signed-off-by: Tamas K Lengyel 

Tamas,

Would you like a detailed review from me for this?  I'm happy to ack the
p2m bits on the basis that they're only touching mem_access code.  A
full review may get stuck behind a pretty long review backlog. :-(

 -George

> ---
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Razvan Cojocaru 
> Cc: Stefano Stabellini 
> Cc: Julien Grall 
> ---
>  xen/arch/x86/mm/p2m.c  | 81 
> +++---
>  xen/arch/x86/vm_event.c| 35 +-
>  xen/common/vm_event.c  | 53 ++-
>  xen/include/asm-arm/p2m.h  |  4 +--
>  xen/include/asm-arm/vm_event.h |  9 -
>  xen/include/asm-x86/p2m.h  |  4 +--
>  xen/include/asm-x86/vm_event.h |  5 ++-
>  xen/include/xen/mem_access.h   | 12 ---
>  8 files changed, 113 insertions(+), 90 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 7d14c3b..6c01868 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1588,62 +1588,55 @@ void p2m_mem_paging_resume(struct domain *d, 
> vm_event_response_t *rsp)
>  }
>  }
>  
> -void p2m_mem_access_emulate_check(struct vcpu *v,
> -  const vm_event_response_t *rsp)
> +bool_t p2m_mem_access_emulate_check(struct vcpu *v,
> +const vm_event_response_t *rsp)
>  {
> -/* Mark vcpu for skipping one instruction upon rescheduling. */
> -if ( rsp->flags & VM_EVENT_FLAG_EMULATE )
> -{
> -xenmem_access_t access;
> -bool_t violation = 1;
> -const struct vm_event_mem_access *data = &rsp->u.mem_access;
> +xenmem_access_t access;
> +bool_t violation = 1;
> +const struct vm_event_mem_access *data = &rsp->u.mem_access;
>  
> -if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +{
> +switch ( access )
>  {
> -switch ( access )
> -{
> -case XENMEM_access_n:
> -case XENMEM_access_n2rwx:
> -default:
> -violation = data->flags & MEM_ACCESS_RWX;
> -break;
> +case XENMEM_access_n:
> +case XENMEM_access_n2rwx:
> +default:
> +violation = data->flags & MEM_ACCESS_RWX;
> +break;
>  
> -case XENMEM_access_r:
> -violation = data->flags & MEM_ACCESS_WX;
> -break;
> +case XENMEM_access_r:
> +violation = data->flags & MEM_ACCESS_WX;
> +break;
>  
> -case XENMEM_access_w:
> -violation = data->flags & MEM_ACCESS_RX;
> -break;
> +case XENMEM_access_w:
> +violation = data->flags & MEM_ACCESS_RX;
> +break;
>  
> -case XENMEM_access_x:
> -violation = data->flags & MEM_ACCESS_RW;
> -break;
> +case XENMEM_access_x:
> +violation = data->flags & MEM_ACCESS_RW;
> +break;
>  
> -case XENMEM_access_rx:
> -case XENMEM_access_rx2rw:
> -violation = data->flags & MEM_ACCESS_W;
> -break;
> +case XENMEM_access_rx:
> +case XENMEM_access_rx2rw:
> +violation = data->flags & MEM_ACCESS_W;
> +break;
>  
> -case XENMEM_access_wx:
> -violation = data->flags & MEM_ACCESS_R;
> -break;
> +case XENMEM_access_wx:
> +violation = data->flags & MEM_ACCESS_R;
> +break;
>  
> -case XENMEM_access_rw:
> -violation = data->flags & MEM_ACCESS_X;
> -break;
> +case XENMEM_access_rw:
> +violation = data->flags & MEM_ACCESS_X;
> +break;
>  
> -case XENMEM_access_rwx:
> -violation = 0;
> -break;
> -}
> +case XENMEM_access_rwx:
> +violation = 0;
> +break;
>  }
> -
> -v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
> -
> -if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
> -

Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-14 Thread Julien Grall 

Hello Tamas,

On 13/09/16 19:12, Tamas K Lengyel wrote:

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 53c4d78..5e9bc54 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -121,10 +121,10 @@ typedef enum {
  p2m_to_mask(p2m_map_foreign)))

 static inline
-void p2m_mem_access_emulate_check(struct vcpu *v,
+bool_t p2m_mem_access_emulate_check(struct vcpu *v,
   const vm_event_response_t *rsp)


s/bool_t/bool/ and please indent properly the second line.

Please indent properly the second line.

Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling

2016-09-14 Thread Razvan Cojocaru 
On 09/13/2016 09:12 PM, Tamas K Lengyel wrote:
> Setting response flags in vm_event are only ever safe if the vCPUs are paused.
> To reflect this we move all checks within the if block that already checks
> whether this is the case. Checks that are only supported on one architecture
> we relocate the bitmask operations to the arch-specific handlers to avoid
> the overhead on architectures that don't support it.
> 
> Furthermore, we clean-up the emulation checks so it more clearly represents 
> the
> decision-logic when emulation should take place. As part of this we also
> set the stage to allow emulation in response to other types of events, not 
> just
> mem_access violations.
> 
> Signed-off-by: Tamas K Lengyel 
> ---
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> Cc: Razvan Cojocaru 
> Cc: Stefano Stabellini 
> Cc: Julien Grall 
> ---
>  xen/arch/x86/mm/p2m.c  | 81 
> +++---
>  xen/arch/x86/vm_event.c| 35 +-
>  xen/common/vm_event.c  | 53 ++-
>  xen/include/asm-arm/p2m.h  |  4 +--
>  xen/include/asm-arm/vm_event.h |  9 -
>  xen/include/asm-x86/p2m.h  |  4 +--
>  xen/include/asm-x86/vm_event.h |  5 ++-
>  xen/include/xen/mem_access.h   | 12 ---
>  8 files changed, 113 insertions(+), 90 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 7d14c3b..6c01868 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1588,62 +1588,55 @@ void p2m_mem_paging_resume(struct domain *d, 
> vm_event_response_t *rsp)
>  }
>  }
>  
> -void p2m_mem_access_emulate_check(struct vcpu *v,
> -  const vm_event_response_t *rsp)
> +bool_t p2m_mem_access_emulate_check(struct vcpu *v,

Judging by the reviews for my pending patch, I believe you'll be asked
to switch to bool / true / false here.

> +const vm_event_response_t *rsp)
>  {
> -/* Mark vcpu for skipping one instruction upon rescheduling. */
> -if ( rsp->flags & VM_EVENT_FLAG_EMULATE )
> -{
> -xenmem_access_t access;
> -bool_t violation = 1;
> -const struct vm_event_mem_access *data = &rsp->u.mem_access;
> +xenmem_access_t access;
> +bool_t violation = 1;
> +const struct vm_event_mem_access *data = &rsp->u.mem_access;
>  
> -if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
> +{
> +switch ( access )
>  {
> -switch ( access )
> -{
> -case XENMEM_access_n:
> -case XENMEM_access_n2rwx:
> -default:
> -violation = data->flags & MEM_ACCESS_RWX;
> -break;
> +case XENMEM_access_n:
> +case XENMEM_access_n2rwx:
> +default:
> +violation = data->flags & MEM_ACCESS_RWX;
> +break;
>  
> -case XENMEM_access_r:
> -violation = data->flags & MEM_ACCESS_WX;
> -break;
> +case XENMEM_access_r:
> +violation = data->flags & MEM_ACCESS_WX;
> +break;
>  
> -case XENMEM_access_w:
> -violation = data->flags & MEM_ACCESS_RX;
> -break;
> +case XENMEM_access_w:
> +violation = data->flags & MEM_ACCESS_RX;
> +break;
>  
> -case XENMEM_access_x:
> -violation = data->flags & MEM_ACCESS_RW;
> -break;
> +case XENMEM_access_x:
> +violation = data->flags & MEM_ACCESS_RW;
> +break;
>  
> -case XENMEM_access_rx:
> -case XENMEM_access_rx2rw:
> -violation = data->flags & MEM_ACCESS_W;
> -break;
> +case XENMEM_access_rx:
> +case XENMEM_access_rx2rw:
> +violation = data->flags & MEM_ACCESS_W;
> +break;
>  
> -case XENMEM_access_wx:
> -violation = data->flags & MEM_ACCESS_R;
> -break;
> +case XENMEM_access_wx:
> +violation = data->flags & MEM_ACCESS_R;
> +break;
>  
> -case XENMEM_access_rw:
> -violation = data->flags & MEM_ACCESS_X;
> -break;
> +case XENMEM_access_rw:
> +violation = data->flags & MEM_ACCESS_X;
> +break;
>  
> -case XENMEM_access_rwx:
> -violation = 0;
> -break;
> -}
> +case XENMEM_access_rwx:
> +violation = 0;
> +break;
>  }
> -
> -v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
> -
> -if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
> -v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>  }
> +
> +return violation;
>  }
>