xen/arm64: Hide FEAT_SME

[PATCH] xen/arm64: Hide FEAT_SME

Posted by Julien Grall 3 months, 1 week ago

Newer hardware may support FEAT_SME. Xen doesn't have any knowledge but
it will still expose the feature to the VM. If the OS is trying to use
SME, then it will crash.

Solve by hiding FEAT_SME.

Signed-off-by: Julien Grall <julien@xen.org>

---

The current approach used to create the domain cpuinfo is to hide
(i.e. a denylist) what we know Xen is not supporting. The drawback
with this approach is for newly introduced feature, Xen will expose it
by default.

If a kernel is trying to use it then it will crash. I can't really
make my mind whether it would be better to expose only what we support
(i.e. use an allowlist).

AFAICT, there is no security concerns with the current approach because
ID_* registers are not a way to tell the kernel which features are
supported. A guest kernel could still try to access the new registers.

So the most annoying bits is that booting Xen on a new HW may lead to
an OS crashing.
---
 xen/arch/arm/cpufeature.c             | 3 +++
 xen/arch/arm/include/asm/cpufeature.h | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index ef77473bf8e3..b45dbe3c668d 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -208,6 +208,9 @@ static int __init create_domain_cpuinfo(void)
     domain_cpuinfo.pfr64.sve = 0;
     domain_cpuinfo.zfr64.bits[0] = 0;
 
+    /* Hide SMT support as Xen does not support it */
+    domain_cpuinfo.pfr64.sme = 0;
+
     /* Hide MTE support as Xen does not support it */
     domain_cpuinfo.pfr64.mte = 0;
 
diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
index c95582044a8a..969e043f5bda 100644
--- a/xen/arch/arm/include/asm/cpufeature.h
+++ b/xen/arch/arm/include/asm/cpufeature.h
@@ -207,7 +207,9 @@ struct cpuinfo_arm {
             unsigned long mte:4;
             unsigned long ras_frac:4;
             unsigned long mpam_frac:4;
-            unsigned long __res1:44;
+            unsigned long __res1:4;
+            unsigned long sme:4;
+            unsigned long __res2:36;
         };
     } pfr64;
 
-- 
2.40.1

Re: [PATCH] xen/arm64: Hide FEAT_SME

Posted by Michal Orzel 3 months, 1 week ago

Hi Julien,

On 14/08/2024 23:00, Julien Grall wrote:
> 
> 
> Newer hardware may support FEAT_SME. Xen doesn't have any knowledge but
> it will still expose the feature to the VM. If the OS is trying to use
> SME, then it will crash.
> 
> Solve by hiding FEAT_SME.
> 
> Signed-off-by: Julien Grall <julien@xen.org>
Acked-by: Michal Orzel <michal.orzel@amd.com>

> 
> ---
> 
> The current approach used to create the domain cpuinfo is to hide
> (i.e. a denylist) what we know Xen is not supporting. The drawback
> with this approach is for newly introduced feature, Xen will expose it
> by default.
> 
> If a kernel is trying to use it then it will crash. I can't really
> make my mind whether it would be better to expose only what we support
> (i.e. use an allowlist).
> 
> AFAICT, there is no security concerns with the current approach because
> ID_* registers are not a way to tell the kernel which features are
> supported. A guest kernel could still try to access the new registers.
I agree with the security aspect but the part of the sentence in the middle is a bit misleading.
ID_ registers *are* a way of informing the kernel about implemented PE features. It's just that
the kernel could still access the features. That said, it should be considered an incorrect behavior
and definitely not something we should worry about.

~Michal

Re: [PATCH] xen/arm64: Hide FEAT_SME

Posted by Julien Grall 3 months ago

Hi,

On 16/08/2024 08:15, Michal Orzel wrote:
  > On 14/08/2024 23:00, Julien Grall wrote:
>>
>>
>> Newer hardware may support FEAT_SME. Xen doesn't have any knowledge but
>> it will still expose the feature to the VM. If the OS is trying to use
>> SME, then it will crash.
>>
>> Solve by hiding FEAT_SME.
>>
>> Signed-off-by: Julien Grall <julien@xen.org>
> Acked-by: Michal Orzel <michal.orzel@amd.com>
> 
>>
>> ---
>>
>> The current approach used to create the domain cpuinfo is to hide
>> (i.e. a denylist) what we know Xen is not supporting. The drawback
>> with this approach is for newly introduced feature, Xen will expose it
>> by default.
>>
>> If a kernel is trying to use it then it will crash. I can't really
>> make my mind whether it would be better to expose only what we support
>> (i.e. use an allowlist).
>>
>> AFAICT, there is no security concerns with the current approach because
>> ID_* registers are not a way to tell the kernel which features are
>> supported. A guest kernel could still try to access the new registers.
> I agree with the security aspect but the part of the sentence in the middle is a bit misleading.

Indeed. It was poorly worded. I was meant to say what you wrote below :).

