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

[Julien Grall <julien@xxxxxxx>]

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.
> > > --- 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