Re: [PATCH v2 for-4.18?] x86: support data operand independent timing mode

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > ---
> > > > 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

Attachment: signature.asc
Description: PGP signature