[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.
--- 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)
{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/
--
Julien Grall
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,
--
Julien Grall
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.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
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 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
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
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, -- Julien Grall
© 2016 - 2024 Red Hat, Inc.