diff mbox series

[V6,1/4] x86/mm: Add array_index_nospec to guest provided index values

Message ID 20191223140409.32449-1-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

Commit Message

Alexandru Stefan ISAILA Dec. 23, 2019, 2:04 p.m. UTC 
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>
---
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>
---
Changes since V5:
	- Add black lines
	- Check altp2m_idx against min(ARRAY_SIZE(d->arch.altp2m_p2m),
MAX_EPTP).
---
 xen/arch/x86/mm/mem_access.c | 21 ++++++++++++---------
 xen/arch/x86/mm/p2m.c        | 26 ++++++++++++++++++--------
 2 files changed, 30 insertions(+), 17 deletions(-)

Comments

Tamas K Lengyel Dec. 23, 2019, 4:38 p.m. UTC | #1 
On Mon, Dec 23, 2019 at 7:04 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.
>
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

For the mem_access bits:
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
George Dunlap Dec. 23, 2019, 6:08 p.m. UTC | #2 
On 12/23/19 2:04 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>
> ---
> 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>
> ---
> Changes since V5:
> 	- Add black lines
> 	- Check altp2m_idx against min(ARRAY_SIZE(d->arch.altp2m_p2m),
> MAX_EPTP).
> ---
>  xen/arch/x86/mm/mem_access.c | 21 ++++++++++++---------
>  xen/arch/x86/mm/p2m.c        | 26 ++++++++++++++++++--------
>  2 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 320b9fe621..a95a50bcae 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;

I realize Jan asked for something like this, and I'm sorry I didn't have
time to bring it up then, but this seems really silly.  If we're worried
about this, wouldn't it be better to have a BUILD_BUG_ON(MAX_ALTP2M >
MAX_EPTP)?

Also, this bit where we check the array value and then re-mask the index
later seems really redundant; both here, but especially...


> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 3119269073..4fc919a9c5 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2577,6 +2577,8 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>      if ( idx >= MAX_ALTP2M )
>          return rc;
>  
> +    idx = array_index_nospec(idx, MAX_ALTP2M);
> +

...here.  What about the attached series of patches (compile-tested only)?

 -George
Jan Beulich Dec. 27, 2019, 7:52 a.m. UTC | #3 
On 23.12.2019 19:08, George Dunlap wrote:
> On 12/23/19 2:04 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>
>> ---
>> 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>
>> ---
>> Changes since V5:
>> 	- Add black lines
>> 	- Check altp2m_idx against min(ARRAY_SIZE(d->arch.altp2m_p2m),
>> MAX_EPTP).
>> ---
>>  xen/arch/x86/mm/mem_access.c | 21 ++++++++++++---------
>>  xen/arch/x86/mm/p2m.c        | 26 ++++++++++++++++++--------
>>  2 files changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 320b9fe621..a95a50bcae 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;
> 
> I realize Jan asked for something like this, and I'm sorry I didn't have
> time to bring it up then, but this seems really silly.  If we're worried
> about this, wouldn't it be better to have a BUILD_BUG_ON(MAX_ALTP2M >
> MAX_EPTP)?

I wouldn't mind this BUILD_BUG_ON() approach as an alternative,
but imo one such instance would then need attaching to every
site.

> Also, this bit where we check the array value and then re-mask the index
> later seems really redundant;

But that's the idea behind the *_nospec() additions: They are to
guard against speculation, i.e. both the bounds check and the
masking of the index have their (distinct) reason.

Jan
Jan Beulich Dec. 27, 2019, 7:59 a.m. UTC | #4 
On 23.12.2019 19:08, George Dunlap wrote:
> What about the attached series of patches (compile-tested only)?

This ...

>+#define nospec_clip(index, size)                 \
>+    ({                                           \
>+        bool clipped = (index >= size);          \
>+        index = array_index_nospec(index, size); \
>+        clipped;                                 \
>+    })

... in particular may misguide people on its use: If the clipped
"index" gets stored in a register, all is going to be fine (afaict),
but if it ends up in memory, there's be new (mis-)speculation
opportunities. Some of the clipping done in the patches is already
not fully safe against this, but in some other cases (especially
once array_access_nospec() would be used where possible) would at
least make things as safe as they can be made without compiler aid.

(As an aside, the suggested macro, if we were to put it in, would
need proper parenthesization of the macro parameter uses.)

Jan
Jan Beulich Dec. 27, 2019, 8:01 a.m. UTC | #5 
(re-sending, as I still don't see the mail having appeared on the list)

On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
> Changes since V5:
> 	- Add black lines

Luckily no color comes through in plain text mails ;-)

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

Stray blank after >= .

> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==

I accept you can't (currently) use array_access_nospec() here,
but ...

> +             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)];

... I don't see why you still effectively open-code it here.

> @@ -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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];

