| Message ID | 20191223140409.32449-4-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V6,1/4] x86/mm: Add array_index_nospec to guest provided index values | expand |
On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote: > At this moment the default_access param from xc_altp2m_create_view is > not used. > > This patch assigns default_access to p2m->default_access at the time of > initializing a new altp2m view. That's certainly not what it looks like. It looks like you're changing it from... > @@ -2562,7 +2564,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) > goto out; > } > > - p2m->default_access = hostp2m->default_access; > + p2m->default_access = hvmmem_default_access; > p2m->domain = hostp2m->domain; > p2m->global_logdirty = hostp2m->global_logdirty; > p2m->min_remapped_gfn = gfn_x(INVALID_GFN); ...hostp2m->default_access to... > @@ -340,7 +340,10 @@ static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m, > *paccess = memaccess[xaccess]; > break; > case XENMEM_access_default: > - *paccess = p2m->default_access; > + if ( !p2m ) > + *paccess = p2m_access_rwx; > + else > + *paccess = p2m->default_access; > break; > default: > return false; ...p2m_access_rwx (by passing NULL in to this function in p2m_init_next_altp2m). Why don't you... > -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) > +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, > + xenmem_access_t hvmmem_default_access) > { > int rc = -EINVAL; > unsigned int i; > + p2m_access_t a; > + struct p2m_domain *p2m; > + > + if ( hvmmem_default_access > XENMEM_access_default || > + !xenmem_access_to_p2m_access(NULL, hvmmem_default_access, &a) ) > + return rc; > > altp2m_list_lock(d); > ...pass in hostp2m here? Also... > @@ -2606,7 +2616,8 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) > if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) ) > continue; > > - rc = p2m_activate_altp2m(d, i); > + p2m = d->arch.altp2m_p2m[i]; What's this about? -George
On 24.12.2019 10:48, George Dunlap wrote: > On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote: >> At this moment the default_access param from xc_altp2m_create_view is >> not used. >> >> This patch assigns default_access to p2m->default_access at the time of >> initializing a new altp2m view. > > That's certainly not what it looks like. It looks like you're changing > it from... > >> @@ -2562,7 +2564,7 @@ static int p2m_activate_altp2m(struct domain *d, > unsigned int idx) >> goto out; >> } >> >> - p2m->default_access = hostp2m->default_access; >> + p2m->default_access = hvmmem_default_access; >> p2m->domain = hostp2m->domain; >> p2m->global_logdirty = hostp2m->global_logdirty; >> p2m->min_remapped_gfn = gfn_x(INVALID_GFN); > > ...hostp2m->default_access to... > >> @@ -340,7 +340,10 @@ static bool xenmem_access_to_p2m_access(struct > p2m_domain *p2m, >> *paccess = memaccess[xaccess]; >> break; >> case XENMEM_access_default: >> - *paccess = p2m->default_access; >> + if ( !p2m ) >> + *paccess = p2m_access_rwx; >> + else >> + *paccess = p2m->default_access; >> break; >> default: >> return false; > > ...p2m_access_rwx (by passing NULL in to this function in > p2m_init_next_altp2m). > > Why don't you... > >> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) >> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, >> + xenmem_access_t hvmmem_default_access) >> { >> int rc = -EINVAL; >> unsigned int i; >> + p2m_access_t a; >> + struct p2m_domain *p2m; >> + >> + if ( hvmmem_default_access > XENMEM_access_default || >> + !xenmem_access_to_p2m_access(NULL, hvmmem_default_access, &a) ) >> + return rc; >> >> altp2m_list_lock(d); >> > > ...pass in hostp2m here? That sound better then the current version, thanks. > > Also... > >> @@ -2606,7 +2616,8 @@ int p2m_init_next_altp2m(struct domain *d, > uint16_t *idx) >> if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) ) >> continue; >> >> - rc = p2m_activate_altp2m(d, i); >> + p2m = d->arch.altp2m_p2m[i]; > > What's this about? > This is an artifact form v3 when xenmem_access_to_p2m_access() needed p2m. I will clean it. Alex
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 4db15768d4..678faa4b14 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4660,7 +4660,8 @@ static int do_altp2m_op( } case HVMOP_altp2m_create_p2m: - if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view)) ) + if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view, + a.u.view.hvmmem_default_access)) ) rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; break; diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index a95a50bcae..80de6c2c65 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m, return rc; } -static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m, - xenmem_access_t xaccess, - p2m_access_t *paccess) +bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m, + xenmem_access_t xaccess, + p2m_access_t *paccess) { static const p2m_access_t memaccess[] = { #define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac @@ -340,7 +340,10 @@ static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m, *paccess = memaccess[xaccess]; break; case XENMEM_access_default: - *paccess = p2m->default_access; + if ( !p2m ) + *paccess = p2m_access_rwx; + else + *paccess = p2m->default_access; break; default: return false; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 5b99d1eb97..926438ed64 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -25,6 +25,7 @@ #include <xen/guest_access.h> /* copy_from_guest() */ #include <xen/iommu.h> +#include <xen/mem_access.h> #include <xen/vm_event.h> #include <xen/event.h> #include <public/vm_event.h> @@ -2536,7 +2537,8 @@ void p2m_flush_altp2m(struct domain *d) altp2m_list_unlock(d); } -static int p2m_activate_altp2m(struct domain *d, unsigned int idx) +static int p2m_activate_altp2m(struct domain *d, unsigned int idx, + p2m_access_t hvmmem_default_access) { struct p2m_domain *hostp2m, *p2m; int rc; @@ -2562,7 +2564,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) goto out; } - p2m->default_access = hostp2m->default_access; + p2m->default_access = hvmmem_default_access; p2m->domain = hostp2m->domain; p2m->global_logdirty = hostp2m->global_logdirty; p2m->min_remapped_gfn = gfn_x(INVALID_GFN); @@ -2579,6 +2581,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) { int rc = -EINVAL; + struct p2m_domain *hostp2m = p2m_get_hostp2m(d); if ( idx >= MAX_ALTP2M ) return rc; @@ -2588,16 +2591,23 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) altp2m_list_lock(d); if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) - rc = p2m_activate_altp2m(d, idx); + rc = p2m_activate_altp2m(d, idx, hostp2m->default_access); altp2m_list_unlock(d); return rc; } -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, + xenmem_access_t hvmmem_default_access) { int rc = -EINVAL; unsigned int i; + p2m_access_t a; + struct p2m_domain *p2m; + + if ( hvmmem_default_access > XENMEM_access_default || + !xenmem_access_to_p2m_access(NULL, hvmmem_default_access, &a) ) + return rc; altp2m_list_lock(d); @@ -2606,7 +2616,8 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx) if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) ) continue; - rc = p2m_activate_altp2m(d, i); + p2m = d->arch.altp2m_p2m[i]; + rc = p2m_activate_altp2m(d, i, a); if ( !rc ) *idx = i; diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index 94285db1b4..ac2d2787f4 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -884,7 +884,8 @@ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l, int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx); /* Find an available alternate p2m and make it valid */ -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx); +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, + xenmem_access_t hvmmem_default_access); /* Make a specific alternate p2m invalid */ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx); diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 1f049cfa2e..bb98abec88 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -251,8 +251,6 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_vcpu_disable_notify_t); struct xen_hvm_altp2m_view { /* IN/OUT variable */ uint16_t view; - /* Create view only: default access type - * NOTE: currently ignored */ uint16_t hvmmem_default_access; /* xenmem_access_t */ }; typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t; diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h index 00e594a0ad..5d53fb8ce4 100644 --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -58,6 +58,10 @@ typedef enum { /* NOTE: Assumed to be only 4 bits right now on x86. */ } p2m_access_t; +bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m, + xenmem_access_t xaccess, + p2m_access_t *paccess); + /* * Set access type for a region of gfns. * If gfn == INVALID_GFN, sets the default access type.
At this moment the default_access param from xc_altp2m_create_view is not used. This patch assigns default_access to p2m->default_access at the time of initializing a new altp2m view. Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> --- CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Wei Liu <wl@xen.org> CC: "Roger Pau Monné" <roger.pau@citrix.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Julien Grall <julien@xen.org> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Razvan Cojocaru <rcojocaru@bitdefender.com> CC: Tamas K Lengyel <tamas@tklengyel.com> CC: Petre Pircalabu <ppircalabu@bitdefender.com> CC: George Dunlap <george.dunlap@eu.citrix.com> --- Changes since V4: - Add const struct p2m_domain *p2m to xenmem_access_to_p2m_access() - Pull xenmem_access_to_p2m_access() out of the locked area - Add a check for NULL p2m in xenmem_access_to_p2m_access(). --- xen/arch/x86/hvm/hvm.c | 3 ++- xen/arch/x86/mm/mem_access.c | 11 +++++++---- xen/arch/x86/mm/p2m.c | 21 ++++++++++++++++----- xen/include/asm-x86/p2m.h | 3 ++- xen/include/public/hvm/hvm_op.h | 2 -- xen/include/xen/mem_access.h | 4 ++++ 6 files changed, 31 insertions(+), 13 deletions(-)