Re: [PATCH v4 07/30] xen/asm-generic: introdure nospec.h

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 21/02/2024 12:47, Oleksii wrote:
> On Wed, 2024-02-21 at 12:00 +0100, Jan Beulich wrote:
> > On 20.02.2024 21:30, Oleksii wrote:
> > > On Mon, 2024-02-19 at 13:18 +0100, Jan Beulich wrote:
> > > > On 19.02.2024 12:59, Oleksii wrote:
> > > > > Hi Julien,
> > > > >
> > > > > On Sun, 2024-02-18 at 18:30 +0000, Julien Grall wrote:
> > > > > > Hi Oleksii,
> > > > > >
> > > > > > Title: Typo s/introdure/introduce/
> > > > > >
> > > > > > On 05/02/2024 15:32, Oleksii Kurochko wrote:
> > > > > > > The <asm/nospec.h> header is similar between Arm, PPC, and
> > > > > > > RISC-V,
> > > > > > > so it has been moved to asm-generic.
> > > > > > I am not 100% convinced that moving this header to asm-
> > > > > > generic is
> > > > > > a
> > > > > > good
> > > > > > idea. At least for Arm, those helpers ought to be non-empty,
> > > > > > what
> > > > > > about
> > > > > > RISC-V?
> > > > > For Arm, they are not taking any action, are they? There are no
> > > > > specific fences or other mechanisms inside
> > > > > evaluate_nospec()/block_speculation() to address speculation.
> > > > The question isn't the status quo, but how things should be
> > > > looking
> > > > like
> > > > if everything was in place that's (in principle) needed.
> > > >
> > > > > For RISC-V, it can be implemented in a similar manner, at least
> > > > > for
> > > > > now. Since these functions are only used in the grant tables
> > > > > code (
> > > > > for
> > > > > Arm and so for RISC-V ), which is not supported by RISC-V.
> > > > Same here - the question is whether long term, when gnttab is
> > > > also
> > > > supported, RISC-V would get away without doing anything. Still
> > > > ...
> > > >
> > > > > > If the answer is they should be non-empty. Then I would
> > > > > > consider
> > > > > > to
> > > > > > keep
> > > > > > the duplication to make clear that each architecture should
> > > > > > take
> > > > > > their
> > > > > > own decision in term of security.
> > > > > >
> > > > > > The alternative, is to have a generic implementation that is
> > > > > > safe
> > > > > > by
> > > > > > default (if that's even possible).
> > > > > I am not certain that we can have a generic implementation, as
> > > > > each
> > > > > architecture may have specific speculation issues.
> > > > ... it's theoretically possible that there'd be an arch with no
> > > > speculation issues, maybe simply because of not speculating.
> > > I am not sure that understand your and Julien point.
> > >
> > > For example, modern CPU uses speculative execution to reduce the
> > > cost
> > > of conditional branch instructions using schemes that predict the
> > > execution path of a program based on the history of branch
> > > executions.
> > >
> > > Arm CPUs are vulnerable for speculative execution, but if to look
> > > at
> > > the code of evaluate_nospec()/block_speculation() functions they
> > > are
> > > doing nothing for Arm.
> > Which, as I understood Julien say, likely isn't correct. In which
> > case
> > this header shouldn't be dropped, using the generic one instead. The
> > generic headers, as pointed out several times before, should imo be
> > used
> > only if their use results in correct behavior. What is acceptable is
> > if
> > their use results in sub-optimal behavior (e.g. reduced performance
> > or
> > lack of a certain feature that another architecture maybe
> > implements).
> As I understand it, evaluate_nospec()/block_speculation() were
> introduced for x86 to address the L1TF vulnerability specific to x86
> CPUs. This vulnerability is exclusive to x86 architectures [1], which
> explains why evaluate_nospec()/block_speculation() are left empty for
> Arm, RISC-V, and PPC.
>
> It is unclear whether these functions should be utilized to mitigate
> other speculation vulnerabilities. 
The name is generic enough that someone may want to use it for other 
speculations. If we think this is only related to L1TF, then the 
functions names should reflect it. But see below.
> If they should, then, based on the
> current implementation, the Arm platform seems to accept having
> speculative vulnerabilities.
Looking at some of the use in common code (such as the grant-table 
code), it is unclear to me why it is empty on Arm. I think we need a 
speculation barrier.
I would raise the same question for RISC-V/PPC.

> The question arises: why can't other architectures make their own
> decisions regarding security? 
Each architecture can make there own decision. I am not trying to 
prevent that. What I am trying to prevent is a developper including the 
asm-generic without realizing that the header doesn't provide a safe 
version.
> By default, if an architecture leaves the
> mentioned functions empty, it implies an agreement to potentially have
> speculative vulnerabilities. 
See above. That agreement is somewhat implicit. It would be better if 
this is explicit.
So overall, I would prefer if that header is not part of asm-generic.

Cheers,

--
Julien Grall