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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.12.2019 10:42, 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 V4:
>       - Change bounds check from MAX_EPTP to MAX_ALTP2M
>       - Move array_index_nospec() closer to the bounds check.
> ---
>  xen/arch/x86/mm/mem_access.c | 15 +++++++++------
>  xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 320b9fe621..33e379db8f 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_ALTP2M)] 
> ==

As implied by a reply to v4, this is still latently buggy: There's
no guarantee anyone will notice the issue here when bumping
MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
the actual bounds check. Then each array access itself can be made
use the correct bound. In fact you'd probably have noticed this if
you had made use of array_access_nospec() where possible (which
also would help readability) - apparently not 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)];

... here. The min() suggested above would then better become
min(ARRAY_SIZE(d->arch.altp2m_eptp), MAX_EPTP), which I think
would then even compile cleanly (the apparently simpler form
above wouldn't as is afaict).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2574,6 +2574,7 @@ 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);

I wouldn't object to there being no blank line between the if()
and the line you add, but you surely want a blank line ahead of
the unrelated lock acquire (similarly at least once more below).

Jan

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