MIME-Version: 1.0
References: <20230125012801.362496-1-ebiggers@kernel.org> <14506678-918f-81e1-2c26-2b347ff50701@intel.com>
In-Reply-To: <14506678-918f-81e1-2c26-2b347ff50701@intel.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 25 Jan 2023 17:22:44 +0100
Message-ID: <CAMj1kXEZi1ewVdqXHTi7kWX9aT+j1=rFOVE55LdJYb9LkV9Dkw@mail.gmail.com>
Subject: Re: [PATCH] x86: enable Data Operand Independent Timing Mode
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     Eric Biggers <ebiggers@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H . Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Roxana Bradescu <roxabee@chromium.org>,
        Adam Langley <agl@google.com>,
        "Jason A . Donenfeld" <Jason@zx2c4.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 25 Jan 2023 at 16:29, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 1/24/23 17:28, Eric Biggers wrote:
> > To mitigate this CPU vulnerability, it's possible to enable "Data
> > Operand Independent Timing Mode" (DOITM) by setting a bit in a MSR.
> > While Intel's documentation suggests that this bit should only be set
> > where "necessary", that is highly impractical, given the fact that
> > cryptography can happen nearly anywhere in the kernel and userspace, and
> > the fact that the entire kernel likely needs to be protected anyway.
>
> I think this misses a key point from the documentation:
>
>         This functionality is intended for use by software which has
>         already applied other techniques to mitigate software timing
>         side channels, such as those documented in Intel's Guidelines
>         for Mitigating Timing Side Channels Against Cryptographic
>         Implementations.
>
> Translating from Intel-speak: Intel thinks that DOITM purely a way to
> make the CPU run slower if you haven't already written code specifically
> to mitigate timing side channels.  All pain, no gain.
>
> The kernel as a whole is not written that way.  I'm sure the crypto
> folks that are cc'd can tell us specifically if the kernel crypto code
> is written following those recommendations.
>

Cryptography is often singled out because it deals with confidential
data, and if timing variances leak the data, the confidentiality is
violated.

However, this is not fundamentally different from execution at a
higher privilege level. In this case, the data is shielded from other
observers by the h/w enforced privilege boundary rather than
encryption, but if timing variances leak the data, the result is the
same, i.e., data that was assumed to be confidential as far as user
space is concerned is no longer confidential.

All the nospec stuff we added for Spectre v1 serves the same purpose,
essentially, although the timing variances due to cache misses are
likely easier to measure. IOW, some of the kernel is now written that
way in fact, although the author of that doc may have had something
else in mind.

So IMHO, the scope is really not as narrow as you think.

> So, let's call this patch what it is: a potential global slowdown which
> protects a very small amount of crypto code, probably just in userspace.
>  That is probably the code that's generating your RSA keys, so it's
> quite important, but it's also a _very_ small total amount of code.
>
> There's another part here which I think was recently added to the
> documentation:
>
>         Intel expects the performance impact of this mode may be
>         significantly higher on future processors.
>
> That's _meant_ to be really scary and keep folks from turning this on by
> default, aka. what this patch does.  Your new CPU will be really slow if
> you turn this on!  Boo!
>

What is the penalty for switching it on and off? On arm64, it is now
on by default in the kernel, and off by default in user space, and
user space can opt into it using an unprivileged instruction.

> All that said, and given the information that Intel has released, I
> think this patch is generally the right thing to do.  I don't think
> people are wrong for looking at "DODT" as being a new vulnerability.
> Intel obviously doesn't see it that way, which is why "DODT" has (as far
> as I can tell) not been treated with the same security pomp and
> circumstance as other stuff.
>
> Last, if you're going to propose that this be turned on, I expect to see
> at least _some_ performance data.  DOITM=1 isn't free, even on Ice Lake.