| Message ID | 20210129135950.32095-1-nmanthey@amazon.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [HVM,v1,1/1] hvm: refactor set param | expand |
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.
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; 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; + + /* Make sure we evaluate permissions before loading data of domains. */ + block_speculation(); + + value = d->arch.hvm_domain.params[index]; switch ( index ) { /* The following parameters should only be changed once. */ @@ -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(); - d = rcu_lock_domain_by_any_id(a.domid); if ( d == NULL ) return -ESRCH;
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. Signed-off-by: Norbert Manthey <nmanthey@amazon.de> Reported-by: Hongyan Xia <hongyxia@amazon.co.uk> --- xen/arch/x86/hvm/hvm.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)