| Message ID | 54005c49-b012-8265-246e-22b03a87f724@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,for-4.18?] x86: support data operand independent timing mode | expand |
Hi Jan, > On Sep 11, 2023, at 23:01, Jan Beulich <jbeulich@suse.com> wrote: > > [1] specifies a long list of instructions which are intended to exhibit > timing behavior independent of the data they operate on. On certain > hardware this independence is optional, controlled by a bit in a new > MSR. Provide a command line option to control the mode Xen and its > guests are to operate in, with a build time control over the default. > Longer term we may want to allow guests to control this. > > Since Arm64 supposedly also has such a control, put command line option > and Kconfig control in common files. > > [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html > > Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > This may be viewed as a new feature, and hence be too late for 4.18. It > may, however, also be viewed as security relevant, which is why I'd like > to propose to at least consider it. Fine with me if this patch can be properly reviewed on time, because of the security relevance. > > Slightly RFC, in particular for whether the Kconfig option should > default to Y or N. > > I would have wanted to invoke setup_doitm() from cpu_init(), but that > works only on the BSP. On APs cpu_init() runs before ucode loading. > Plus recheck_cpu_features() invoking identify_cpu() takes care of the > BSP during S3 resume. > --- > v2: Introduce and use cpu_has_doitm. Add comment "borrowed" from the > XenServer patch queue patch providing similar functionality. > Re-base. > > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -788,6 +788,14 @@ Specify the size of the console debug tr > additionally a trace buffer of the specified size is allocated per cpu. > The debug trace feature is only enabled in debugging builds of Xen. > > +### dit (x86) > +> `= <boolean>` > + > +> Default: `CONFIG_DIT_DEFAULT` > + > +Specify whether Xen and guests should operate in Data Independent Timing > +mode. > + Since a new cmdline interface is added, I am wondering would such addtion deserves a CHANGELOG entry? Kind regards, Henry
On 12.09.2023 02:48, Henry Wang wrote: >> On Sep 11, 2023, at 23:01, Jan Beulich <jbeulich@suse.com> wrote: >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -788,6 +788,14 @@ Specify the size of the console debug tr >> additionally a trace buffer of the specified size is allocated per cpu. >> The debug trace feature is only enabled in debugging builds of Xen. >> >> +### dit (x86) >> +> `= <boolean>` >> + >> +> Default: `CONFIG_DIT_DEFAULT` >> + >> +Specify whether Xen and guests should operate in Data Independent Timing >> +mode. >> + > > Since a new cmdline interface is added, I am wondering would such > addtion deserves a CHANGELOG entry? I'm open to opinions, albeit not so much because of the added command line option. Jan
Hi, On 12/09/2023 07:21, Jan Beulich wrote: > On 12.09.2023 02:48, Henry Wang wrote: >>> On Sep 11, 2023, at 23:01, Jan Beulich <jbeulich@suse.com> wrote: >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -788,6 +788,14 @@ Specify the size of the console debug tr >>> additionally a trace buffer of the specified size is allocated per cpu. >>> The debug trace feature is only enabled in debugging builds of Xen. >>> >>> +### dit (x86) >>> +> `= <boolean>` >>> + >>> +> Default: `CONFIG_DIT_DEFAULT` >>> + >>> +Specify whether Xen and guests should operate in Data Independent Timing >>> +mode. >>> + >> >> Since a new cmdline interface is added, I am wondering would such >> addtion deserves a CHANGELOG entry? > > I'm open to opinions, albeit not so much because of the added command line > option. I think it would make sense to add an entry to say the user can now decide the operation mode for DIT. Cheers,
Hi, On 11/09/2023 16:01, Jan Beulich wrote: > [1] specifies a long list of instructions which are intended to exhibit > timing behavior independent of the data they operate on. On certain > hardware this independence is optional, controlled by a bit in a new > MSR. Provide a command line option to control the mode Xen and its > guests are to operate in, with a build time control over the default. > Longer term we may want to allow guests to control this. If I read correctly the discussion on the previous version. There was some concern with this version of patch. I can't find anything public suggesting that the concerned were dismissed. Do you have any pointer? > Since Arm64 supposedly also has such a control, put command line option > and Kconfig control in common files. > > [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html > > Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > This may be viewed as a new feature, and hence be too late for 4.18. It > may, however, also be viewed as security relevant, which is why I'd like > to propose to at least consider it. > > Slightly RFC, in particular for whether the Kconfig option should > default to Y or N. I am not entirely sure. I understand that DIT will help in the cryptographic case but it is not clear to me what is the performance impact. > > I would have wanted to invoke setup_doitm() from cpu_init(), but that > works only on the BSP. On APs cpu_init() runs before ucode loading. > Plus recheck_cpu_features() invoking identify_cpu() takes care of the > BSP during S3 resume. > --- > v2: Introduce and use cpu_has_doitm. Add comment "borrowed" from the > XenServer patch queue patch providing similar functionality. > Re-base. > > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -788,6 +788,14 @@ Specify the size of the console debug tr > additionally a trace buffer of the specified size is allocated per cpu. > The debug trace feature is only enabled in debugging builds of Xen. > > +### dit (x86) > +> `= <boolean>` > + > +> Default: `CONFIG_DIT_DEFAULT` > + > +Specify whether Xen and guests should operate in Data Independent Timing > +mode. > + > ### dma_bits > > `= <integer>` > > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -15,6 +15,7 @@ config X86 > select HAS_ALTERNATIVE > select HAS_COMPAT > select HAS_CPUFREQ > + select HAS_DIT > select HAS_EHCI > select HAS_EX_TABLE > select HAS_FAST_MULTIPLY > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct > alternative_vcall(ctxt_switch_masking, next); > } > > +static void setup_doitm(void) > +{ > + uint64_t msr; > + > + if ( !cpu_has_doitm ) I am not very familiar with the feature. If it is not present, then can we guarantee that the instructions will be executed in constant time? If not, I think we should taint Xen and/or print a message if the admin requested to use DIT. This would make clear that the admin is trying to do something that doesn't work. > + return; > + > + /* > + * We don't currently enumerate DOITM to guests. As a conseqeuence, guest > + * kernels will believe they're safe even when they are not. > + * > + * For now, set it unilaterally. This prevents otherwise-correct crypto > + * code from becoming vulnerable to timing sidechannels. > + */ > + > + rdmsrl(MSR_UARCH_MISC_CTRL, msr); > + msr |= UARCH_CTRL_DOITM; > + if ( !opt_dit ) > + msr &= ~UARCH_CTRL_DOITM; > + wrmsrl(MSR_UARCH_MISC_CTRL, msr); > +} > + > bool opt_cpu_info; > boolean_param("cpuinfo", opt_cpu_info); > > @@ -589,6 +611,8 @@ void identify_cpu(struct cpuinfo_x86 *c) > > mtrr_bp_init(); > } > + > + setup_doitm(); > } > > /* leaf 0xb SMT level */ > --- a/xen/arch/x86/include/asm/cpufeature.h > +++ b/xen/arch/x86/include/asm/cpufeature.h > @@ -201,6 +201,7 @@ static inline bool boot_cpu_has(unsigned > #define cpu_has_if_pschange_mc_no boot_cpu_has(X86_FEATURE_IF_PSCHANGE_MC_NO) > #define cpu_has_tsx_ctrl boot_cpu_has(X86_FEATURE_TSX_CTRL) > #define cpu_has_taa_no boot_cpu_has(X86_FEATURE_TAA_NO) > +#define cpu_has_doitm boot_cpu_has(X86_FEATURE_DOITM) > #define cpu_has_fb_clear boot_cpu_has(X86_FEATURE_FB_CLEAR) > #define cpu_has_rrsba boot_cpu_has(X86_FEATURE_RRSBA) > #define cpu_has_gds_ctrl boot_cpu_has(X86_FEATURE_GDS_CTRL) > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -41,6 +41,9 @@ config HAS_COMPAT > config HAS_DEVICE_TREE > bool > > +config HAS_DIT # Data Independent Timing > + bool > + > config HAS_EX_TABLE > bool > > @@ -175,6 +178,18 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS > > endmenu > > +config DIT_DEFAULT > + bool "Data Independent Timing default" > + depends on HAS_DIT > + help > + Hardware often surfaces instructions the timing of which is dependent > + on the data they process. Some of these instructions may be used in > + timing sensitive environments, e.g. cryptography. When such > + instructions exist, hardware may further surface a control allowing > + to make the behavior of such instructions independent of the data > + they act upon. Choose the default here for when no "dit" command line > + option is present. > + > config HYPFS > bool "Hypervisor file system support" > default y > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -28,6 +28,11 @@ CHECK_feature_info; > > enum system_state system_state = SYS_STATE_early_boot; > > +#ifdef CONFIG_HAS_DIT > +bool __ro_after_init opt_dit = IS_ENABLED(CONFIG_DIT_DEFAULT); > +boolean_param("dit", opt_dit); > +#endif > + > static xen_commandline_t saved_cmdline; > static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; > > --- a/xen/include/xen/param.h > +++ b/xen/include/xen/param.h > @@ -184,6 +184,8 @@ extern struct param_hypfs __paramhypfs_s > string_param(_name, _var); \ > string_runtime_only_param(_name, _var) > > +extern bool opt_dit; > + > static inline void no_config_param(const char *cfg, const char *param, > const char *s, const char *e) > { Cheers, [1] https://lore.kernel.org/all/8f07c532-e742-fa02-27ee-b08c56299d09@citrix.com/
On 13.09.2023 19:56, Julien Grall wrote: > On 11/09/2023 16:01, Jan Beulich wrote: >> [1] specifies a long list of instructions which are intended to exhibit >> timing behavior independent of the data they operate on. On certain >> hardware this independence is optional, controlled by a bit in a new >> MSR. Provide a command line option to control the mode Xen and its >> guests are to operate in, with a build time control over the default. >> Longer term we may want to allow guests to control this. > > If I read correctly the discussion on the previous version. There was > some concern with this version of patch. I can't find anything public > suggesting that the concerned were dismissed. Do you have any pointer? Well, I can point to the XenServer patch queue, which since then has gained a patch of similar (less flexible) functionality (and seeing the similarity I was puzzled by this patch, which was posted late for 4.17, not having gone in quite some time ago). This to me demonstrates that the original concerns were "downgraded". It would of course still be desirable to have guests control the behavior for themselves, but that's a more intrusive change left for the future. Beyond that I guess I need to have Andrew speak for himself. >> Since Arm64 supposedly also has such a control, put command line option >> and Kconfig control in common files. >> >> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html >> >> Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> This may be viewed as a new feature, and hence be too late for 4.18. It >> may, however, also be viewed as security relevant, which is why I'd like >> to propose to at least consider it. >> >> Slightly RFC, in particular for whether the Kconfig option should >> default to Y or N. > > I am not entirely sure. I understand that DIT will help in the > cryptographic case but it is not clear to me what is the performance impact. The entire purpose of the bit is to have a performance impact, slowing down execution when fastpaths could be taken, but shouldn't to achieve the named goal. I have no numbers though, and like you I can only go from what the referenced documentation says. >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct >> alternative_vcall(ctxt_switch_masking, next); >> } >> >> +static void setup_doitm(void) >> +{ >> + uint64_t msr; >> + >> + if ( !cpu_has_doitm ) > > I am not very familiar with the feature. If it is not present, then can > we guarantee that the instructions will be executed in constant time? _We_ cannot guarantee anything. It is only hardware vendors who can. As a result I can only again refer you to the referenced documentation. It claims that without the bit being supported in hardware, an extensive list of insns is going to expose data operand independent performance. > If not, I think we should taint Xen and/or print a message if the admin > requested to use DIT. This would make clear that the admin is trying to > do something that doesn't work. Tainting Xen on all hardware not exposing the bit would seem entirely unreasonable to me. If we learned of specific cases where the vendor promises are broken, we could taint there, sure. I guess that would be individual XSAs / CVEs then for each instance. Jan
Hi Jan, On 14/09/2023 07:32, Jan Beulich wrote: > On 13.09.2023 19:56, Julien Grall wrote: >> On 11/09/2023 16:01, Jan Beulich wrote: >>> [1] specifies a long list of instructions which are intended to exhibit >>> timing behavior independent of the data they operate on. On certain >>> hardware this independence is optional, controlled by a bit in a new >>> MSR. Provide a command line option to control the mode Xen and its >>> guests are to operate in, with a build time control over the default. >>> Longer term we may want to allow guests to control this. >> >> If I read correctly the discussion on the previous version. There was >> some concern with this version of patch. I can't find anything public >> suggesting that the concerned were dismissed. Do you have any pointer? > > Well, I can point to the XenServer patch queue, which since then has > gained a patch of similar (less flexible) functionality (and seeing > the similarity I was puzzled by this patch, which was posted late > for 4.17, not having gone in quite some time ago). This to me > demonstrates that the original concerns were "downgraded". It would > of course still be desirable to have guests control the behavior for > themselves, but that's a more intrusive change left for the future. > > Beyond that I guess I need to have Andrew speak for himself. > >>> Since Arm64 supposedly also has such a control, put command line option >>> and Kconfig control in common files. >>> >>> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html >>> >>> Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> This may be viewed as a new feature, and hence be too late for 4.18. It >>> may, however, also be viewed as security relevant, which is why I'd like >>> to propose to at least consider it. >>> >>> Slightly RFC, in particular for whether the Kconfig option should >>> default to Y or N. >> >> I am not entirely sure. I understand that DIT will help in the >> cryptographic case but it is not clear to me what is the performance impact. > > The entire purpose of the bit is to have a performance impact, slowing > down execution when fastpaths could be taken, but shouldn't to achieve > the named goal. I understood that. My question was more related to how much it will degrade the performance. This will help to know whether the default should be yes or no. >>> --- a/xen/arch/x86/cpu/common.c >>> +++ b/xen/arch/x86/cpu/common.c >>> @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct >>> alternative_vcall(ctxt_switch_masking, next); >>> } >>> >>> +static void setup_doitm(void) >>> +{ >>> + uint64_t msr; >>> + >>> + if ( !cpu_has_doitm ) >> >> I am not very familiar with the feature. If it is not present, then can >> we guarantee that the instructions will be executed in constant time? > > _We_ cannot guarantee anything. It is only hardware vendors who can. As a > result I can only again refer you to the referenced documentation. It > claims that without the bit being supported in hardware, an extensive > list of insns is going to expose data operand independent performance. Right. So ... > >> If not, I think we should taint Xen and/or print a message if the admin >> requested to use DIT. This would make clear that the admin is trying to >> do something that doesn't work. > > Tainting Xen on all hardware not exposing the bit would seem entirely > unreasonable to me. If we learned of specific cases where the vendor > promises are broken, we could taint there, sure. I guess that would be > individual XSAs / CVEs then for each instance. ... we need to somehow let the user know that the HW it is running on may not honor DIT. Tainting might be a bit too harsh, but I think printing a message with warning_add() is necessary. Cheers,
On 14.09.2023 11:04, Julien Grall wrote: > On 14/09/2023 07:32, Jan Beulich wrote: >> On 13.09.2023 19:56, Julien Grall wrote: >>> On 11/09/2023 16:01, Jan Beulich wrote: >>>> [1] specifies a long list of instructions which are intended to exhibit >>>> timing behavior independent of the data they operate on. On certain >>>> hardware this independence is optional, controlled by a bit in a new >>>> MSR. Provide a command line option to control the mode Xen and its >>>> guests are to operate in, with a build time control over the default. >>>> Longer term we may want to allow guests to control this. >>> >>> If I read correctly the discussion on the previous version. There was >>> some concern with this version of patch. I can't find anything public >>> suggesting that the concerned were dismissed. Do you have any pointer? >> >> Well, I can point to the XenServer patch queue, which since then has >> gained a patch of similar (less flexible) functionality (and seeing >> the similarity I was puzzled by this patch, which was posted late >> for 4.17, not having gone in quite some time ago). This to me >> demonstrates that the original concerns were "downgraded". It would >> of course still be desirable to have guests control the behavior for >> themselves, but that's a more intrusive change left for the future. >> >> Beyond that I guess I need to have Andrew speak for himself. >> >>>> Since Arm64 supposedly also has such a control, put command line option >>>> and Kconfig control in common files. >>>> >>>> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html >>>> >>>> Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> This may be viewed as a new feature, and hence be too late for 4.18. It >>>> may, however, also be viewed as security relevant, which is why I'd like >>>> to propose to at least consider it. >>>> >>>> Slightly RFC, in particular for whether the Kconfig option should >>>> default to Y or N. >>> >>> I am not entirely sure. I understand that DIT will help in the >>> cryptographic case but it is not clear to me what is the performance impact. >> >> The entire purpose of the bit is to have a performance impact, slowing >> down execution when fastpaths could be taken, but shouldn't to achieve >> the named goal. > > I understood that. My question was more related to how much it will > degrade the performance. This will help to know whether the default > should be yes or no. Well, as said - I have no information beyond that to be found at the provided URL. >>>> --- a/xen/arch/x86/cpu/common.c >>>> +++ b/xen/arch/x86/cpu/common.c >>>> @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct >>>> alternative_vcall(ctxt_switch_masking, next); >>>> } >>>> >>>> +static void setup_doitm(void) >>>> +{ >>>> + uint64_t msr; >>>> + >>>> + if ( !cpu_has_doitm ) >>> >>> I am not very familiar with the feature. If it is not present, then can >>> we guarantee that the instructions will be executed in constant time? >> >> _We_ cannot guarantee anything. It is only hardware vendors who can. As a >> result I can only again refer you to the referenced documentation. It >> claims that without the bit being supported in hardware, an extensive >> list of insns is going to expose data operand independent performance. > > Right. So ... > >> >>> If not, I think we should taint Xen and/or print a message if the admin >>> requested to use DIT. This would make clear that the admin is trying to >>> do something that doesn't work. >> >> Tainting Xen on all hardware not exposing the bit would seem entirely >> unreasonable to me. If we learned of specific cases where the vendor >> promises are broken, we could taint there, sure. I guess that would be >> individual XSAs / CVEs then for each instance. > > ... we need to somehow let the user know that the HW it is running on > may not honor DIT. Tainting might be a bit too harsh, but I think > printing a > message with warning_add() is necessary. But Intel's claim is that with the bit unavailable hardware behaves as if DIT was in effect. Therefore you're still suggesting to emit a warning on most of Intel's hardware and on all non-Intel one. That's going too far, imo. Jan
Hi, On 14/09/2023 10:11, Jan Beulich wrote: > On 14.09.2023 11:04, Julien Grall wrote: >> On 14/09/2023 07:32, Jan Beulich wrote: >>> On 13.09.2023 19:56, Julien Grall wrote: >>>> On 11/09/2023 16:01, Jan Beulich wrote: >>>>> [1] specifies a long list of instructions which are intended to exhibit >>>>> timing behavior independent of the data they operate on. On certain >>>>> hardware this independence is optional, controlled by a bit in a new >>>>> MSR. Provide a command line option to control the mode Xen and its >>>>> guests are to operate in, with a build time control over the default. >>>>> Longer term we may want to allow guests to control this. >>>> >>>> If I read correctly the discussion on the previous version. There was >>>> some concern with this version of patch. I can't find anything public >>>> suggesting that the concerned were dismissed. Do you have any pointer? >>> >>> Well, I can point to the XenServer patch queue, which since then has >>> gained a patch of similar (less flexible) functionality (and seeing >>> the similarity I was puzzled by this patch, which was posted late >>> for 4.17, not having gone in quite some time ago). This to me >>> demonstrates that the original concerns were "downgraded". It would >>> of course still be desirable to have guests control the behavior for >>> themselves, but that's a more intrusive change left for the future. >>> >>> Beyond that I guess I need to have Andrew speak for himself. >>> >>>>> Since Arm64 supposedly also has such a control, put command line option >>>>> and Kconfig control in common files. >>>>> >>>>> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html >>>>> >>>>> Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>> --- >>>>> This may be viewed as a new feature, and hence be too late for 4.18. It >>>>> may, however, also be viewed as security relevant, which is why I'd like >>>>> to propose to at least consider it. >>>>> >>>>> Slightly RFC, in particular for whether the Kconfig option should >>>>> default to Y or N. >>>> >>>> I am not entirely sure. I understand that DIT will help in the >>>> cryptographic case but it is not clear to me what is the performance impact. >>> >>> The entire purpose of the bit is to have a performance impact, slowing >>> down execution when fastpaths could be taken, but shouldn't to achieve >>> the named goal. >> >> I understood that. My question was more related to how much it will >> degrade the performance. This will help to know whether the default >> should be yes or no. > > Well, as said - I have no information beyond that to be found at the > provided URL. > >>>>> --- a/xen/arch/x86/cpu/common.c >>>>> +++ b/xen/arch/x86/cpu/common.c >>>>> @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct >>>>> alternative_vcall(ctxt_switch_masking, next); >>>>> } >>>>> >>>>> +static void setup_doitm(void) >>>>> +{ >>>>> + uint64_t msr; >>>>> + >>>>> + if ( !cpu_has_doitm ) >>>> >>>> I am not very familiar with the feature. If it is not present, then can >>>> we guarantee that the instructions will be executed in constant time? >>> >>> _We_ cannot guarantee anything. It is only hardware vendors who can. As a >>> result I can only again refer you to the referenced documentation. It >>> claims that without the bit being supported in hardware, an extensive >>> list of insns is going to expose data operand independent performance. >> >> Right. So ... >> >>> >>>> If not, I think we should taint Xen and/or print a message if the admin >>>> requested to use DIT. This would make clear that the admin is trying to >>>> do something that doesn't work. >>> >>> Tainting Xen on all hardware not exposing the bit would seem entirely >>> unreasonable to me. If we learned of specific cases where the vendor >>> promises are broken, we could taint there, sure. I guess that would be >>> individual XSAs / CVEs then for each instance. >> >> ... we need to somehow let the user know that the HW it is running on >> may not honor DIT. Tainting might be a bit too harsh, but I think >> printing a >> message with warning_add() is necessary. > > But Intel's claim is that with the bit unavailable hardware behaves as > if DIT was in effect. I am confused. Above, you suggested it cannot be guaranteed. So I interpreted we don't know what's happening on any processor. So where you referring to... Therefore you're still suggesting to emit a > warning on most of Intel's hardware and on all non-Intel one. ... non-Intel HW? > That's > going too far, imo. We could restrict the warning to non-Intel platform. > Jan
On 14.09.2023 11:18, Julien Grall wrote: > On 14/09/2023 10:11, Jan Beulich wrote: >> On 14.09.2023 11:04, Julien Grall wrote: >>> On 14/09/2023 07:32, Jan Beulich wrote: >>>> On 13.09.2023 19:56, Julien Grall wrote: >>>>> If not, I think we should taint Xen and/or print a message if the admin >>>>> requested to use DIT. This would make clear that the admin is trying to >>>>> do something that doesn't work. >>>> >>>> Tainting Xen on all hardware not exposing the bit would seem entirely >>>> unreasonable to me. If we learned of specific cases where the vendor >>>> promises are broken, we could taint there, sure. I guess that would be >>>> individual XSAs / CVEs then for each instance. >>> >>> ... we need to somehow let the user know that the HW it is running on >>> may not honor DIT. Tainting might be a bit too harsh, but I think >>> printing a >>> message with warning_add() is necessary. >> >> But Intel's claim is that with the bit unavailable hardware behaves as >> if DIT was in effect. > > I am confused. Above, you suggested it cannot be guaranteed. So I > interpreted we don't know what's happening on any processor. Right. We can trust vendors, or not. > So where > you referring to... > > > Therefore you're still suggesting to emit a >> warning on most of Intel's hardware and on all non-Intel one. > > ... non-Intel HW? > >> That's >> going too far, imo. > > We could restrict the warning to non-Intel platform. That still goes too far imo. I'm not convinced we should cover for vendor uncertainty here. We can't make a system any safer than its hardware is, in this regard. We simply have no (or not enough) influence on the internal workings of their pipelines. What I have done is add a sentence to the command line option's documentation: "Note that enabling this option cannot guarantee anything beyond what underlying hardware guarantees (with, where available and known to Xen, respective tweaks applied)." Plus I've further qualified the option: ### dit (x86/Intel) Jan
On Thu, Sep 14, 2023 at 10:04:31AM +0100, Julien Grall wrote: > Hi Jan, > > On 14/09/2023 07:32, Jan Beulich wrote: > > On 13.09.2023 19:56, Julien Grall wrote: > > > On 11/09/2023 16:01, Jan Beulich wrote: > > > > [1] specifies a long list of instructions which are intended to exhibit > > > > timing behavior independent of the data they operate on. On certain > > > > hardware this independence is optional, controlled by a bit in a new > > > > MSR. Provide a command line option to control the mode Xen and its > > > > guests are to operate in, with a build time control over the default. > > > > Longer term we may want to allow guests to control this. > > > > > > If I read correctly the discussion on the previous version. There was > > > some concern with this version of patch. I can't find anything public > > > suggesting that the concerned were dismissed. Do you have any pointer? > > > > Well, I can point to the XenServer patch queue, which since then has > > gained a patch of similar (less flexible) functionality (and seeing > > the similarity I was puzzled by this patch, which was posted late > > for 4.17, not having gone in quite some time ago). This to me > > demonstrates that the original concerns were "downgraded". It would > > of course still be desirable to have guests control the behavior for > > themselves, but that's a more intrusive change left for the future. > > > > Beyond that I guess I need to have Andrew speak for himself. > > > > > > Since Arm64 supposedly also has such a control, put command line option > > > > and Kconfig control in common files. > > > > > > > > [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html > > > > > > > > Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > --- > > > > This may be viewed as a new feature, and hence be too late for 4.18. It > > > > may, however, also be viewed as security relevant, which is why I'd like > > > > to propose to at least consider it. > > > > > > > > Slightly RFC, in particular for whether the Kconfig option should > > > > default to Y or N. > > > > > > I am not entirely sure. I understand that DIT will help in the > > > cryptographic case but it is not clear to me what is the performance impact. > > > > The entire purpose of the bit is to have a performance impact, slowing > > down execution when fastpaths could be taken, but shouldn't to achieve > > the named goal. > > I understood that. My question was more related to how much it will degrade > the performance. This will help to know whether the default should be yes or > no. This information is not available. You could do benchmarks with and without this patch if you wanted to obtain it. > > > > --- a/xen/arch/x86/cpu/common.c > > > > +++ b/xen/arch/x86/cpu/common.c > > > > @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct > > > > alternative_vcall(ctxt_switch_masking, next); > > > > } > > > > +static void setup_doitm(void) > > > > +{ > > > > + uint64_t msr; > > > > + > > > > + if ( !cpu_has_doitm ) > > > > > > I am not very familiar with the feature. If it is not present, then can > > > we guarantee that the instructions will be executed in constant time? > > > > _We_ cannot guarantee anything. It is only hardware vendors who can. As a > > result I can only again refer you to the referenced documentation. It > > claims that without the bit being supported in hardware, an extensive > > list of insns is going to expose data operand independent performance. > > Right. So ... > > > > > > If not, I think we should taint Xen and/or print a message if the admin > > > requested to use DIT. This would make clear that the admin is trying to > > > do something that doesn't work. > > > > Tainting Xen on all hardware not exposing the bit would seem entirely > > unreasonable to me. If we learned of specific cases where the vendor > > promises are broken, we could taint there, sure. I guess that would be > > individual XSAs / CVEs then for each instance. > > ... we need to somehow let the user know that the HW it is running on may > not honor DIT. Tainting might be a bit too harsh, but I think printing a > message with warning_add() is necessary. Prior to DOITM, the data-operand-independent timing of certain instructions was an implicit contract between hardware and software. DOITM made several changes: 1. The implicit contract has been replaced by an architectural guarantee. This guarantee has a specific list of instructions to which it applies, as well as a specific list of operands that timing will be independent of. The guarantee has also been made conditional on DOITM being enabled — if it is disabled, no guarantees are made. 2. Intel has retroactively extended this guarantee to all previous CPU architectures, including ones that do not give software control over DOITM. 3. Intel has stated that on architectures that do not expose DOITM control to software, DOITM is always enabled and cannot be disabled. 4. Intel has stated that on newer architectures (ones that _do_ expose DOITM), _DOITM is now disabled by default_. This is a _backwards-incompatible change to the Intel 64 architecture_. Software (including user-mode software) that was correct on older hardware is now vulnerable on newer hardware if DOITM is disabled, which is the default. To fix this software, DOITM must be enabled by systems software. Conditionally enabling DOITM is not practical. Modifying the DOITM state requires an MSR write, which is a privileged instruction. Executing a system call on entry and exit to cryptographic code has a prohibitive performance penalty and requires changing not only every single cryptographic library on the system, but also other libraries (such as libcryptsetup) that rely on constant-time handling of secrets. Therefore, enabling DOITM unconditionally and permanently is the only feasible mitigation for this problem.
Hi Jan, On 14/09/2023 12:01, Jan Beulich wrote: > On 14.09.2023 11:18, Julien Grall wrote: >> On 14/09/2023 10:11, Jan Beulich wrote: >>> On 14.09.2023 11:04, Julien Grall wrote: >>>> On 14/09/2023 07:32, Jan Beulich wrote: >>>>> On 13.09.2023 19:56, Julien Grall wrote: >>>>>> If not, I think we should taint Xen and/or print a message if the admin >>>>>> requested to use DIT. This would make clear that the admin is trying to >>>>>> do something that doesn't work. >>>>> >>>>> Tainting Xen on all hardware not exposing the bit would seem entirely >>>>> unreasonable to me. If we learned of specific cases where the vendor >>>>> promises are broken, we could taint there, sure. I guess that would be >>>>> individual XSAs / CVEs then for each instance. >>>> >>>> ... we need to somehow let the user know that the HW it is running on >>>> may not honor DIT. Tainting might be a bit too harsh, but I think >>>> printing a >>>> message with warning_add() is necessary. >>> >>> But Intel's claim is that with the bit unavailable hardware behaves as >>> if DIT was in effect. >> >> I am confused. Above, you suggested it cannot be guaranteed. So I >> interpreted we don't know what's happening on any processor. > > Right. We can trust vendors, or not. > >> So where >> you referring to... >> >> >> Therefore you're still suggesting to emit a >>> warning on most of Intel's hardware and on all non-Intel one. >> >> ... non-Intel HW? >> >>> That's >>> going too far, imo. >> >> We could restrict the warning to non-Intel platform. > > That still goes too far imo. I'm not convinced we should cover for > vendor uncertainty here. We can't make a system any safer than its > hardware is, in this regard. We simply have no (or not enough) > influence on the internal workings of their pipelines. Fair enough. I would still prefer a message in the log but ... > > What I have done is add a sentence to the command line option's > documentation: > > "Note that enabling this option cannot guarantee anything beyond what > underlying hardware guarantees (with, where available and known to Xen, > respective tweaks applied)." > > Plus I've further qualified the option: > > ### dit (x86/Intel) ... I am also happy with these two changes. Cheers,
Hi Jan, > On Sep 12, 2023, at 08:48, Henry Wang <Henry.Wang@arm.com> wrote: > > Hi Jan, > >> On Sep 11, 2023, at 23:01, Jan Beulich <jbeulich@suse.com> wrote: >> >> [1] specifies a long list of instructions which are intended to exhibit >> timing behavior independent of the data they operate on. On certain >> hardware this independence is optional, controlled by a bit in a new >> MSR. Provide a command line option to control the mode Xen and its >> guests are to operate in, with a build time control over the default. >> Longer term we may want to allow guests to control this. >> >> Since Arm64 supposedly also has such a control, put command line option >> and Kconfig control in common files. >> >> [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html >> >> Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> This may be viewed as a new feature, and hence be too late for 4.18. It >> may, however, also be viewed as security relevant, which is why I'd like >> to propose to at least consider it. > > Fine with me if this patch can be properly reviewed on time, because of > the security relevance. Based on this, if this patch can be properly reviewed before we release 4.18, please feel free to add: Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
--- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -788,6 +788,14 @@ Specify the size of the console debug tr additionally a trace buffer of the specified size is allocated per cpu. The debug trace feature is only enabled in debugging builds of Xen. +### dit (x86) +> `= <boolean>` + +> Default: `CONFIG_DIT_DEFAULT` + +Specify whether Xen and guests should operate in Data Independent Timing +mode. + ### dma_bits > `= <integer>` --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -15,6 +15,7 @@ config X86 select HAS_ALTERNATIVE select HAS_COMPAT select HAS_CPUFREQ + select HAS_DIT select HAS_EHCI select HAS_EX_TABLE select HAS_FAST_MULTIPLY --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -204,6 +204,28 @@ void ctxt_switch_levelling(const struct alternative_vcall(ctxt_switch_masking, next); } +static void setup_doitm(void) +{ + uint64_t msr; + + if ( !cpu_has_doitm ) + return; + + /* + * We don't currently enumerate DOITM to guests. As a conseqeuence, guest + * kernels will believe they're safe even when they are not. + * + * For now, set it unilaterally. This prevents otherwise-correct crypto + * code from becoming vulnerable to timing sidechannels. + */ + + rdmsrl(MSR_UARCH_MISC_CTRL, msr); + msr |= UARCH_CTRL_DOITM; + if ( !opt_dit ) + msr &= ~UARCH_CTRL_DOITM; + wrmsrl(MSR_UARCH_MISC_CTRL, msr); +} + bool opt_cpu_info; boolean_param("cpuinfo", opt_cpu_info); @@ -589,6 +611,8 @@ void identify_cpu(struct cpuinfo_x86 *c) mtrr_bp_init(); } + + setup_doitm(); } /* leaf 0xb SMT level */ --- a/xen/arch/x86/include/asm/cpufeature.h +++ b/xen/arch/x86/include/asm/cpufeature.h @@ -201,6 +201,7 @@ static inline bool boot_cpu_has(unsigned #define cpu_has_if_pschange_mc_no boot_cpu_has(X86_FEATURE_IF_PSCHANGE_MC_NO) #define cpu_has_tsx_ctrl boot_cpu_has(X86_FEATURE_TSX_CTRL) #define cpu_has_taa_no boot_cpu_has(X86_FEATURE_TAA_NO) +#define cpu_has_doitm boot_cpu_has(X86_FEATURE_DOITM) #define cpu_has_fb_clear boot_cpu_has(X86_FEATURE_FB_CLEAR) #define cpu_has_rrsba boot_cpu_has(X86_FEATURE_RRSBA) #define cpu_has_gds_ctrl boot_cpu_has(X86_FEATURE_GDS_CTRL) --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -41,6 +41,9 @@ config HAS_COMPAT config HAS_DEVICE_TREE bool +config HAS_DIT # Data Independent Timing + bool + config HAS_EX_TABLE bool @@ -175,6 +178,18 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS endmenu +config DIT_DEFAULT + bool "Data Independent Timing default" + depends on HAS_DIT + help + Hardware often surfaces instructions the timing of which is dependent + on the data they process. Some of these instructions may be used in + timing sensitive environments, e.g. cryptography. When such + instructions exist, hardware may further surface a control allowing + to make the behavior of such instructions independent of the data + they act upon. Choose the default here for when no "dit" command line + option is present. + config HYPFS bool "Hypervisor file system support" default y --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -28,6 +28,11 @@ CHECK_feature_info; enum system_state system_state = SYS_STATE_early_boot; +#ifdef CONFIG_HAS_DIT +bool __ro_after_init opt_dit = IS_ENABLED(CONFIG_DIT_DEFAULT); +boolean_param("dit", opt_dit); +#endif + static xen_commandline_t saved_cmdline; static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; --- a/xen/include/xen/param.h +++ b/xen/include/xen/param.h @@ -184,6 +184,8 @@ extern struct param_hypfs __paramhypfs_s string_param(_name, _var); \ string_runtime_only_param(_name, _var) +extern bool opt_dit; + static inline void no_config_param(const char *cfg, const char *param, const char *s, const char *e) {
[1] specifies a long list of instructions which are intended to exhibit timing behavior independent of the data they operate on. On certain hardware this independence is optional, controlled by a bit in a new MSR. Provide a command line option to control the mode Xen and its guests are to operate in, with a build time control over the default. Longer term we may want to allow guests to control this. Since Arm64 supposedly also has such a control, put command line option and Kconfig control in common files. [1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/best-practices/data-operand-independent-timing-isa-guidance.html Requested-by: Demi Marie Obenour <demi@invisiblethingslab.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- This may be viewed as a new feature, and hence be too late for 4.18. It may, however, also be viewed as security relevant, which is why I'd like to propose to at least consider it. Slightly RFC, in particular for whether the Kconfig option should default to Y or N. I would have wanted to invoke setup_doitm() from cpu_init(), but that works only on the BSP. On APs cpu_init() runs before ucode loading. Plus recheck_cpu_features() invoking identify_cpu() takes care of the BSP during S3 resume. --- v2: Introduce and use cpu_has_doitm. Add comment "borrowed" from the XenServer patch queue patch providing similar functionality. Re-base.