Same two remarks here then, and again further down.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2577,6 +2577,8 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>      if ( idx >= MAX_ALTP2M )
>          return rc;
>  
> +    idx = array_index_nospec(idx, MAX_ALTP2M);
> +
>      altp2m_list_lock(d);
>  
>      if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )

What about this array access?

> @@ -2618,6 +2620,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>      if ( !idx || idx >= MAX_ALTP2M )
>          return rc;
>  
> +    idx = array_index_nospec(idx, MAX_ALTP2M);

There's a d->arch.altp2m_eptp[] access down from here too. I'm not
going to look further. Please get things into consistent shape while
you do this transformation.

Jan
George Dunlap Dec. 27, 2019, 10:52 a.m. UTC | #6 
On 12/27/19 7:59 AM, Jan Beulich wrote:
> On 23.12.2019 19:08, George Dunlap wrote:
>> What about the attached series of patches (compile-tested only)?
> 
> This ...
> 
>> +#define nospec_clip(index, size)                 \
>> +    ({                                           \
>> +        bool clipped = (index >= size);          \
>> +        index = array_index_nospec(index, size); \
>> +        clipped;                                 \
>> +    })
> 
> ... in particular may misguide people on its use: If the clipped
> "index" gets stored in a register, all is going to be fine (afaict),
> but if it ends up in memory, there's be new (mis-)speculation
> opportunities.

That makes sense; but in that case code like this:

> +    idx = array_index_nospec(idx, MAX_ALTP2M);
> +

...could end up stored on the stack and re-read, couldn't it?  I mean
yes, it's *very likely* going to stay in a register, but there's no way
to actually guarantee it, is there?

 -George
Jan Beulich Dec. 27, 2019, 12:17 p.m. UTC | #7 
On 27.12.2019 11:52, George Dunlap wrote:
> On 12/27/19 7:59 AM, Jan Beulich wrote:
>> On 23.12.2019 19:08, George Dunlap wrote:
>>> What about the attached series of patches (compile-tested only)?
>>
>> This ...
>>
>>> +#define nospec_clip(index, size)                 \
>>> +    ({                                           \
>>> +        bool clipped = (index >= size);          \
>>> +        index = array_index_nospec(index, size); \
>>> +        clipped;                                 \
>>> +    })
>>
>> ... in particular may misguide people on its use: If the clipped
>> "index" gets stored in a register, all is going to be fine (afaict),
>> but if it ends up in memory, there's be new (mis-)speculation
>> opportunities.
> 
> That makes sense; but in that case code like this:
> 
>> +    idx = array_index_nospec(idx, MAX_ALTP2M);
>> +
> 
> ...could end up stored on the stack and re-read, couldn't it?  I mean
> yes, it's *very likely* going to stay in a register, but there's no way
> to actually guarantee it, is there?

Indeed - hence my "Some of the clipping done in the patches is
already not fully safe against this" in the prior response ("the
patches" meaning Alexandru's, not yours, and it would probably
better have been singular).

Jan
Alexandru Stefan ISAILA Jan. 7, 2020, 1:25 p.m. UTC | #8 
On 27.12.2019 10:01, Jan Beulich wrote:
> (re-sending, as I still don't see the mail having appeared on the list)
> 
> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>> Changes since V5:
>> 	- Add black lines
> 
> Luckily no color comes through in plain text mails ;-)
> 
>> --- 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) ||
> 
> Stray blank after >= .
> 
>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> 
> I accept you can't (currently) use array_access_nospec() here,
> but ...
> 
>> +             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)];
> 
> ... I don't see why you still effectively open-code it here.

I am not sure I follow you here, that is what we agreed in v5 
(https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html). 
Did I miss something?


> 
>> @@ -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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
> 
> Same two remarks here then, and again further down.
> 
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2577,6 +2577,8 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
>>       if ( idx >= MAX_ALTP2M )
>>           return rc;
>>   
>> +    idx = array_index_nospec(idx, MAX_ALTP2M);
>> +
>>       altp2m_list_lock(d);
>>   
>>       if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> 
> What about this array access?
> 
>> @@ -2618,6 +2620,8 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>       if ( !idx || idx >= MAX_ALTP2M )
>>           return rc;
>>   
>> +    idx = array_index_nospec(idx, MAX_ALTP2M);
> 
> There's a d->arch.altp2m_eptp[] access down from here too. I'm not
> going to look further. Please get things into consistent shape while
> you do this transformation.
> 

I will change the idx part in p2m_init_altp2m_by_id() and 
p2m_destroy_altp2m_by_id() so they match the rest of the checks:
"if ( idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP))...", drop 
the idx = array_index_nospec(idx, MAX_ALTP2M); and have 
array_index_nospec() into place.