> ID_ registers *are* a way of informing the kernel about implemented PE features. It's just that
> the kernel could still access the features. That said, it should be considered an incorrect behavior
> and definitely not something we should worry about.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm64: Hide FEAT_SME

Posted by Ayan Kumar Halder 3 months, 1 week ago

Hi Julien,

On 14/08/2024 22:00, Julien Grall wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> Newer hardware may support FEAT_SME. Xen doesn't have any knowledge but
> it will still expose the feature to the VM. If the OS is trying to use
> SME, then it will crash.
>
> Solve by hiding FEAT_SME.
>
> Signed-off-by: Julien Grall <julien@xen.org>
>
> ---
>
> The current approach used to create the domain cpuinfo is to hide
> (i.e. a denylist) what we know Xen is not supporting. The drawback
> with this approach is for newly introduced feature, Xen will expose it
> by default.
>
> If a kernel is trying to use it then it will crash. I can't really
> make my mind whether it would be better to expose only what we support
> (i.e. use an allowlist).
>
> AFAICT, there is no security concerns with the current approach because
> ID_* registers are not a way to tell the kernel which features are
> supported. A guest kernel could still try to access the new registers.
>
> So the most annoying bits is that booting Xen on a new HW may lead to
> an OS crashing.
> ---
>   xen/arch/arm/cpufeature.c             | 3 +++
>   xen/arch/arm/include/asm/cpufeature.h | 4 +++-
>   2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
> index ef77473bf8e3..b45dbe3c668d 100644
> --- a/xen/arch/arm/cpufeature.c
> +++ b/xen/arch/arm/cpufeature.c
> @@ -208,6 +208,9 @@ static int __init create_domain_cpuinfo(void)
>       domain_cpuinfo.pfr64.sve = 0;
>       domain_cpuinfo.zfr64.bits[0] = 0;
>
> +    /* Hide SMT support as Xen does not support it */
> +    domain_cpuinfo.pfr64.sme = 0;

Instead of this, can we do the following :-

domain_cpuinfo.pfr64.res1 = 0;
This would imply that SME, RNDR_trap, CSV2_frac, NMI, etc are not supported.

If later Xen decides to support any of these, then they can be selectively turned on for a domain in do_sysreg() (Similar to SVE).

- Ayan

Re: [PATCH] xen/arm64: Hide FEAT_SME

Posted by Julien Grall 3 months, 1 week ago

On 15/08/2024 09:58, Ayan Kumar Halder wrote:
> Hi Julien,

Hi Ayan,

> On 14/08/2024 22:00, Julien Grall wrote:
>> CAUTION: This message has originated from an External Source. Please 
>> use proper judgment and caution when opening attachments, clicking 
>> links, or responding to this email.
>>
>>
>> Newer hardware may support FEAT_SME. Xen doesn't have any knowledge but
>> it will still expose the feature to the VM. If the OS is trying to use
>> SME, then it will crash.
>>
>> Solve by hiding FEAT_SME.
>>
>> Signed-off-by: Julien Grall <julien@xen.org>
>>
>> ---
>>
>> The current approach used to create the domain cpuinfo is to hide
>> (i.e. a denylist) what we know Xen is not supporting. The drawback
>> with this approach is for newly introduced feature, Xen will expose it
>> by default.
>>
>> If a kernel is trying to use it then it will crash. I can't really
>> make my mind whether it would be better to expose only what we support
>> (i.e. use an allowlist).
>>
>> AFAICT, there is no security concerns with the current approach because
>> ID_* registers are not a way to tell the kernel which features are
>> supported. A guest kernel could still try to access the new registers.
>>
>> So the most annoying bits is that booting Xen on a new HW may lead to
>> an OS crashing.
>> ---
>>   xen/arch/arm/cpufeature.c             | 3 +++
>>   xen/arch/arm/include/asm/cpufeature.h | 4 +++-
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>> index ef77473bf8e3..b45dbe3c668d 100644
>> --- a/xen/arch/arm/cpufeature.c
>> +++ b/xen/arch/arm/cpufeature.c
>> @@ -208,6 +208,9 @@ static int __init create_domain_cpuinfo(void)
>>       domain_cpuinfo.pfr64.sve = 0;
>>       domain_cpuinfo.zfr64.bits[0] = 0;
>>
>> +    /* Hide SMT support as Xen does not support it */
>> +    domain_cpuinfo.pfr64.sme = 0;
> 
> Instead of this, can we do the following :-
> 
> domain_cpuinfo.pfr64.res1 = 0;
> This would imply that SME, RNDR_trap, CSV2_frac, NMI, etc are not 
> supported.

I could but I haven't done it for two reasons:
   * AFAICT, there are no issue to expose RNDR_trap to the guest. Also 
not all the bits in the field res1 are defined yet. So effectively, we 
would be implementing an allowlist like.
   * In the future, we could split res1 in two. If we are not careful, 
we would expose the feature again.

So I find this approach rather risky. There is also a more general 
question on how the features should be handled (see what I wrote after 
--- and below).

> 
> If later Xen decides to support any of these, then they can be 
> selectively turned on for a domain in do_sysreg() 

