Re: [Xen-devel] [PATCH V8 1/4] x86/mm: Add array_index_nospec to guest provided index values

2020-01-23 Thread Tamas K Lengyel 
On Thu, Jan 23, 2020 at 11:45 AM Andrew Cooper
 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 
> > Signed-off-by: Alexandru Isaila 
> > Acked-by: Tamas K Lengyel 
>
> 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

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

Re: [Xen-devel] [PATCH V8 1/4] x86/mm: Add array_index_nospec to guest provided index values

2020-01-23 Thread Andrew Cooper 
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 
> Signed-off-by: Alexandru Isaila 
> Acked-by: Tamas K Lengyel 

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

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

Re: [Xen-devel] [PATCH V8 1/4] x86/mm: Add array_index_nospec to guest provided index values

2020-01-23 Thread George Dunlap 
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 
> Signed-off-by: Alexandru Isaila 
> Acked-by: Tamas K Lengyel 

Acked-by: George Dunlap 

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

Re: [Xen-devel] [PATCH V8 1/4] x86/mm: Add array_index_nospec to guest provided index values

2020-01-21 Thread Petre Ovidiu PIRCALABU 
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 
> Signed-off-by: Alexandru Isaila 
> Acked-by: Tamas K Lengyel 
> 
Reviewed-by: Petre Pircalabu 

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

Re: [Xen-devel] [PATCH V8 1/4] x86/mm: Add array_index_nospec to guest provided index values

2020-01-17 Thread Jan Beulich 
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 
> Signed-off-by: Alexandru Isaila 
> Acked-by: Tamas K Lengyel 

Reviewed-by: Jan Beulich 

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

[Xen-devel] [PATCH V8 1/4] x86/mm: Add array_index_nospec to guest provided index values

2020-01-17 Thread Alexandru Stefan ISAILA 
This patch aims to sanitize indexes, potentially guest provided
values, for altp2m_eptp[] and altp2m_p2m[] arrays.

Requested-by: Jan Beulich 
Signed-off-by: Alexandru Isaila 
Acked-by: Tamas K Lengyel 
---
CC: Razvan Cojocaru 
CC: Tamas K Lengyel 
CC: Petre Pircalabu 
CC: George Dunlap 
CC: Jan Beulich 
CC: Andrew Cooper 
CC: Wei Liu 
CC: "Roger Pau Monné" 
CC: Jun Nakajima 
CC: Kevin Tian 
---
Changes since V7:
- Make use of array_access_nospec() over
array_index_nospec(altp2m_idx, ARRAY_SIZE(d->arch.altp2m_p2m).
---
 xen/arch/x86/mm/mem_access.c | 21 ++-
 xen/arch/x86/mm/p2m-ept.c|  4 ++--
 xen/arch/x86/mm/p2m.c| 39 +---
 3 files changed, 37 insertions(+), 27 deletions(-)

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 = >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