|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] vm_event: Sanitize vm_event response handling
On Wed, Sep 14, 2016 at 7:38 AM, George Dunlap <george.dunlap@xxxxxxxxxx> 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.lengyel@xxxxxxxxxxxx>
>
> 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 <george.dunlap@xxxxxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> 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) )
>> - v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>> }
>> +
>> + return violation;
>> }
>>
>> void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> index e938ca3..343b9c8 100644
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -18,6 +18,7 @@
>> * License along with this program; If not, see
>> <http://www.gnu.org/licenses/>.
>> */
>>
>> +#include <asm/p2m.h>
>> #include <asm/vm_event.h>
>>
>> /* Implicitly serialized by the domctl lock. */
>> @@ -56,8 +57,12 @@ void vm_event_cleanup_domain(struct domain *d)
>> d->arch.mem_access_emulate_each_rep = 0;
>> }
>>
>> -void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>> +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
>> + vm_event_response_t *rsp)
>> {
>> + if ( !(rsp->flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP) )
>> + return;
>> +
>> if ( !is_hvm_domain(d) )
>> return;
>>
>> @@ -186,6 +191,34 @@ void vm_event_fill_regs(vm_event_request_t *req)
>> req->data.regs.x86.cs_arbytes = seg.attr.bytes;
>> }
>>
>> +void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>> +{
>> + if ( !(rsp->flags & VM_EVENT_FLAG_EMULATE) )
>> + {
>> + v->arch.vm_event->emulate_flags = 0;
>> + return;
>> + }
>> +
>> + switch ( rsp->reason )
>> + {
>> + case VM_EVENT_REASON_MEM_ACCESS:
>> + /*
>> + * Emulate iff this is a response to a mem_access violation and
>> there
>> + * are still conflicting mem_access permissions in-place.
>> + */
>> + if ( p2m_mem_access_emulate_check(v, rsp) )
>> + {
>> + if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
>> + v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>> +
>> + v->arch.vm_event->emulate_flags = rsp->flags;
>> + }
>> + break;
>> + default:
>> + break;
>> + };
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
>> index 8398af7..907ab40 100644
>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -398,42 +398,41 @@ void vm_event_resume(struct domain *d, struct
>> vm_event_domain *ved)
>> * In some cases the response type needs extra handling, so here
>> * we call the appropriate handlers.
>> */
>> - switch ( rsp.reason )
>> - {
>> -#ifdef CONFIG_X86
>> - case VM_EVENT_REASON_MOV_TO_MSR:
>> -#endif
>> - case VM_EVENT_REASON_WRITE_CTRLREG:
>> - vm_event_register_write_resume(v, &rsp);
>> - break;
>> -
>> -#ifdef CONFIG_HAS_MEM_ACCESS
>> - case VM_EVENT_REASON_MEM_ACCESS:
>> - mem_access_resume(v, &rsp);
>> - break;
>> -#endif
>>
>> + /* Check flags which apply only when the vCPU is paused */
>> + if ( atomic_read(&v->vm_event_pause_count) )
>> + {
>> #ifdef CONFIG_HAS_MEM_PAGING
>> - case VM_EVENT_REASON_MEM_PAGING:
>> - p2m_mem_paging_resume(d, &rsp);
>> - break;
>> + if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
>> + p2m_mem_paging_resume(d, &rsp);
>> #endif
>>
>> - };
>> + /*
>> + * Check emulation flags in the arch-specific handler only, as
>> it
>> + * has to set arch-specific flags when supported, and to avoid
>> + * bitmask overhead when it isn't supported.
>> + */
>> + vm_event_emulate_check(v, &rsp);
>> +
>> + /*
>> + * Check in arch-specific handler to avoid bitmask overhead when
>> + * not supported.
>> + */
>> + vm_event_register_write_resume(v, &rsp);
>>
>> - /* Check for altp2m switch */
>> - if ( rsp.flags & VM_EVENT_FLAG_ALTERNATE_P2M )
>> - p2m_altp2m_check(v, rsp.altp2m_idx);
>> + /*
>> + * Check in arch-specific handler to avoid bitmask overhead when
>> + * not supported.
>> + */
>> + vm_event_toggle_singlestep(d, v, &rsp);
>> +
>> + /* Check for altp2m switch */
>> + if ( rsp.flags & VM_EVENT_FLAG_ALTERNATE_P2M )
>> + p2m_altp2m_check(v, rsp.altp2m_idx);
>>
>> - /* Check flags which apply only when the vCPU is paused */
>> - if ( atomic_read(&v->vm_event_pause_count) )
>> - {
>> if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>> vm_event_set_registers(v, &rsp);
>>
>> - if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>> - vm_event_toggle_singlestep(d, v);
>> -
>> if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>> vm_event_vcpu_unpause(v);
>> }
>> 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)
>> {
>> - /* Not supported on ARM. */
>> + return false;
>> }
>>
>> static inline
>> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
>> index 9482636..66f2474 100644
>> --- a/xen/include/asm-arm/vm_event.h
>> +++ b/xen/include/asm-arm/vm_event.h
>> @@ -34,7 +34,8 @@ static inline void vm_event_cleanup_domain(struct domain
>> *d)
>> memset(&d->monitor, 0, sizeof(d->monitor));
>> }
>>
>> -static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu
>> *v)
>> +static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu
>> *v,
>> + vm_event_response_t *rsp)
>> {
>> /* Not supported on ARM. */
>> }
>> @@ -45,4 +46,10 @@ void vm_event_register_write_resume(struct vcpu *v,
>> vm_event_response_t *rsp)
>> /* Not supported on ARM. */
>> }
>>
>> +static inline
>> +void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>> +{
>> + /* Not supported on ARM. */
>> +}
>> +
>> #endif /* __ASM_ARM_VM_EVENT_H__ */
>> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
>> index 9fc9ead..1897def 100644
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -677,8 +677,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long
>> gla,
>>
>> /* Check for emulation and mark vcpu for skipping one instruction
>> * upon rescheduling if required. */
>> -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);
>>
>> /* Sanity check for mem_access hardware support */
>> static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
>> diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
>> index 294def6..ebb5d88 100644
>> --- a/xen/include/asm-x86/vm_event.h
>> +++ b/xen/include/asm-x86/vm_event.h
>> @@ -35,8 +35,11 @@ int vm_event_init_domain(struct domain *d);
>>
>> void vm_event_cleanup_domain(struct domain *d);
>>
>> -void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
>> +void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v,
>> + vm_event_response_t *rsp);
>>
>> void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t
>> *rsp);
>>
>> +void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp);
>> +
>> #endif /* __ASM_X86_VM_EVENT_H__ */
>> diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
>> index 3d054e0..da36e07 100644
>> --- a/xen/include/xen/mem_access.h
>> +++ b/xen/include/xen/mem_access.h
>> @@ -30,12 +30,6 @@
>> int mem_access_memop(unsigned long cmd,
>> XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
>>
>> -static inline
>> -void mem_access_resume(struct vcpu *v, vm_event_response_t *rsp)
>> -{
>> - p2m_mem_access_emulate_check(v, rsp);
>> -}
>> -
>> #else
>>
>> static inline
>> @@ -45,12 +39,6 @@ int mem_access_memop(unsigned long cmd,
>> return -ENOSYS;
>> }
>>
>> -static inline
>> -void mem_access_resume(struct vcpu *vcpu, vm_event_response_t *rsp)
>> -{
>> - /* Nothing to do. */
>> -}
>> -
>> #endif /* HAS_MEM_ACCESS */
>>
>> #endif /* _XEN_ASM_MEM_ACCESS_H */
>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |