| Message ID | 20240220121743.3680715-2-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add emulation of Debug Data Transfer Registers | expand |
On 20.02.2024 13:17, Ayan Kumar Halder wrote: > There can be situations when the registers cannot be emulated to their full > functionality. This can be due to the complexity involved. In such cases, one > can emulate those registers as RAZ/WI for example. We call them as partial > emulation. > > Some registers are non-optional and as such there is nothing preventing an OS > from accessing them. > Instead of injecting undefined exception (thus crashing a guest), one may want > to prefer a partial emulation to let the guest running (in some cases accepting > the fact that it might result in unwanted behavior). > > A suitable example of this (as seen in subsequent patches) is emulation of > DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional > registers can be emulated as RAZ/WI and they can be enclosed within > CONFIG_PARTIAL_EMULATION. > > Further, "partial-emulation" command line option allows us to > enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION > enables support for partial emulation at compile time (i.e. adds code for > partial emulation), this option may be enabled or disabled by Yocto or other > build systems. However if the build system turns this option on, users > can use scripts like Imagebuilder to generate uboot-script which will append > "partial-emulation=true" to xen command line to turn on the partial emulation. > Thus, it helps to avoid rebuilding xen. > > By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false". > This is done so that Xen supports partial emulation. However, customers are > fully aware when they enable partial emulation. It's important to note that > enabling such support might result in unwanted/non-spec compliant behavior. > > Added a note in SUPPORT.md to clarify the security support for partial > emulation. There may have been prior discussion of this, but I view this as too little to justify ... > --- a/SUPPORT.md > +++ b/SUPPORT.md > @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to support MSI. > > Status: Experimental > > +### ARM/Partial Emulation > + > +Enable partial emulation of registers, otherwise considered unimplemented, > +that would normally trigger a fault injection. > + > + Status: Supported, with caveats > + > +Bugs allowing the userspace to attack the guest OS will not be considered > +security vulnerabilities. > + > +Bugs that could compromise Xen will be considered security vulnerabilities. ... the odd statement regarding in-guest vulnerabilities that might be introduced. I see only two ways of treating this as supported: Either you simply refuse emulation when the access is from user space, or you support that mode of emulation as much as that of kernel space accesses. After all "injecting undefined exception (thus crashing a guest)" (quoting the description) doesn't seem quite right: If you inject such an exception for a user space access, all that ought to happen is the user space process to be killed. Jan
Hi Jan, On 20/02/2024 12:33, Jan Beulich wrote: > On 20.02.2024 13:17, Ayan Kumar Halder wrote: >> There can be situations when the registers cannot be emulated to their full >> functionality. This can be due to the complexity involved. In such cases, one >> can emulate those registers as RAZ/WI for example. We call them as partial >> emulation. >> >> Some registers are non-optional and as such there is nothing preventing an OS >> from accessing them. >> Instead of injecting undefined exception (thus crashing a guest), one may want >> to prefer a partial emulation to let the guest running (in some cases accepting >> the fact that it might result in unwanted behavior). >> >> A suitable example of this (as seen in subsequent patches) is emulation of >> DBGDTRTX_EL0 (on Arm64) and DBGDTRTXINT(on Arm32). These non-optional >> registers can be emulated as RAZ/WI and they can be enclosed within >> CONFIG_PARTIAL_EMULATION. >> >> Further, "partial-emulation" command line option allows us to >> enable/disable partial emulation at run time. While CONFIG_PARTIAL_EMULATION >> enables support for partial emulation at compile time (i.e. adds code for >> partial emulation), this option may be enabled or disabled by Yocto or other >> build systems. However if the build system turns this option on, users >> can use scripts like Imagebuilder to generate uboot-script which will append >> "partial-emulation=true" to xen command line to turn on the partial emulation. >> Thus, it helps to avoid rebuilding xen. >> >> By default, "CONFIG_PARTIAL_EMULATION=y" and "partial-emulation=false". >> This is done so that Xen supports partial emulation. However, customers are >> fully aware when they enable partial emulation. It's important to note that >> enabling such support might result in unwanted/non-spec compliant behavior. >> >> Added a note in SUPPORT.md to clarify the security support for partial >> emulation. > There may have been prior discussion of this, but I view this as too little > to justify ... > >> --- a/SUPPORT.md >> +++ b/SUPPORT.md >> @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to support MSI. >> >> Status: Experimental >> >> +### ARM/Partial Emulation >> + >> +Enable partial emulation of registers, otherwise considered unimplemented, >> +that would normally trigger a fault injection. >> + >> + Status: Supported, with caveats >> + >> +Bugs allowing the userspace to attack the guest OS will not be considered >> +security vulnerabilities. >> + >> +Bugs that could compromise Xen will be considered security vulnerabilities. > ... the odd statement regarding in-guest vulnerabilities that might be > introduced. I see only two ways of treating this as supported: Either > you simply refuse emulation when the access is from user space, I am wondering how do we enforce that. Let me try to understand this with the current implementation of partial emulation for system registers. 1. DBGDTRTX_EL0 :- I understand that EL0 access to this register will cause a trap to EL2. The reason being MDCR_EL2.TDA == 1. In that case, if we refuse emulation then an undef exception is injected to the guest (this is the behavior as of today even without this patch). So, are you saying that the undef exception is to be injected to the user space process. This may be possible for Arm64 guests (inject_undef64_exception() needs to be changed). However for Arm32 guests, this may not be possible as the mode changes to PSR_MODE_UND. Let me know if I understood you correctly. > or you > support that mode of emulation as much as that of kernel space accesses. Do you mean we support partial emulation only for traps from EL1 mode ? - Ayan > After all "injecting undefined exception (thus crashing a guest)" > (quoting the description) doesn't seem quite right: If you inject such > an exception for a user space access, all that ought to happen is the > user space process to be killed. > > Jan
On 20.02.2024 16:22, Ayan Kumar Halder wrote: > On 20/02/2024 12:33, Jan Beulich wrote: >> On 20.02.2024 13:17, Ayan Kumar Halder wrote: >>> --- a/SUPPORT.md >>> +++ b/SUPPORT.md >>> @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to support MSI. >>> >>> Status: Experimental >>> >>> +### ARM/Partial Emulation >>> + >>> +Enable partial emulation of registers, otherwise considered unimplemented, >>> +that would normally trigger a fault injection. >>> + >>> + Status: Supported, with caveats >>> + >>> +Bugs allowing the userspace to attack the guest OS will not be considered >>> +security vulnerabilities. >>> + >>> +Bugs that could compromise Xen will be considered security vulnerabilities. >> ... the odd statement regarding in-guest vulnerabilities that might be >> introduced. I see only two ways of treating this as supported: Either >> you simply refuse emulation when the access is from user space, > > I am wondering how do we enforce that. > > Let me try to understand this with the current implementation of partial > emulation for system registers. > > 1. DBGDTRTX_EL0 :- I understand that EL0 access to this register will > cause a trap to EL2. The reason being MDCR_EL2.TDA == 1. > > In that case, if we refuse emulation then an undef exception is injected > to the guest (this is the behavior as of today even without this patch). > > So, are you saying that the undef exception is to be injected to the > user space process. This may be possible for Arm64 guests > (inject_undef64_exception() needs to be changed). No, injection is always to the guest, not to a specific entity within the guest. That ought to be the same on bare hardware: An exception when raised has an architecturally defined entry point for handling. That'll typically be kernel code. The handler then figures out whether the source of the exception was in user or kernel mode. For user mode code, the kernel may or may not try to handle the exception and then continue the user mode process. If it can't or doesn't want to handle it, it'll raise (in UNIX terms) a signal to the process. That signal, in turn, may or may not be fatal to the process. But such an exception from user mode should never be fatal to the guest as a whole. > However for Arm32 guests, this may not be possible as the mode changes > to PSR_MODE_UND. I'm afraid my Arm foo isn't good enough to understand this. On the surface it looks to violate above outlined principle. > Let me know if I understood you correctly. > >> or you >> support that mode of emulation as much as that of kernel space accesses. > > Do you mean we support partial emulation only for traps from EL1 mode ? Possibly. Jan
Hi Jan, On 21/02/2024 07:09, Jan Beulich wrote: > On 20.02.2024 16:22, Ayan Kumar Halder wrote: >> On 20/02/2024 12:33, Jan Beulich wrote: >>> On 20.02.2024 13:17, Ayan Kumar Halder wrote: >>>> --- a/SUPPORT.md >>>> +++ b/SUPPORT.md >>>> @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to support MSI. >>>> >>>> Status: Experimental >>>> >>>> +### ARM/Partial Emulation >>>> + >>>> +Enable partial emulation of registers, otherwise considered unimplemented, >>>> +that would normally trigger a fault injection. >>>> + >>>> + Status: Supported, with caveats >>>> + >>>> +Bugs allowing the userspace to attack the guest OS will not be considered >>>> +security vulnerabilities. >>>> + >>>> +Bugs that could compromise Xen will be considered security vulnerabilities. >>> ... the odd statement regarding in-guest vulnerabilities that might be >>> introduced. I see only two ways of treating this as supported: Either >>> you simply refuse emulation when the access is from user space, >> I am wondering how do we enforce that. >> >> Let me try to understand this with the current implementation of partial >> emulation for system registers. >> >> 1. DBGDTRTX_EL0 :- I understand that EL0 access to this register will >> cause a trap to EL2. The reason being MDCR_EL2.TDA == 1. >> >> In that case, if we refuse emulation then an undef exception is injected >> to the guest (this is the behavior as of today even without this patch). >> >> So, are you saying that the undef exception is to be injected to the >> user space process. This may be possible for Arm64 guests >> (inject_undef64_exception() needs to be changed). > No, injection is always to the guest, not to a specific entity within the > guest. That ought to be the same on bare hardware: An exception when > raised has an architecturally defined entry point for handling. That'll > typically be kernel code. The handler then figures out whether the source > of the exception was in user or kernel mode. For user mode code, the > kernel may or may not try to handle the exception and then continue the > user mode process. If it can't or doesn't want to handle it, it'll raise > (in UNIX terms) a signal to the process. That signal, in turn, may or may > not be fatal to the process. But such an exception from user mode should > never be fatal to the guest as a whole. Thanks for explaining it so well. > >> However for Arm32 guests, this may not be possible as the mode changes >> to PSR_MODE_UND. > I'm afraid my Arm foo isn't good enough to understand this. On the surface > it looks to violate above outlined principle. > >> Let me know if I understood you correctly. >> >>> or you >>> support that mode of emulation as much as that of kernel space accesses. >> Do you mean we support partial emulation only for traps from EL1 mode ? > Possibly. Now, I understand you. We will allow partial_emulation only from kernel mode. So we need to do sth :- if ( 'partial_emulation == true' && '!regs_mode_is_user(regs)' ) { /* partial_emulation logic */ } I am ok with this. And the updates message will be ### ARM/Partial Emulation Enable partial emulation of registers, otherwise considered unimplemented, that would normally trigger a fault injection. Status: Supported, with caveats Partial emulation for traps from userspace is not allowed. Bugs that could compromise Xen will be considered security vulnerabilities. Also, want @Julien, @Michal to comment on this. - Ayan > > Jan
On 21/02/2024 11:33, Ayan Kumar Halder wrote: > Hi Jan, > > On 21/02/2024 07:09, Jan Beulich wrote: >> On 20.02.2024 16:22, Ayan Kumar Halder wrote: >>> On 20/02/2024 12:33, Jan Beulich wrote: >>>> On 20.02.2024 13:17, Ayan Kumar Halder wrote: >>>>> --- a/SUPPORT.md >>>>> +++ b/SUPPORT.md >>>>> @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to >>>>> support MSI. >>>>> Status: Experimental >>>>> +### ARM/Partial Emulation >>>>> + >>>>> +Enable partial emulation of registers, otherwise considered >>>>> unimplemented, >>>>> +that would normally trigger a fault injection. >>>>> + >>>>> + Status: Supported, with caveats >>>>> + >>>>> +Bugs allowing the userspace to attack the guest OS will not be >>>>> considered >>>>> +security vulnerabilities. >>>>> + >>>>> +Bugs that could compromise Xen will be considered security >>>>> vulnerabilities. >>>> ... the odd statement regarding in-guest vulnerabilities that might be >>>> introduced. I see only two ways of treating this as supported: Either >>>> you simply refuse emulation when the access is from user space, >>> I am wondering how do we enforce that. >>> >>> Let me try to understand this with the current implementation of partial >>> emulation for system registers. >>> >>> 1. DBGDTRTX_EL0 :- I understand that EL0 access to this register will >>> cause a trap to EL2. The reason being MDCR_EL2.TDA == 1. >>> >>> In that case, if we refuse emulation then an undef exception is injected >>> to the guest (this is the behavior as of today even without this patch). >>> >>> So, are you saying that the undef exception is to be injected to the >>> user space process. This may be possible for Arm64 guests >>> (inject_undef64_exception() needs to be changed). >> No, injection is always to the guest, not to a specific entity within the >> guest. That ought to be the same on bare hardware: An exception when >> raised has an architecturally defined entry point for handling. That'll >> typically be kernel code. The handler then figures out whether the source >> of the exception was in user or kernel mode. For user mode code, the >> kernel may or may not try to handle the exception and then continue the >> user mode process. If it can't or doesn't want to handle it, it'll raise >> (in UNIX terms) a signal to the process. That signal, in turn, may or may >> not be fatal to the process. But such an exception from user mode should >> never be fatal to the guest as a whole. > Thanks for explaining it so well. >> >>> However for Arm32 guests, this may not be possible as the mode changes >>> to PSR_MODE_UND. >> I'm afraid my Arm foo isn't good enough to understand this. On the >> surface >> it looks to violate above outlined principle. >> >>> Let me know if I understood you correctly. >>> >>>> or you >>>> support that mode of emulation as much as that of kernel space >>>> accesses. >>> Do you mean we support partial emulation only for traps from EL1 mode ? >> Possibly. > > Now, I understand you. We will allow partial_emulation only from kernel > mode. > > So we need to do sth :- > > if ( 'partial_emulation == true' && '!regs_mode_is_user(regs)' ) > > { > > /* partial_emulation logic */ > > } > > I am ok with this. The helpers take a min_el. So you can have simpler code and set the value to 1 (rather than 0 today). > > And the updates message will be > > ### ARM/Partial Emulation > > Enable partial emulation of registers, otherwise considered unimplemented, > that would normally trigger a fault injection. > > Status: Supported, with caveats > > Partial emulation for traps from userspace is not allowed. I am not sure how this helps... > > Bugs that could compromise Xen will be considered security vulnerabilities. ... you are still implying that any userland breaching the kernel will not be supported because of a bug. But, I don't see how preventing the userland to access a register will help. In theory you could have a register that can only be accessed by EL1 but has an impact to EL0. By definition, it means that we don't faithfully follow the Arm Arm and anything can happen. Which is why I wanted to exclude security support from userland and kernel. I think the current registers are low risk. But I am under the impression that you may wan to add more partial emulation. So we need to make some sort of statement that doesn't put any burden to the security team for future registers. An option would be to explicitly list the registers in SUPPORT.md. As I mentioned above, I think the registers you currently emulate are low-risk to introduce a security bug. So I would be ok to fully security support them. This would need to be re-assessed for new registers. I am open to other suggestions. Cheers,
On 21/02/2024 13:20, Julien Grall wrote: > > > On 21/02/2024 11:33, Ayan Kumar Halder wrote: >> Hi Jan, >> >> On 21/02/2024 07:09, Jan Beulich wrote: >>> On 20.02.2024 16:22, Ayan Kumar Halder wrote: >>>> On 20/02/2024 12:33, Jan Beulich wrote: >>>>> On 20.02.2024 13:17, Ayan Kumar Halder wrote: >>>>>> --- a/SUPPORT.md >>>>>> +++ b/SUPPORT.md >>>>>> @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to >>>>>> support MSI. >>>>>> Status: Experimental >>>>>> +### ARM/Partial Emulation >>>>>> + >>>>>> +Enable partial emulation of registers, otherwise considered >>>>>> unimplemented, >>>>>> +that would normally trigger a fault injection. >>>>>> + >>>>>> + Status: Supported, with caveats >>>>>> + >>>>>> +Bugs allowing the userspace to attack the guest OS will not be >>>>>> considered >>>>>> +security vulnerabilities. >>>>>> + >>>>>> +Bugs that could compromise Xen will be considered security >>>>>> vulnerabilities. >>>>> ... the odd statement regarding in-guest vulnerabilities that might be >>>>> introduced. I see only two ways of treating this as supported: Either >>>>> you simply refuse emulation when the access is from user space, >>>> I am wondering how do we enforce that. >>>> >>>> Let me try to understand this with the current implementation of partial >>>> emulation for system registers. >>>> >>>> 1. DBGDTRTX_EL0 :- I understand that EL0 access to this register will >>>> cause a trap to EL2. The reason being MDCR_EL2.TDA == 1. >>>> >>>> In that case, if we refuse emulation then an undef exception is injected >>>> to the guest (this is the behavior as of today even without this patch). >>>> >>>> So, are you saying that the undef exception is to be injected to the >>>> user space process. This may be possible for Arm64 guests >>>> (inject_undef64_exception() needs to be changed). >>> No, injection is always to the guest, not to a specific entity within the >>> guest. That ought to be the same on bare hardware: An exception when >>> raised has an architecturally defined entry point for handling. That'll >>> typically be kernel code. The handler then figures out whether the source >>> of the exception was in user or kernel mode. For user mode code, the >>> kernel may or may not try to handle the exception and then continue the >>> user mode process. If it can't or doesn't want to handle it, it'll raise >>> (in UNIX terms) a signal to the process. That signal, in turn, may or may >>> not be fatal to the process. But such an exception from user mode should >>> never be fatal to the guest as a whole. >> Thanks for explaining it so well. >>> >>>> However for Arm32 guests, this may not be possible as the mode changes >>>> to PSR_MODE_UND. >>> I'm afraid my Arm foo isn't good enough to understand this. On the >>> surface >>> it looks to violate above outlined principle. >>> >>>> Let me know if I understood you correctly. >>>> >>>>> or you >>>>> support that mode of emulation as much as that of kernel space >>>>> accesses. >>>> Do you mean we support partial emulation only for traps from EL1 mode ? >>> Possibly. >> >> Now, I understand you. We will allow partial_emulation only from kernel >> mode. >> >> So we need to do sth :- >> >> if ( 'partial_emulation == true' && '!regs_mode_is_user(regs)' ) >> >> { >> >> /* partial_emulation logic */ >> >> } >> >> I am ok with this. > > The helpers take a min_el. So you can have simpler code and set the > value to 1 (rather than 0 today). > >> >> And the updates message will be >> >> ### ARM/Partial Emulation >> >> Enable partial emulation of registers, otherwise considered unimplemented, >> that would normally trigger a fault injection. >> >> Status: Supported, with caveats >> >> Partial emulation for traps from userspace is not allowed. > > I am not sure how this helps... >> >> Bugs that could compromise Xen will be considered security vulnerabilities. > > ... you are still implying that any userland breaching the kernel will > not be supported because of a bug. > > But, I don't see how preventing the userland to access a register will > help. In theory you could have a register that can only be accessed by > EL1 but has an impact to EL0. > > By definition, it means that we don't faithfully follow the Arm Arm and > anything can happen. Which is why I wanted to exclude security support > from userland and kernel. > > I think the current registers are low risk. But I am under the > impression that you may wan to add more partial emulation. So we need to > make some sort of statement that doesn't put any burden to the security > team for future registers. > > An option would be to explicitly list the registers in SUPPORT.md. As I > mentioned above, I think the registers you currently emulate are > low-risk to introduce a security bug. So I would be ok to fully security > support them. This would need to be re-assessed for new registers. > > I am open to other suggestions. +1. Providing partial emulation support only for kernel mode where the Arm ARM allows for user mode access is confusing and would require excessive number of comments to back up the implementation. While it might be ok for DCC, imposing such limit on all the future use cases sounds too severe. I think the following would be ok: Status: Supported, with caveats Only the following system registers are security supported: - ... ~Michal
diff --git a/SUPPORT.md b/SUPPORT.md index a90d1108c9..037578b10c 100644 --- a/SUPPORT.md +++ b/SUPPORT.md @@ -101,6 +101,18 @@ Extension to the GICv3 interrupt controller to support MSI. Status: Experimental +### ARM/Partial Emulation + +Enable partial emulation of registers, otherwise considered unimplemented, +that would normally trigger a fault injection. + + Status: Supported, with caveats + +Bugs allowing the userspace to attack the guest OS will not be considered +security vulnerabilities. + +Bugs that could compromise Xen will be considered security vulnerabilities. + ### ARM Scalable Vector Extension (SVE/SVE2) Arm64 domains can use Scalable Vector Extension (SVE/SVE2). diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index be76be8d53..0cac601066 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -1942,6 +1942,17 @@ This option is ignored in **pv-shim** mode. > Default: `on` +### partial-emulation (arm) +> `= <boolean>` + +> Default: `false` + +Flag to enable or disable partial emulation of system/coprocessor registers. +Only effective if CONFIG_PARTIAL_EMULATION is enabled. + +**WARNING: Enabling this option might result in unwanted/non-spec compliant +behavior.** + ### pci = List of [ serr=<bool>, perr=<bool> ] diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 50e9bfae1a..dc243f4124 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -225,6 +225,15 @@ config STATIC_EVTCHN This option enables establishing static event channel communication between domains on a dom0less system (domU-domU as well as domU-dom0). +config PARTIAL_EMULATION + bool "Enable partial emulation of system/coprocessor registers" + default y + help + This option enables partial emulation of registers to prevent guests + crashing when accessing registers which are not optional but have not been + emulated to their complete functionality. Enabling this might result in + unwanted/non-spec compliant behavior. + endmenu menu "ARM errata workaround via the alternative framework" diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h index 883dae368e..9a60dbf70e 100644 --- a/xen/arch/arm/include/asm/traps.h +++ b/xen/arch/arm/include/asm/traps.h @@ -10,6 +10,12 @@ # include <asm/arm64/traps.h> #endif +#ifdef CONFIG_PARTIAL_EMULATION +extern bool partial_emulation; +#else +#define partial_emulation false +#endif + /* * GUEST_BUG_ON is intended for checking that the guest state has not been * corrupted in hardware and/or that the hardware behaves as we diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 9c10e8f78c..d1c7a6c516 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -42,6 +42,15 @@ #include <asm/vgic.h> #include <asm/vtimer.h> +/* + * partial_emulation: If true, partial emulation for system/coprocessor + * registers will be enabled. + */ +#ifdef CONFIG_PARTIAL_EMULATION +bool __ro_after_init partial_emulation = false; +boolean_param("partial-emulation", partial_emulation); +#endif + /* The base of the stack must always be double-word aligned, which means * that both the kernel half of struct cpu_user_regs (which is pushed in * entry.S) and struct cpu_info (which lives at the bottom of a Xen