| Message ID | 20200117133059.14602-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V8,1/4] x86/mm: Add array_index_nospec to guest provided index values | expand |
On 17.01.2020 14:31, Alexandru Stefan ISAILA wrote: > This patch aims to sanitize indexes, potentially guest provided > values, for altp2m_eptp[] and altp2m_p2m[] arrays. > > Requested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On Fri, 2020-01-17 at 15:31 +0200, Alexandru Stefan ISAILA wrote: > This patch aims to sanitize indexes, potentially guest provided > values, for altp2m_eptp[] and altp2m_p2m[] arrays. > > Requested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > Reviewed-by: Petre Pircalabu <ppircalabu@bitdefender.com>
On 1/17/20 1:31 PM, Alexandru Stefan ISAILA wrote: > This patch aims to sanitize indexes, potentially guest provided > values, for altp2m_eptp[] and altp2m_p2m[] arrays. > > Requested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> Acked-by: George Dunlap <george.dunlap@citrix.com>
On 17/01/2020 13:31, Alexandru Stefan ISAILA wrote: > This patch aims to sanitize indexes, potentially guest provided > values, for altp2m_eptp[] and altp2m_p2m[] arrays. > > Requested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> Something in this series broke the ARM build. Sorry, but I don't have any further time to investigate. gcc -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ -include /builds/xen-project/xen/xen/include/xen/config.h '-D__OBJECT_FILE__="asm-offsets.s"' -Wa,--strip-local-absolute -g -MMD -MF ./.asm-offsets.s.d -mcpu=generic -mgeneral-regs-only -I/builds/xen-project/xen/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -S -o asm-offsets.s arm64/asm-offsets.c In file included from /builds/xen-project/xen/xen/include/asm/p2m.h:7, from /builds/xen-project/xen/xen/include/asm/domain.h:7, from /builds/xen-project/xen/xen/include/xen/domain.h:8, from /builds/xen-project/xen/xen/include/xen/sched.h:11, from arm64/asm-offsets.c:9: /builds/xen-project/xen/xen/include/xen/mem_access.h:61:47: error: 'struct p2m_domain' declared inside parameter list will not be visible outside of this definition or declaration [-Werror] bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m, ^~~~~~~~~~ /builds/xen-project/xen/xen/include/xen/mem_access.h:83:38: error: 'struct xen_hvm_altp2m_suppress_ve_multi' declared inside parameter list will not be visible outside of this definition or declaration [-Werror] struct xen_hvm_altp2m_suppress_ve_multi *suppress_ve); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[3]: *** [Makefile:124: asm-offsets.s] Error 1 make[3]: Leaving directory '/builds/xen-project/xen/xen/arch/arm' make[2]: *** [Makefile:146: /builds/xen-project/xen/xen/xen] Error 2 make[2]: Leaving directory '/builds/xen-project/xen/xen' make[1]: *** [Makefile:50: install] Error 2 make[1]: Leaving directory '/builds/xen-project/xen/xen' make: *** [Makefile:130: install-xen] Error 2 make: *** Waiting for unfinished jobs.... Full logs: https://gitlab.com/xen-project/xen/-/jobs/412893448 ~Andrew
On Thu, Jan 23, 2020 at 11:45 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 17/01/2020 13:31, Alexandru Stefan ISAILA wrote: > > This patch aims to sanitize indexes, potentially guest provided > > values, for altp2m_eptp[] and altp2m_p2m[] arrays. > > > > Requested-by: Jan Beulich <jbeulich@suse.com> > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > > Something in this series broke the ARM build. Sorry, but I don't have > any further time to investigate. > > gcc -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes > -Wdeclaration-after-statement -Wno-unused-but-set-variable > -Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc > -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith > -Wvla -pipe -D__XEN__ -include > /builds/xen-project/xen/xen/include/xen/config.h > '-D__OBJECT_FILE__="asm-offsets.s"' -Wa,--strip-local-absolute -g -MMD > -MF ./.asm-offsets.s.d -mcpu=generic -mgeneral-regs-only > -I/builds/xen-project/xen/xen/include -fno-stack-protector > -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -S -o > asm-offsets.s arm64/asm-offsets.c > In file included from /builds/xen-project/xen/xen/include/asm/p2m.h:7, > from /builds/xen-project/xen/xen/include/asm/domain.h:7, > from /builds/xen-project/xen/xen/include/xen/domain.h:8, > from /builds/xen-project/xen/xen/include/xen/sched.h:11, > from arm64/asm-offsets.c:9: > /builds/xen-project/xen/xen/include/xen/mem_access.h:61:47: error: > 'struct p2m_domain' declared inside parameter list will not be visible > outside of this definition or declaration [-Werror] > bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m, > ^~~~~~~~~~ > /builds/xen-project/xen/xen/include/xen/mem_access.h:83:38: error: > 'struct xen_hvm_altp2m_suppress_ve_multi' declared inside parameter list Looks like we need an explicit include for asm/p2m.h and public/hvm/hvm_op.h in the mem_access.h header (both of which end up being included prior to mem_access.h on an x86 build). Although from the looks of it wrapping the _ve functions in #ifdef CONFIG_X86 .. #endif would be even better. Tamas
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 320b9fe621..31ff826393 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -366,11 +366,12 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, #ifdef CONFIG_HVM if ( altp2m_idx ) { - if ( altp2m_idx >= MAX_ALTP2M || - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == + mfn_x(INVALID_MFN) ) return -EINVAL; - ap2m = d->arch.altp2m_p2m[altp2m_idx]; + ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx); } #else ASSERT(!altp2m_idx); @@ -425,11 +426,12 @@ long p2m_set_mem_access_multi(struct domain *d, #ifdef CONFIG_HVM if ( altp2m_idx ) { - if ( altp2m_idx >= MAX_ALTP2M || - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == + mfn_x(INVALID_MFN) ) return -EINVAL; - ap2m = d->arch.altp2m_p2m[altp2m_idx]; + ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx); } #else ASSERT(!altp2m_idx); @@ -491,11 +493,12 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access, } else if ( altp2m_idx ) /* altp2m view 0 is treated as the hostp2m */ { - if ( altp2m_idx >= MAX_ALTP2M || - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == + mfn_x(INVALID_MFN) ) return -EINVAL; - p2m = d->arch.altp2m_p2m[altp2m_idx]; + p2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx); } #else ASSERT(!altp2m_idx); diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index b5517769c9..b078a9a59e 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1353,7 +1353,7 @@ void setup_ept_dump(void) void p2m_init_altp2m_ept(struct domain *d, unsigned int i) { - struct p2m_domain *p2m = d->arch.altp2m_p2m[i]; + struct p2m_domain *p2m = array_access_nospec(d->arch.altp2m_p2m, i); struct p2m_domain *hostp2m = p2m_get_hostp2m(d); struct ept_data *ept; @@ -1366,7 +1366,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i) p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0; ept = &p2m->ept; ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m)); - d->arch.altp2m_eptp[i] = ept->eptp; + d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp; } unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp) diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 3119269073..00b24342fc 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2502,7 +2502,7 @@ static void p2m_reset_altp2m(struct domain *d, unsigned int idx, struct p2m_domain *p2m; ASSERT(idx < MAX_ALTP2M); - p2m = d->arch.altp2m_p2m[idx]; + p2m = array_access_nospec(d->arch.altp2m_p2m, idx); p2m_lock(p2m); @@ -2543,7 +2543,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) ASSERT(idx < MAX_ALTP2M); - p2m = d->arch.altp2m_p2m[idx]; + p2m = array_access_nospec(d->arch.altp2m_p2m, idx); hostp2m = p2m_get_hostp2m(d); p2m_lock(p2m); @@ -2574,12 +2574,13 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) { int rc = -EINVAL; - if ( idx >= MAX_ALTP2M ) + if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ) return rc; altp2m_list_lock(d); - if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) + if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] == + mfn_x(INVALID_MFN) ) rc = p2m_activate_altp2m(d, idx); altp2m_list_unlock(d); @@ -2615,7 +2616,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) struct p2m_domain *p2m; int rc = -EBUSY; - if ( !idx || idx >= MAX_ALTP2M ) + if ( !idx || idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ) return rc; rc = domain_pause_except_self(d); @@ -2625,14 +2626,16 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) rc = -EBUSY; altp2m_list_lock(d); - if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) ) + if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] != + mfn_x(INVALID_MFN) ) { - p2m = d->arch.altp2m_p2m[idx]; + p2m = array_access_nospec(d->arch.altp2m_p2m, idx); if ( !_atomic_read(p2m->active_vcpus) ) { p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE); - d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN); + d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] = + mfn_x(INVALID_MFN); rc = 0; } } @@ -2689,11 +2692,13 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, mfn_t mfn; int rc = -EINVAL; - if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) ) + if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] == + mfn_x(INVALID_MFN) ) return rc; hp2m = p2m_get_hostp2m(d); - ap2m = d->arch.altp2m_p2m[idx]; + ap2m = array_access_nospec(d->arch.altp2m_p2m, idx); p2m_lock(hp2m); p2m_lock(ap2m); @@ -3032,11 +3037,12 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, if ( altp2m_idx > 0 ) { - if ( altp2m_idx >= MAX_ALTP2M || - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == + mfn_x(INVALID_MFN) ) return -EINVAL; - p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; + p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx); } else p2m = host_p2m; @@ -3075,11 +3081,12 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve, if ( altp2m_idx > 0 ) { - if ( altp2m_idx >= MAX_ALTP2M || - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == + mfn_x(INVALID_MFN) ) return -EINVAL; - p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; + p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx); } else p2m = host_p2m;