do_sysreg() is just returning the ID registers. This only helps the OS 
to discover the features at runtime and it is free to ignore them. What 
matter is the configuration in of the trap registers (such as HCR_EL2).

If a feature is not gated by a trap register, then it could be enabled 
everywhere. So effectively, the decision will depend on the feature. In 
the future, we may have to take care of suspend/resume or live-migration 
between two Xen versions. At that point the feature may need to be 
per-domain.

 > (Similar to SVE).

The main reason SVE is enabled per-domain is because of the large state. 
But if we have feature that doesn't have an impact on the memory usage, 
then they could be enabled everywhere.

Anyway, the first decision we need to take is whether we want to change 
to an allowlist. There are pros and cons with both of them (see what I 
wrote above). At the moment, I am still leaning towards the existing 
model which is expose everything by default but hide features when they 
appear and Xen needs more handling.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm64: Hide FEAT_SME

Posted by Ayan Kumar Halder 3 months, 1 week ago

On 15/08/2024 10:37, Julien Grall wrote:
>
>
> On 15/08/2024 09:58, Ayan Kumar Halder wrote:
>> Hi Julien,
>
> Hi Ayan,
Hi Julien,
>
>> On 14/08/2024 22:00, Julien Grall wrote:
>>> CAUTION: This message has originated from an External Source. Please 
>>> use proper judgment and caution when opening attachments, clicking 
>>> links, or responding to this email.
>>>
>>>
>>> Newer hardware may support FEAT_SME. Xen doesn't have any knowledge but
>>> it will still expose the feature to the VM. If the OS is trying to use
>>> SME, then it will crash.
>>>
>>> Solve by hiding FEAT_SME.
>>>
>>> Signed-off-by: Julien Grall <julien@xen.org>
>>>
>>> ---
>>>
>>> The current approach used to create the domain cpuinfo is to hide
>>> (i.e. a denylist) what we know Xen is not supporting. The drawback
>>> with this approach is for newly introduced feature, Xen will expose it
>>> by default.
>>>
>>> If a kernel is trying to use it then it will crash. I can't really
>>> make my mind whether it would be better to expose only what we support
>>> (i.e. use an allowlist).
>>>
>>> AFAICT, there is no security concerns with the current approach because
>>> ID_* registers are not a way to tell the kernel which features are
>>> supported. A guest kernel could still try to access the new registers.
>>>
>>> So the most annoying bits is that booting Xen on a new HW may lead to
>>> an OS crashing.
>>> ---
>>>   xen/arch/arm/cpufeature.c             | 3 +++
>>>   xen/arch/arm/include/asm/cpufeature.h | 4 +++-
>>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>> index ef77473bf8e3..b45dbe3c668d 100644
>>> --- a/xen/arch/arm/cpufeature.c
>>> +++ b/xen/arch/arm/cpufeature.c
>>> @@ -208,6 +208,9 @@ static int __init create_domain_cpuinfo(void)
>>>       domain_cpuinfo.pfr64.sve = 0;
>>>       domain_cpuinfo.zfr64.bits[0] = 0;
>>>
>>> +    /* Hide SMT support as Xen does not support it */
>>> +    domain_cpuinfo.pfr64.sme = 0;
>>
>> Instead of this, can we do the following :-
>>
>> domain_cpuinfo.pfr64.res1 = 0;
>> This would imply that SME, RNDR_trap, CSV2_frac, NMI, etc are not 
>> supported.
>
> I could but I haven't done it for two reasons:
>   * AFAICT, there are no issue to expose RNDR_trap to the guest. Also 
> not all the bits in the field res1 are defined yet. So effectively, we 
> would be implementing an allowlist like.
>   * In the future, we could split res1 in two. If we are not careful, 
> we would expose the feature again.
I see your point.
>
> So I find this approach rather risky. There is also a more general 
> question on how the features should be handled (see what I wrote after 
> --- and below).
>
>>
>> If later Xen decides to support any of these, then they can be 
>> selectively turned on for a domain in do_sysreg() 
>
> do_sysreg() is just returning the ID registers. This only helps the OS 
> to discover the features at runtime and it is free to ignore them. 
> What matter is the configuration in of the trap registers (such as 
> HCR_EL2).
>
> If a feature is not gated by a trap register, then it could be enabled 
> everywhere. So effectively, the decision will depend on the feature. 
> In the future, we may have to take care of suspend/resume or 
> live-migration between two Xen versions. At that point the feature may 
> need to be per-domain.
>
> > (Similar to SVE).
>
> The main reason SVE is enabled per-domain is because of the large 
> state. But if we have feature that doesn't have an impact on the 
> memory usage, then they could be enabled everywhere.
>
> Anyway, the first decision we need to take is whether we want to 
> change to an allowlist. There are pros and cons with both of them (see 
> what I wrote above). At the moment, I am still leaning towards the 
> existing model which is expose everything by default but hide features 
> when they appear and Xen needs more handling.

This makes sense. The existing model is simple.

Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>