[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH HVM v1 1/1] hvm: refactor set param
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |