Re: [PATCH HVM v1 1/1] hvm: refactor set param

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Hi.  Thanks for this.

Norbert Manthey writes ("[PATCH HVM v1 1/1] hvm: refactor set param"):
> To prevent leaking HVM params via L1TF and similar issues on a
> hyperthread pair, let's load values of domains as late as possible.
> 
> Furthermore, speculative barriers are re-arranged to make sure we do not
> allow guests running on co-located VCPUs to leak hvm parameter values of
> other domains.
> 
> This is part of the speculative hardening effort.

This missed the feature freeze last posing date.  But I think it may
well warrant a freeze exception.  Could someone who understands this
better explain to me the risks of this patch, and the risks of not
taking it ?

I had two comments from reading the diff (but not the code in context):

> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4060,7 +4060,7 @@ static int hvm_allow_set_param(struct domain *d,
>                                 uint32_t index,
>                                 uint64_t new_value)
>  {
> -    uint64_t value = d->arch.hvm.params[index];
> +    uint64_t value;
>      int rc;

This leaves the value "value" uninitialised.  Does hypervisor style
not allow late declaration ?  We can hope, I guess, that the compiler
would spot uses of value between here and the point of use.  But maybe
&value is used.  </Rust-programmer>

>      rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
> @@ -4108,6 +4108,13 @@ static int hvm_allow_set_param(struct domain *d,
>      if ( rc )
>          return rc;
>  
> +    if ( index >= HVM_NR_PARAMS )
> +        return -EINVAL;

This condition is new AFAICT.  Presumably it duplicates an earlier
check for the same condition ?  I'm not sure I fully understand the
implications.  But maybe I don't need to.

> @@ -4388,6 +4395,10 @@ int hvm_get_param(struct domain *d, uint32_t index, 
> uint64_t *value)
>      if ( rc )
>          return rc;
>  
> +    /* Make sure the index bound check in hvm_get_param is respected, as 
> well as
> +       the above domain permissions. */
> +    block_speculation();
> +
>      switch ( index )
>      {
>      case HVM_PARAM_ACPI_S_STATE:
> @@ -4428,9 +4439,6 @@ static int hvmop_get_param(
>      if ( a.index >= HVM_NR_PARAMS )
>          return -EINVAL;
>  
> -    /* Make sure the above bound check is not bypassed during speculation. */
> -    block_speculation();
> -

This pair of hunks seem "more obviously" OK to me...

Thanks,
Ian.