| Message ID | 20191217151144.9781-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V4,1/4] x86/mm: Add array_index_nospec to guest provided index values | expand |
On Tue, Dec 17, 2019 at 8:12 AM Alexandru Stefan ISAILA <aisaila@bitdefender.com> wrote: > > This patch aims to sanitize indexes, potentially guest provided > values, for altp2m_eptp[] and altp2m_p2m[] arrays. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> LGTM, thanks! Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote: > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, > if ( altp2m_idx ) > { > if ( altp2m_idx >= MAX_ALTP2M || > - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) > + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == The bounds check is against MAX_ALTP2M. Both MAX_ values look to be independent, which means bounds check and value passed to the helper need to match up (not just here). > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -1353,7 +1353,8 @@ 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 = > + d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)]; > struct p2m_domain *hostp2m = p2m_get_hostp2m(d); > struct ept_data *ept; > > @@ -1366,7 +1367,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) > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2499,7 +2499,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; > > p2m_lock(p2m); > > @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) > > ASSERT(idx < MAX_ALTP2M); > > - p2m = d->arch.altp2m_p2m[idx]; > + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; All of the above have a more or less significant disconnect between the bounds check and the use as array index. I think it would be quite helpful if these could live close to one another, so one can (see further up) easily prove that both specified bounds actually match up. Jan
On 17.12.2019 18:50, Jan Beulich wrote: > On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote: >> --- a/xen/arch/x86/mm/mem_access.c >> +++ b/xen/arch/x86/mm/mem_access.c >> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, >> if ( altp2m_idx ) >> { >> if ( altp2m_idx >= MAX_ALTP2M || >> - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) >> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == > > The bounds check is against MAX_ALTP2M. Both MAX_ values look to be > independent, which means bounds check and value passed to the > helper need to match up (not just here). I will have both checks against MAX_ALTP2M. > >> --- a/xen/arch/x86/mm/p2m-ept.c >> +++ b/xen/arch/x86/mm/p2m-ept.c >> @@ -1353,7 +1353,8 @@ 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 = >> + d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)]; >> struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >> struct ept_data *ept; >> >> @@ -1366,7 +1367,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) >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -2499,7 +2499,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; >> >> p2m_lock(p2m); >> >> @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) >> >> ASSERT(idx < MAX_ALTP2M); >> >> - p2m = d->arch.altp2m_p2m[idx]; >> + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; > > All of the above have a more or less significant disconnect between > the bounds check and the use as array index. I think it would be > quite helpful if these could live close to one another, so one can > (see further up) easily prove that both specified bounds actually > match up. > Sure, I can move the array use closer together. Alex
On 18.12.2019 10:06, Alexandru Stefan ISAILA wrote: > > > On 17.12.2019 18:50, Jan Beulich wrote: >> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote: >>> --- a/xen/arch/x86/mm/mem_access.c >>> +++ b/xen/arch/x86/mm/mem_access.c >>> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, >>> if ( altp2m_idx ) >>> { >>> if ( altp2m_idx >= MAX_ALTP2M || >>> - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) >>> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == >> >> The bounds check is against MAX_ALTP2M. Both MAX_ values look to be >> independent, which means bounds check and value passed to the >> helper need to match up (not just here). > > I will have both checks against MAX_ALTP2M. > >> >>> --- a/xen/arch/x86/mm/p2m-ept.c >>> +++ b/xen/arch/x86/mm/p2m-ept.c >>> @@ -1353,7 +1353,8 @@ 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 = >>> + d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)]; >>> struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >>> struct ept_data *ept; >>> >>> @@ -1366,7 +1367,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) >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -2499,7 +2499,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; >>> >>> p2m_lock(p2m); >>> >>> @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) >>> >>> ASSERT(idx < MAX_ALTP2M); >>> >>> - p2m = d->arch.altp2m_p2m[idx]; >>> + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; >> >> All of the above have a more or less significant disconnect between >> the bounds check and the use as array index. I think it would be >> quite helpful if these could live close to one another, so one can >> (see further up) easily prove that both specified bounds actually >> match up. >> > > Sure, I can move the array use closer together. > Sorry to come back on this but I was looking in the code and I am not sure I follow where is the disconnect. If you are talking about p2m_init_altp2m_ept() the eptp code will move up in patch 3/4. Can you please clarify? Thanks, Alex
On 18.12.2019 09:06, Alexandru Stefan ISAILA wrote: > On 17.12.2019 18:50, Jan Beulich wrote: >> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote: >>> --- a/xen/arch/x86/mm/mem_access.c >>> +++ b/xen/arch/x86/mm/mem_access.c >>> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, >>> if ( altp2m_idx ) >>> { >>> if ( altp2m_idx >= MAX_ALTP2M || >>> - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) >>> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == >> >> The bounds check is against MAX_ALTP2M. Both MAX_ values look to be >> independent, which means bounds check and value passed to the >> helper need to match up (not just here). > > I will have both checks against MAX_ALTP2M. An alternative would be something along the lines of if ( altp2m_idx >= min(MAX_ALTP2M, MAX_EPTP) || Jan
On 18.12.2019 10:57, Alexandru Stefan ISAILA wrote: > On 18.12.2019 10:06, Alexandru Stefan ISAILA wrote: >> On 17.12.2019 18:50, Jan Beulich wrote: >>> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote: >>>> --- a/xen/arch/x86/mm/p2m-ept.c >>>> +++ b/xen/arch/x86/mm/p2m-ept.c >>>> @@ -1353,7 +1353,8 @@ 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 = >>>> + d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)]; >>>> struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >>>> struct ept_data *ept; >>>> >>>> @@ -1366,7 +1367,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) >>>> --- a/xen/arch/x86/mm/p2m.c >>>> +++ b/xen/arch/x86/mm/p2m.c >>>> @@ -2499,7 +2499,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; >>>> >>>> p2m_lock(p2m); >>>> >>>> @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) >>>> >>>> ASSERT(idx < MAX_ALTP2M); >>>> >>>> - p2m = d->arch.altp2m_p2m[idx]; >>>> + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; >>> >>> All of the above have a more or less significant disconnect between >>> the bounds check and the use as array index. I think it would be >>> quite helpful if these could live close to one another, so one can >>> (see further up) easily prove that both specified bounds actually >>> match up. >>> >> >> Sure, I can move the array use closer together. >> > > Sorry to come back on this but I was looking in the code and I am not > sure I follow where is the disconnect. If you are talking about > p2m_init_altp2m_ept() the eptp code will move up in patch 3/4. My remark was about all four hunks left in context (and then still possibly extending to other ones). Let's take the last one above: p2m_activate_altp2m() has two callers, one of which loops over altp2m-s (and hence doesn't need the guard). The other one is p2m_init_altp2m_by_id() which does the range check I'm talking about (ASSERT() doesn't count), and which therefore is the place to use array_index_nospec(). Once you look there you'll notice that the function also has an array access itself which you've left untouched. Jan
On 18.12.2019 12:06, Jan Beulich wrote: > On 18.12.2019 10:57, Alexandru Stefan ISAILA wrote: >> On 18.12.2019 10:06, Alexandru Stefan ISAILA wrote: >>> On 17.12.2019 18:50, Jan Beulich wrote: >>>> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote: >>>>> --- a/xen/arch/x86/mm/p2m-ept.c >>>>> +++ b/xen/arch/x86/mm/p2m-ept.c >>>>> @@ -1353,7 +1353,8 @@ 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 = >>>>> + d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)]; >>>>> struct p2m_domain *hostp2m = p2m_get_hostp2m(d); >>>>> struct ept_data *ept; >>>>> >>>>> @@ -1366,7 +1367,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) >>>>> --- a/xen/arch/x86/mm/p2m.c >>>>> +++ b/xen/arch/x86/mm/p2m.c >>>>> @@ -2499,7 +2499,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; >>>>> >>>>> p2m_lock(p2m); >>>>> >>>>> @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) >>>>> >>>>> ASSERT(idx < MAX_ALTP2M); >>>>> >>>>> - p2m = d->arch.altp2m_p2m[idx]; >>>>> + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; >>>> >>>> All of the above have a more or less significant disconnect between >>>> the bounds check and the use as array index. I think it would be >>>> quite helpful if these could live close to one another, so one can >>>> (see further up) easily prove that both specified bounds actually >>>> match up. >>>> >>> >>> Sure, I can move the array use closer together. >>> >> >> Sorry to come back on this but I was looking in the code and I am not >> sure I follow where is the disconnect. If you are talking about >> p2m_init_altp2m_ept() the eptp code will move up in patch 3/4. > > My remark was about all four hunks left in context (and then still > possibly extending to other ones). Let's take the last one above: > p2m_activate_altp2m() has two callers, one of which loops over > altp2m-s (and hence doesn't need the guard). The other one is > p2m_init_altp2m_by_id() which does the range check I'm talking > about (ASSERT() doesn't count), and which therefore is the place > to use array_index_nospec(). Once you look there you'll notice > that the function also has an array access itself which you've > left untouched. > So add a "idx = array_index_nospec(idx, MAX_ALTP2M)" in the callers where there is a need for this and drop checks in the lower functions. Alex
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 320b9fe621..70f3528bb1 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, if ( altp2m_idx ) { if ( altp2m_idx >= MAX_ALTP2M || - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + 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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)]; } #else ASSERT(!altp2m_idx); @@ -426,10 +427,11 @@ long p2m_set_mem_access_multi(struct domain *d, if ( altp2m_idx ) { if ( altp2m_idx >= MAX_ALTP2M || - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + 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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)]; } #else ASSERT(!altp2m_idx); @@ -492,10 +494,11 @@ 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) ) + 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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)]; } #else ASSERT(!altp2m_idx); diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index b5517769c9..e088a63f56 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1353,7 +1353,8 @@ 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 = + d->arch.altp2m_p2m[array_index_nospec(i, MAX_ALTP2M)]; struct p2m_domain *hostp2m = p2m_get_hostp2m(d); struct ept_data *ept; @@ -1366,7 +1367,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 ba126f790a..7e7f4f1a7c 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2499,7 +2499,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; p2m_lock(p2m); @@ -2540,7 +2540,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx) ASSERT(idx < MAX_ALTP2M); - p2m = d->arch.altp2m_p2m[idx]; + p2m = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; hostp2m = p2m_get_hostp2m(d); p2m_lock(p2m); @@ -2622,9 +2622,10 @@ 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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; if ( !_atomic_read(p2m->active_vcpus) ) { @@ -2686,11 +2687,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 >= MAX_ALTP2M || + 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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; p2m_lock(hp2m); p2m_lock(ap2m); @@ -3030,10 +3033,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) ) + 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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, + MAX_ALTP2M)]; } else p2m = host_p2m; @@ -3073,10 +3078,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) ) + 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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, + MAX_ALTP2M)]; } else p2m = host_p2m;
This patch aims to sanitize indexes, potentially guest provided values, for altp2m_eptp[] and altp2m_p2m[] arrays. Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> --- 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> 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: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> --- xen/arch/x86/mm/mem_access.c | 15 +++++++++------ xen/arch/x86/mm/p2m-ept.c | 5 +++-- xen/arch/x86/mm/p2m.c | 27 +++++++++++++++++---------- 3 files changed, 29 insertions(+), 18 deletions(-)