Alex
Jan Beulich Jan. 7, 2020, 1:55 p.m. UTC | #9 
On 07.01.2020 14:25, Alexandru Stefan ISAILA wrote:
> On 27.12.2019 10:01, Jan Beulich wrote:
>> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>>> --- 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) ||
>>
>> Stray blank after >= .
>>
>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>>
>> I accept you can't (currently) use array_access_nospec() here,
>> but ...
>>
>>> +             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)];
>>
>> ... I don't see why you still effectively open-code it here.
> 
> I am not sure I follow you here, that is what we agreed in v5 
> (https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html). 
> Did I miss something?

In context there (from an earlier reply of mine) you will find me
having mentioned array_access_nospec(). This wasn't invalidated or
overridden by my "Yes, that's how I think it ought to be." I didn't
say so explicitly (again) because to me it goes without saying that
open-coding _anything_ is, in the common case, bad practice.

Jan
Alexandru Stefan ISAILA Jan. 7, 2020, 2:31 p.m. UTC | #10 
On 07.01.2020 15:55, Jan Beulich wrote:
> On 07.01.2020 14:25, Alexandru Stefan ISAILA wrote:
>> On 27.12.2019 10:01, Jan Beulich wrote:
>>> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>>>> --- 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) ||
>>>
>>> Stray blank after >= .
>>>
>>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>>>
>>> I accept you can't (currently) use array_access_nospec() here,
>>> but ...
>>>
>>>> +             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)];
>>>
>>> ... I don't see why you still effectively open-code it here.
>>
>> I am not sure I follow you here, that is what we agreed in v5
>> (https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html).
>> Did I miss something?
> 
> In context there (from an earlier reply of mine) you will find me
> having mentioned array_access_nospec(). This wasn't invalidated or
> overridden by my "Yes, that's how I think it ought to be." I didn't
> say so explicitly (again) because to me it goes without saying that
> open-coding _anything_ is, in the common case, bad practice.
> 

So the way to go is to have:

altp2m_idx = array_index_nospec(altp2m_idx, MAX_ALTP2M);
ap2m = d->arch.altp2m_p2m[altp2m_idx];


Alex
Jan Beulich Jan. 7, 2020, 3:06 p.m. UTC | #11 
On 07.01.2020 15:31, Alexandru Stefan ISAILA wrote:
> 
> 
> On 07.01.2020 15:55, Jan Beulich wrote:
>> On 07.01.2020 14:25, Alexandru Stefan ISAILA wrote:
>>> On 27.12.2019 10:01, Jan Beulich wrote:
>>>> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>>>>> --- 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) ||
>>>>
>>>> Stray blank after >= .
>>>>
>>>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>>>>
>>>> I accept you can't (currently) use array_access_nospec() here,
>>>> but ...
>>>>
>>>>> +             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)];
>>>>
>>>> ... I don't see why you still effectively open-code it here.
>>>
>>> I am not sure I follow you here, that is what we agreed in v5
>>> (https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html).
>>> Did I miss something?
>>
>> In context there (from an earlier reply of mine) you will find me
>> having mentioned array_access_nospec(). This wasn't invalidated or
>> overridden by my "Yes, that's how I think it ought to be." I didn't
>> say so explicitly (again) because to me it goes without saying that
>> open-coding _anything_ is, in the common case, bad practice.
>>
> 
> So the way to go is to have:
> 
> altp2m_idx = array_index_nospec(altp2m_idx, MAX_ALTP2M);
> ap2m = d->arch.altp2m_p2m[altp2m_idx];

No. The way to go is to use array_access_nospec() wherever possible.
Besides (as said) avoiding its open-coding, this is the construct
correctly matching your uses of ARRAY_SIZE(), avoiding the explicit
specification of the upper array bound (MAX_ALTP2M). (I really don't
see how my previous reply was not crystal clear in this regard.)

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 320b9fe621..a95a50bcae 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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #else
     ASSERT(!altp2m_idx);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 3119269073..4fc919a9c5 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2577,6 +2577,8 @@  int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
     if ( idx >= MAX_ALTP2M )
         return rc;
 
+    idx = array_index_nospec(idx, MAX_ALTP2M);
+
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
@@ -2618,6 +2620,8 @@  int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
     if ( !idx || idx >= MAX_ALTP2M )
         return rc;
 
+    idx = array_index_nospec(idx, MAX_ALTP2M);
+
     rc = domain_pause_except_self(d);
     if ( rc )
         return rc;
@@ -2689,11 +2693,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
 
     p2m_lock(hp2m);
     p2m_lock(ap2m);
@@ -3032,11 +3038,13 @@  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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+                                                           MAX_ALTP2M)];
     }
     else
         p2m = host_p2m;
@@ -3075,11 +3083,13 @@  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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+                                                           MAX_ALTP2M)];
     }
     else
         p2m = host_p2m;