| Message ID | 20240814210054.67520-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm64: Hide FEAT_SME | expand |
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
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,
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>
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
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,
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;
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(-)