[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH HVM v2 1/1] hvm: refactor set param
On 2/8/21 3:21 PM, Jan Beulich wrote: > On 05.02.2021 21:39, Norbert Manthey wrote: >> 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@xxxxxxxxx> >> Reported-by: Hongyan Xia <hongyxia@xxxxxxxxxxxx> > Did you lose Ian's release-ack, or did you drop it for a specific > reason? That happened by accident, similarly to not chaining this v2 to the former v1. I'll add it to the next revision. > >> --- 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.params[index]; >> switch ( index ) >> { >> /* The following parameters should only be changed once. */ > I don't see the need for the heavier block_speculation() here; > afaict array_access_nospec() should do fine. The switch() in > context above as well as the switch() further down in the > function don't have any speculation susceptible code. The reason to block speculation instead of just using the hardened index access is to not allow to speculatively load data from another domain. > > Furthermore the first switch() doesn't use "value" at all, so > you could move the access even further down. This may have the > downside of adding latency, so may not be worth it, but in > this case at least the description should say half a word, > especially seeing it say "as late as possible" right now. Agreed, I can move this further down, or adapt the wording. The initial intention was to move it only below the first possible speculative blocker. Hence, let me adapt the wording. > >> @@ -4141,6 +4148,9 @@ static int hvm_set_param(struct domain *d, uint32_t >> index, uint64_t value) >> if ( rc ) >> return rc; >> >> + /* Make sure we evaluate permissions before loading data of domains. */ >> + block_speculation(); >> + >> switch ( index ) >> { >> case HVM_PARAM_CALLBACK_IRQ: > Like you do for the "get" path I think this similarly renders > pointless the use in hvmop_set_param() (and - see below - the > same consideration wrt is_hvm_domain() applies). Can you please be more specific why this is pointless? I understand that the is_hvm_domain check comes with a barrier that can be used to not add another barrier. However, I did not find such a barrier here, which comes between the 'if (rc)' just above, and the potential next access based on the value of 'index'. At least the access behind the switch statement cannot be optimized and replaced with a constant value easily. > >> @@ -4388,6 +4398,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(); > Nit: Please fix comment style here. Will do. > >> @@ -4428,9 +4442,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; > This one really was pointless anyway, as is_hvm_domain() (used > down from here) already contains a suitable barrier. Yes, agreed. Best, Norbert > > Jan Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |