| Message ID | 20210205203905.8824-1-nmanthey@amazon.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [HVM,v2,1/1] hvm: refactor set param | expand |
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@amazon.de> > Reported-by: Hongyan Xia <hongyxia@amazon.co.uk> Did you lose Ian's release-ack, or did you drop it for a specific reason? > --- 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. 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. > @@ -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). > @@ -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. > @@ -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. Jan
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@amazon.de> >> Reported-by: Hongyan Xia <hongyxia@amazon.co.uk> > 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
On 08.02.2021 20:47, Norbert Manthey wrote: > On 2/8/21 3:21 PM, Jan Beulich wrote: >> On 05.02.2021 21:39, Norbert Manthey wrote: >>> @@ -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. Okay, looks like I got mislead by the added bounds check. Why do you add that, when the sole caller already has one? It'll suffice since you move the array access past the barrier, won't it? >>> @@ -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. I'm suspecting a misunderstanding (the more that further down you did agree to what I've said for hvmop_get_param()): I'm not saying your addition is pointless. Instead I'm saying that your addition should be accompanied by removal of the barrier from hvmop_set_param(), paralleling what you do to hvmop_get_param(). And additionally I'm saying that just like in hvmop_get_param() the barrier there was already previously redundant with that inside is_hvm_domain(). Jan
On 2/9/21 10:40 AM, Jan Beulich wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > On 08.02.2021 20:47, Norbert Manthey wrote: >> On 2/8/21 3:21 PM, Jan Beulich wrote: >>> On 05.02.2021 21:39, Norbert Manthey wrote: >>>> @@ -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. > Okay, looks like I got mislead by the added bounds check. Why > do you add that, when the sole caller already has one? It'll > suffice since you move the array access past the barrier, > won't it? I can drop that bound check again. This was added to make sure other callers would be save as well. Thinking about this a little more, the check could actually be moved into the hvm_allow_set_param function, above the first index access in that function. Are there good reasons to not move the index check into the allow function? > >>>> @@ -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. > I'm suspecting a misunderstanding (the more that further down > you did agree to what I've said for hvmop_get_param()): I'm > not saying your addition is pointless. Instead I'm saying that > your addition should be accompanied by removal of the barrier > from hvmop_set_param(), paralleling what you do to > hvmop_get_param(). And additionally I'm saying that just like > in hvmop_get_param() the barrier there was already previously > redundant with that inside is_hvm_domain(). I now understand, thank you. I agree, the already existing barrier in the hvmop_set_param function can be dropped as well. I will update the diff accordingly, after we concluded where to put the index check. Best, Norbert 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
On 09.02.2021 14:41, Norbert Manthey wrote: > On 2/9/21 10:40 AM, Jan Beulich wrote: >> On 08.02.2021 20:47, Norbert Manthey wrote: >>> On 2/8/21 3:21 PM, Jan Beulich wrote: >>>> On 05.02.2021 21:39, Norbert Manthey wrote: >>>>> @@ -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. >> Okay, looks like I got mislead by the added bounds check. Why >> do you add that, when the sole caller already has one? It'll >> suffice since you move the array access past the barrier, >> won't it? > I can drop that bound check again. This was added to make sure other > callers would be save as well. Thinking about this a little more, the > check could actually be moved into the hvm_allow_set_param function, > above the first index access in that function. Are there good reasons to > not move the index check into the allow function? I guess I'm confused: We're talking about dropping the check you add to hvm_allow_set_param() and you suggest to "move" it right there? Jan
On 2/9/21 2:45 PM, Jan Beulich wrote: > On 09.02.2021 14:41, Norbert Manthey wrote: >> On 2/9/21 10:40 AM, Jan Beulich wrote: >>> On 08.02.2021 20:47, Norbert Manthey wrote: >>>> On 2/8/21 3:21 PM, Jan Beulich wrote: >>>>> On 05.02.2021 21:39, Norbert Manthey wrote: >>>>>> @@ -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. >>> Okay, looks like I got mislead by the added bounds check. Why >>> do you add that, when the sole caller already has one? It'll >>> suffice since you move the array access past the barrier, >>> won't it? >> I can drop that bound check again. This was added to make sure other >> callers would be save as well. Thinking about this a little more, the >> check could actually be moved into the hvm_allow_set_param function, >> above the first index access in that function. Are there good reasons to >> not move the index check into the allow function? > I guess I'm confused: We're talking about dropping the check > you add to hvm_allow_set_param() and you suggest to "move" it > right there? Yes. I can either just no introduce that check in that function (which is what you suggested). As an alternative, to have all checks in one function, I proposed to instead move it into hvm_allow_set_param. If you do not like this proposal, I'll follow your suggestion and will simply not introduce the additional bound check as part of this patch. Best, Norbert 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
On 09.02.2021 14:56, Norbert Manthey wrote: > On 2/9/21 2:45 PM, Jan Beulich wrote: >> On 09.02.2021 14:41, Norbert Manthey wrote: >>> On 2/9/21 10:40 AM, Jan Beulich wrote: >>>> On 08.02.2021 20:47, Norbert Manthey wrote: >>>>> On 2/8/21 3:21 PM, Jan Beulich wrote: >>>>>> On 05.02.2021 21:39, Norbert Manthey wrote: >>>>>>> @@ -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. >>>> Okay, looks like I got mislead by the added bounds check. Why >>>> do you add that, when the sole caller already has one? It'll >>>> suffice since you move the array access past the barrier, >>>> won't it? >>> I can drop that bound check again. This was added to make sure other >>> callers would be save as well. Thinking about this a little more, the >>> check could actually be moved into the hvm_allow_set_param function, >>> above the first index access in that function. Are there good reasons to >>> not move the index check into the allow function? >> I guess I'm confused: We're talking about dropping the check >> you add to hvm_allow_set_param() and you suggest to "move" it >> right there? > > Yes. I can either just no introduce that check in that function (which > is what you suggested). As an alternative, to have all checks in one > function, I proposed to instead move it into hvm_allow_set_param. Oh, I see. What I'd like to be the result of your re-arrangement is symmetry between "get" and "set" where possible in this regard, and asymmetry having a clear reason. Seeing that hvm_{get,set}_param() are the non-static functions here, I think it would be quite desirable for the bounds checking to live there. Since hvm_allow_{get,set}_param() are specifically helpers of the two earlier named functions, checks consistently living there would as well be fine with me. Jan
On 2/9/21 3:21 PM, Jan Beulich wrote: > On 09.02.2021 14:56, Norbert Manthey wrote: >> On 2/9/21 2:45 PM, Jan Beulich wrote: >>> On 09.02.2021 14:41, Norbert Manthey wrote: >>>> On 2/9/21 10:40 AM, Jan Beulich wrote: >>>>> On 08.02.2021 20:47, Norbert Manthey wrote: >>>>>> On 2/8/21 3:21 PM, Jan Beulich wrote: >>>>>>> On 05.02.2021 21:39, Norbert Manthey wrote: >>>>>>>> @@ -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. >>>>> Okay, looks like I got mislead by the added bounds check. Why >>>>> do you add that, when the sole caller already has one? It'll >>>>> suffice since you move the array access past the barrier, >>>>> won't it? >>>> I can drop that bound check again. This was added to make sure other >>>> callers would be save as well. Thinking about this a little more, the >>>> check could actually be moved into the hvm_allow_set_param function, >>>> above the first index access in that function. Are there good reasons to >>>> not move the index check into the allow function? >>> I guess I'm confused: We're talking about dropping the check >>> you add to hvm_allow_set_param() and you suggest to "move" it >>> right there? >> Yes. I can either just no introduce that check in that function (which >> is what you suggested). As an alternative, to have all checks in one >> function, I proposed to instead move it into hvm_allow_set_param. > Oh, I see. What I'd like to be the result of your re-arrangement is > symmetry between "get" and "set" where possible in this regard, and > asymmetry having a clear reason. Seeing that hvm_{get,set}_param() > are the non-static functions here, I think it would be quite > desirable for the bounds checking to live there. Since > hvm_allow_{get,set}_param() are specifically helpers of the two > earlier named functions, checks consistently living there would as > well be fine with me. I agree with the symmetry for get and set. This is what I'd aim for: 1. hvmop_set_param and hvmop_get_param (static) both check for the index, and afterwards use the is_hvm_domain(d) function with its barrier 2. hvm_set_param (static) and hvm_get_param both call their allow helper function, evaluate the return code, and afterwards block speculation. 2.1. hvm_get_param is declared in a public header, and cannot be turned into a static function, hence needs the index check 2.2. hvm_set_param is only called from hvmop_set_param, and index is already checked there, hence, do not add check 3. hvm_allow_set_param (static) and hvm_allow_get_param (static) do not validate the index parameter 3.1. hvm_allow_set_param blocks speculative execution with a barrier after domain permissions have been evaluated, before accessing the parameters of the domain. hvm_allow_get_param does not access the params member of the domain, and hence does not require additional protection. To simplify the code, I propose to furthermore make the hvmop_set_param function static as well. Please let me know whether the above would is acceptable. 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
On 11.02.2021 21:46, Norbert Manthey wrote: > I agree with the symmetry for get and set. This is what I'd aim for: > > 1. hvmop_set_param and hvmop_get_param (static) both check for the > index, and afterwards use the is_hvm_domain(d) function with its barrier > 2. hvm_set_param (static) and hvm_get_param both call their allow > helper function, evaluate the return code, and afterwards block speculation. > 2.1. hvm_get_param is declared in a public header, and cannot be turned > into a static function, hence needs the index check But both further call sites are in bounded loops, with the bounds not guest controlled. It can rely on the callers just as much as ... > 2.2. hvm_set_param is only called from hvmop_set_param, and index is > already checked there, hence, do not add check ... this. > 3. hvm_allow_set_param (static) and hvm_allow_get_param (static) do not > validate the index parameter > 3.1. hvm_allow_set_param blocks speculative execution with a barrier > after domain permissions have been evaluated, before accessing the > parameters of the domain. hvm_allow_get_param does not access the params > member of the domain, and hence does not require additional protection. > > To simplify the code, I propose to furthermore make the hvmop_set_param > function static as well. Yes - this not being so already is likely simply an oversight, supported by the fact that there's no declaration in any header. Jan
On 2/12/21 11:04 AM, Jan Beulich wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > On 11.02.2021 21:46, Norbert Manthey wrote: >> I agree with the symmetry for get and set. This is what I'd aim for: >> >> 1. hvmop_set_param and hvmop_get_param (static) both check for the >> index, and afterwards use the is_hvm_domain(d) function with its barrier >> 2. hvm_set_param (static) and hvm_get_param both call their allow >> helper function, evaluate the return code, and afterwards block speculation. >> 2.1. hvm_get_param is declared in a public header, and cannot be turned >> into a static function, hence needs the index check > But both further call sites are in bounded loops, with the bounds not > guest controlled. It can rely on the callers just as much as ... Okay, so I will not add the check there either. I thought about future modifications that allow to call that function from other places, or modified call environments with eventually guest control - but I am fine to not consider these. > >> 2.2. hvm_set_param is only called from hvmop_set_param, and index is >> already checked there, hence, do not add check > ... this. > >> 3. hvm_allow_set_param (static) and hvm_allow_get_param (static) do not >> validate the index parameter >> 3.1. hvm_allow_set_param blocks speculative execution with a barrier >> after domain permissions have been evaluated, before accessing the >> parameters of the domain. hvm_allow_get_param does not access the params >> member of the domain, and hence does not require additional protection. >> >> To simplify the code, I propose to furthermore make the hvmop_set_param >> function static as well. > Yes - this not being so already is likely simply an oversight, > supported by the fact that there's no declaration in any header. Okay. Best, Norbert 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
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.params[index]; switch ( index ) { /* The following parameters should only be changed once. */ @@ -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: @@ -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(); + switch ( index ) { case HVM_PARAM_ACPI_S_STATE: @@ -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;
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> --- v2: Add another speculative blocker, which protects the return code check of the function hvm_allow_set_param. xen/arch/x86/hvm/hvm.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)