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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 30/09/2022 16:41, Marek Marczykowski-Górecki wrote:

> On Fri, Sep 30, 2022 at 11:25:12AM +0000, Andrew Cooper wrote:
> > On 15/09/2022 11:04, 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.
> > >
> > > 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 patch should not be taken; at least not in this form.  The whole
> > DOITM infrastructure is currently under argument, for being impossible
> > to use appropriately.
> >
> > I understand why Qubes want this blanket set, but it is a steep penalty
> > to pay;  It's only code which is already written trying to be constant
> > time/cache which gains any security from this. 
> Based on the bit description, I'd say rather "prevent _breaking_
> security of the code already written". It is not setting this bit that
> change behaviour on new parts, but it's not setting it that breaks
> previous guarantees. It's really bizarre design choice from Intel...
>
> >  On current parts, using
> > SSBD has the same behaviour, but this isn't expected to remain true in
> > the future.
> >
> > Forcing it on behind the back of a VM is mutually exclusive with
> > enumerating it for VMs to use at some point in the future when we have
> > the capability to.  i.e. specifically, you are not able to maintain the
> > ABI/API in this patch in the future.
> Regarding the current behavior of the hypervisor (without this patch):
> will guest see DOITM present but not set? Or will they not see it at
> all?
>
> Documentation clearly state:
>     For Intel® Core™ family processors based on microarchitectures
>     before Ice Lake and Intel Atom® family processors based on
>     microarchitectures before Gracemont that do not enumerate
>     IA32_UARCH_MISC_CTL, developers may assume that the instructions listed
>     here operate as if DOITM is enabled.
>
> So, if guests will not see the feature at all, it Xen should set it
> unconditionally, to remain compliant with expected hardware behaviour.
>
> If guests will see the thing (and see it not enabled), then indeed it's
> less clear what should be done right now (but I'd still like to have an
> option to enable it).

Hmm.  So yes, lets approach the problem from the other side, as "this bit needs setting to unbreak crypto code".

On hardware supporting DOITM, where we do not advertise the feature to guests (all guests right now), the guest kernel would conclude that it is safe, when in fact it is not.

So Xen should set the bit behind the back of a guest which doesn't have the DOITM enumeration presented (which is all guests right now).

But I don't think we want any Kconfig about this, or a dedicated cmdline option.  So how about this for a plan which avoids painting ourselves into a corner.

1) Extend cpuid= with a no-doitm option.  I know it's not actually a CPUID enumeration, but MSR_ARCH_CAPS should have been CPUID data, and this is the mechanism we have meaning "pretend this feature isn't enumerated".

2) On boot, and S3 resume, if DOITM and availble, set invariant mode.

That should do as a stopgap for now that keeps software safe.


Then, when we've got MSR_ARCH_CAPS working for guests, the internals of 
MSR_UARCH_MISC_CTL change to being a context switched thing which, like MSR_SPEC_CTRL, we have options for bits set behind the guest's back.  Then we set DOITM behind the guests back if levelling causes the feature to be hidden.  We do this for some bits already,
 and need to do so for more controls too.

~Andrew