| Message ID | 5889d7a5fa81722472f95cc1448af0be8f359a7d.1707146506.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
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? 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). Cheers,
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. 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. > > 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. ~ Oleksii
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. Jan
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. ~ Oleksii
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). Jan
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. If they should, then, based on the current implementation, the Arm platform seems to accept having speculative vulnerabilities. The question arises: why can't other architectures make their own decisions regarding security? By default, if an architecture leaves the mentioned functions empty, it implies an agreement to potentially have speculative vulnerabilities. Subsequently, if an architecture needs to address such vulnerabilities, they can develop arch-specific nospec.h to implement the required code. If reaching an agreement to potentially have speculative vulnerabilities is deemed unfavorable, I am open to reconsidering these changes and reverting to the use of arch-specific nospec.h. Your input on this matter is appreciated. [1] https://docs.kernel.org/admin-guide/hw-vuln/l1tf.html
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,
On 21.02.2024 13:47, Oleksii wrote: > As I understand it, evaluate_nospec()/block_speculation() were > introduced for x86 to address the L1TF vulnerability specific to x86 > CPUs. Well, if you look at one of the later commits altering x86'es variant, you'll find that this wasn't really correct. > 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. If they should, then, based on the > current implementation, the Arm platform seems to accept having > speculative vulnerabilities. > > The question arises: why can't other architectures make their own > decisions regarding security? By default, if an architecture leaves the > mentioned functions empty, it implies an agreement to potentially have > speculative vulnerabilities. Subsequently, if an architecture needs to > address such vulnerabilities, they can develop arch-specific nospec.h > to implement the required code. You can't take different perspectives on security. There is some hardening which one architecture may go farther with than another, but e.g. information leaks are information leaks and hence need addressing. Of course if an arch knew it had no (known) issues, then using a generic form of this header would be okay (until such time where an issue would be found). And btw, looking at how xen/nospec.h came about, it looks pretty clear to me that array_index_mask_nospec() should move from system.h to nospec.h. That would make Arm's form immediately different from what a generic one might have, and quite likely an inline assembly variant could also do better on RISC-V (unless, as said, RISC-V simply has no such issues). Then again I notice Arm64 has no override here ... Jan
diff --git a/xen/arch/arm/include/asm/Makefile b/xen/arch/arm/include/asm/Makefile index 4a4036c951..41f73bf968 100644 --- a/xen/arch/arm/include/asm/Makefile +++ b/xen/arch/arm/include/asm/Makefile @@ -3,6 +3,7 @@ generic-y += altp2m.h generic-y += device.h generic-y += hardirq.h generic-y += iocap.h +generic-y += nospec.h generic-y += paging.h generic-y += percpu.h generic-y += random.h diff --git a/xen/arch/ppc/include/asm/Makefile b/xen/arch/ppc/include/asm/Makefile index ced02e26ed..2e8623bb10 100644 --- a/xen/arch/ppc/include/asm/Makefile +++ b/xen/arch/ppc/include/asm/Makefile @@ -5,6 +5,7 @@ generic-y += div64.h generic-y += hardirq.h generic-y += hypercall.h generic-y += iocap.h +generic-y += nospec.h generic-y += paging.h generic-y += percpu.h generic-y += random.h diff --git a/xen/arch/ppc/include/asm/nospec.h b/xen/arch/ppc/include/asm/nospec.h deleted file mode 100644 index b97322e48d..0000000000 --- a/xen/arch/ppc/include/asm/nospec.h +++ /dev/null @@ -1,15 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* From arch/arm/include/asm/nospec.h. */ -#ifndef __ASM_PPC_NOSPEC_H__ -#define __ASM_PPC_NOSPEC_H__ - -static inline bool evaluate_nospec(bool condition) -{ - return condition; -} - -static inline void block_speculation(void) -{ -} - -#endif /* __ASM_PPC_NOSPEC_H__ */ diff --git a/xen/arch/riscv/include/asm/Makefile b/xen/arch/riscv/include/asm/Makefile index ced02e26ed..2e8623bb10 100644 --- a/xen/arch/riscv/include/asm/Makefile +++ b/xen/arch/riscv/include/asm/Makefile @@ -5,6 +5,7 @@ generic-y += div64.h generic-y += hardirq.h generic-y += hypercall.h generic-y += iocap.h +generic-y += nospec.h generic-y += paging.h generic-y += percpu.h generic-y += random.h diff --git a/xen/arch/arm/include/asm/nospec.h b/xen/include/asm-generic/nospec.h similarity index 79% rename from xen/arch/arm/include/asm/nospec.h rename to xen/include/asm-generic/nospec.h index 51c7aea4f4..65fd745db2 100644 --- a/xen/arch/arm/include/asm/nospec.h +++ b/xen/include/asm-generic/nospec.h @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. */ -#ifndef _ASM_ARM_NOSPEC_H -#define _ASM_ARM_NOSPEC_H +#ifndef _ASM_GENERIC_NOSPEC_H +#define _ASM_GENERIC_NOSPEC_H static inline bool evaluate_nospec(bool condition) { @@ -13,7 +13,7 @@ static inline void block_speculation(void) { } -#endif /* _ASM_ARM_NOSPEC_H */ +#endif /* _ASM_GENERIC_NOSPEC_H */ /* * Local variables: