[PATCH HVM v2 1/1] hvm: refactor set param

[PATCH HVM v2 1/1] hvm: refactor set param

[Norbert Manthey]

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(-)

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;
-- 
2.17.1




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

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

[Jan Beulich]

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

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

[Norbert Manthey]

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

[PATCH HVM v3 1/1] hvm: refactor set param

[Norbert Manthey]

To prevent leaking HVM params via L1TF and similar issues on a
hyperthread pair, let's load values of domains after performing all
relevant checks, and blocking speculative execution.

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>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

---
v3: * rephrased commit message to better explain code relocation
    * added release-acked


 xen/arch/x86/hvm/hvm.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

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;
-- 
2.17.1




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

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

[Jan Beulich]

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

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

[Jan Beulich]

On 08.02.2021 21:00, Norbert Manthey wrote:
> To prevent leaking HVM params via L1TF and similar issues on a
> hyperthread pair, let's load values of domains after performing all
> relevant checks, and blocking speculative execution.

I'd like to suggest "..., let's load values of domains only
after ...".

But there are other points open from v2; I'd like to further
suggest that you allow discussion on a prior version to first
settle, before sending a new one. Unless of course discussion
appears to have stalled.

Jan

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

[Norbert Manthey]

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

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

[Jan Beulich]

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

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

[Norbert Manthey]

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

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

[Jan Beulich]

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

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

[Norbert Manthey]

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

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

[Jan Beulich]

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

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

[Norbert Manthey]

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

[PATCH HVM v4 1/1] hvm: refactor set param

[Norbert Manthey]

To prevent leaking HVM params via L1TF and similar issues on a
hyperthread pair, let's load values of domains only after performing all
relevant checks, and blocking speculative execution.

For both get and set, the value of the index is already checked in the
outer calling function. The block_speculation calls in hvmop_get_param
and hvmop_set_param are removed, because is_hvm_domain already blocks
speculation.

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.

To improve symmetry between the get and set operations, function
hvmop_set_param is made static.

This is part of the speculative hardening effort.

Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Reported-by: Hongyan Xia <hongyxia@amazon.co.uk>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

---
v4: * add 'static' attribute to hvmop_set_param
    * drop introduced bound checks, e.g. in hvm_allow_set_param
    * drop existing bound check from hvm_set_param
    * do not introduce block_speculation in hvmop_set_param,
      as is_hvm_domain already blocks speculation
    * fix comments

 xen/arch/x86/hvm/hvm.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

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,10 @@ static int hvm_allow_set_param(struct domain *d,
     if ( rc )
         return rc;
 
+    /* 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. */
@@ -4134,13 +4138,13 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
     struct vcpu *v;
     int rc;
 
-    if ( index >= HVM_NR_PARAMS )
-        return -EINVAL;
-
     rc = hvm_allow_set_param(d, index, value);
     if ( rc )
         return rc;
 
+    /* Make sure we evaluate permissions before loading data of domains. */
+    block_speculation();
+
     switch ( index )
     {
     case HVM_PARAM_CALLBACK_IRQ:
@@ -4305,7 +4309,7 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value)
     return rc;
 }
 
-int hvmop_set_param(
+static int hvmop_set_param(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
 {
     struct xen_hvm_param a;
@@ -4318,9 +4322,6 @@ int hvmop_set_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;
@@ -4388,6 +4389,9 @@ int hvm_get_param(struct domain *d, uint32_t index, uint64_t *value)
     if ( rc )
         return rc;
 
+    /* Make sure the above domain permissions check is respected. */
+    block_speculation();
+
     switch ( index )
     {
     case HVM_PARAM_ACPI_S_STATE:
@@ -4428,9 +4432,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;
-- 
2.17.1




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

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

[Jan Beulich]

On 18.02.2021 16:01, Norbert Manthey wrote:
> To prevent leaking HVM params via L1TF and similar issues on a
> hyperthread pair, let's load values of domains only after performing all
> relevant checks, and blocking speculative execution.
> 
> For both get and set, the value of the index is already checked in the
> outer calling function. The block_speculation calls in hvmop_get_param
> and hvmop_set_param are removed, because is_hvm_domain already blocks
> speculation.
> 
> 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.
> 
> To improve symmetry between the get and set operations, function
> hvmop_set_param is made static.
> 
> This is part of the speculative hardening effort.
> 
> Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
> Reported-by: Hongyan Xia <hongyxia@amazon.co.uk>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Reviewed-by: Jan Beulich <jbeulich@suse.com>