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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxx>
> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> ---
> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> CC: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
> 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

Attachment: 0001-x86-p2m-Remove-some-trailing-whitespace.patch
Description: Text Data

Attachment: 0002-x86-altp2m-Restrict-MAX_EPTP-to-hap.c.patch
Description: Text Data

Attachment: 0003-nospec-Introduce-nospec_clip-macro.patch
Description: Text Data

Attachment: 0004-x86-mm-Use-nospec_clip-to-check-guest-supplied-value.patch
Description: Text Data

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