Re: [Xen-devel] [PATCH V4] x86/altp2m: Hypercall to set altp2m view visibility

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2020 09:30, Alexandru Stefan ISAILA wrote:
> @@ -4835,6 +4836,26 @@ static int do_altp2m_op(
>          break;
>      }
>  
> +    case HVMOP_altp2m_set_visibility:
> +    {
> +        uint16_t idx = a.u.set_visibility.altp2m_idx;
> +
> +        if ( a.u.set_visibility.pad ||
> +             idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||

Why min() here? You only access MAX_EPTP-dimensioned arrays afaics. If
this is intentional, I think it deserves a comment.

> +             d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
> +             mfn_x(INVALID_MFN) )
> +            rc = -EINVAL;
> +        else if ( !altp2m_active(d) )
> +            rc = -EOPNOTSUPP;
> +        else if ( a.u.set_visibility.visible )
> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
> +                d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)];
> +        else
> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
> +                mfn_x(INVALID_MFN);
> +        break;

You don't seem to be holding any locks. At the very least this means
the in-range-index-is-valid check further up will have become stale
by the time you actually consume the slot.

> @@ -2638,7 +2639,9 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned 
> int idx)
>          {
>              p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
>              d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
> -            mfn_x(INVALID_MFN);
> +                mfn_x(INVALID_MFN);
> +            d->arch.altp2m_working_eptp[array_index_nospec(idx, MAX_EPTP)] =
> +                mfn_x(INVALID_MFN);

I appreciate you also adjusting the bogus indentation of the pre-
existing line.

Jan

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