Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
On 6/1/2018 1:40 PM, Michael Ellerman wrote: > Scott Wood writes: > >> On Thu, 2018-05-31 at 14:35 +, Diana Madalina Craciun wrote: >>> On 5/31/2018 5:21 PM, Michael Ellerman wrote: We can add a nospectre_v1 command line option if necessary. >>> What about nobarrier_nospec (or similar) instead of nospectre_v1 command >>> line? We are not disabling all the v1 mitigations, the masking part will >>> remain unchanged. >> I think nospectre_v1 makes more sense as it's about the user's intentions >> rather than the implementation. The user is giving the kernel permission to >> not defend against spectre v1, and it's up to the implementation which >> mitigations (if any) to disable in response to that, same as any other >> optimization. > Yeah I agree. We also have `nospectre_v2` on x86/s390 so I think keeping > consistency with that is a must. > > cheers > OK Diana
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
Scott Wood writes: > On Thu, 2018-05-31 at 14:35 +, Diana Madalina Craciun wrote: >> On 5/31/2018 5:21 PM, Michael Ellerman wrote: >> > >> > We can add a nospectre_v1 command line option if necessary. >> >> What about nobarrier_nospec (or similar) instead of nospectre_v1 command >> line? We are not disabling all the v1 mitigations, the masking part will >> remain unchanged. > > I think nospectre_v1 makes more sense as it's about the user's intentions > rather than the implementation. The user is giving the kernel permission to > not defend against spectre v1, and it's up to the implementation which > mitigations (if any) to disable in response to that, same as any other > optimization. Yeah I agree. We also have `nospectre_v2` on x86/s390 so I think keeping consistency with that is a must. cheers
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
On Thu, 2018-05-31 at 14:35 +, Diana Madalina Craciun wrote: > On 5/31/2018 5:21 PM, Michael Ellerman wrote: > > > > We can add a nospectre_v1 command line option if necessary. > > What about nobarrier_nospec (or similar) instead of nospectre_v1 command > line? We are not disabling all the v1 mitigations, the masking part will > remain unchanged. I think nospectre_v1 makes more sense as it's about the user's intentions rather than the implementation. The user is giving the kernel permission to not defend against spectre v1, and it's up to the implementation which mitigations (if any) to disable in response to that, same as any other optimization. -Scott
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
On 5/31/2018 5:21 PM, Michael Ellerman wrote: > Scott Wood writes: >> On Tue, 2018-05-29 at 15:22 +, Diana Madalina Craciun wrote: >>> On 05/22/2018 11:31 PM, Scott Wood wrote: Should there be a way for the user to choose not to enable this (editing the device tree doesn't count), for a use case that is not sufficiently security sensitive to justify the performance loss? What is the performance impact of this patch? >>> My reason was that on the other architectures Spectre variant 1 >>> mitigations are not disabled either. But I think that it might be a good >>> idea to add a bootarg parameter to disable the barrier. >> Is there a specific policy reason why they allow spectre v2 to be disabled >> but >> not v1, > No. > >> or just a matter of not having a mechanism to disable it, > Yes and no. Some of the v1 mitigation is done via masking which can't be > easily patched. eg. array_index_nospec() > >> or the parts which could practically be disabled not impacting >> performance much? > That's the mean reason AIUI. > > We can add a nospectre_v1 command line option if necessary. What about nobarrier_nospec (or similar) instead of nospectre_v1 command line? We are not disabling all the v1 mitigations, the masking part will remain unchanged. Diana
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
Scott Wood writes: > On Tue, 2018-05-29 at 15:22 +, Diana Madalina Craciun wrote: >> On 05/22/2018 11:31 PM, Scott Wood wrote: > >> > Should there be a way for the user to choose not to enable this (editing >> > the >> > device tree doesn't count), for a use case that is not sufficiently >> > security >> > sensitive to justify the performance loss? What is the performance impact >> > of >> > this patch? >> >> My reason was that on the other architectures Spectre variant 1 >> mitigations are not disabled either. But I think that it might be a good >> idea to add a bootarg parameter to disable the barrier. > > Is there a specific policy reason why they allow spectre v2 to be disabled but > not v1, No. > or just a matter of not having a mechanism to disable it, Yes and no. Some of the v1 mitigation is done via masking which can't be easily patched. eg. array_index_nospec() > or the parts which could practically be disabled not impacting > performance much? That's the mean reason AIUI. We can add a nospectre_v1 command line option if necessary. cheers
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
On 05/29/2018 10:16 PM, Scott Wood wrote: > On Tue, 2018-05-29 at 15:22 +, Diana Madalina Craciun wrote: >> Hi Scott, >> >> Thanks for the review. >> >> On 05/22/2018 11:31 PM, Scott Wood wrote: >>> On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote: Implement the barrier_nospec as a isync;sync instruction sequence. The implementation uses the infrastructure built for BOOK3S 64 with the difference that for NXP platforms there is no firmware involved and the need for a speculation barrier is read from the device tree. I have used the same name for the property: fsl,needs-spec-barrier-for-bounds-check >>> Using the device tree this way means that anyone without an updated device >>> tree won't get the protection. I also don't see any device tree updates >>> -- >>> which chips are affected? >> I was planning to have the device tree changes in a different patch-set. >> The affected cores are e500, e500mc, e5500, e6500. > So, all supported FSL/NXP book E chips. Why not just enable the workaround > unconditionally (and revisit if NXP ever produces a book E chip that doesn't > need it and/or e200 is ever supported if that's simple enough to be immune)? I think it makes sense having in mind that all the NXP book E chips are vulnerable. e200 is not vulnerable, but it is not properly supported in the kernel anyway. So I guess I can enable the workaround unconditionally. I am wondering if it does make sense patching the instructions at all (instead just use the barrier as an isync; sync sequence always), but in this case we will loose the possibility of controlling it via debugfs at runtime. > >>> Why patch nops in if not enabled? Aren't those locations already >>> nops? For >>> that matter, how can this function even be called on FSL_BOOK3E with >>> enable != >>> true? >> There is some code in arch/powerpc/kernel/security.c which allows >> control of barrier_nospec via debugfs. > OK. > >>> Should there be a way for the user to choose not to enable this (editing >>> the >>> device tree doesn't count), for a use case that is not sufficiently >>> security >>> sensitive to justify the performance loss? What is the performance impact >>> of >>> this patch? >> My reason was that on the other architectures Spectre variant 1 >> mitigations are not disabled either. But I think that it might be a good >> idea to add a bootarg parameter to disable the barrier. > Is there a specific policy reason why they allow spectre v2 to be disabled but > not v1, or just a matter of not having a mechanism to disable it, or the parts > which could practically be disabled not impacting performance much? I do not know for sure but I can speculate. The other architectures read some flags set by the firmware, so I might think that they might use different versions of firmware if they do not want the mitigations. On the other hand the other architectures defined special barriers just for the purpose of preventing speculations which might be more lightweight and maybe they are not impacting the performance much. But having in mind that the NXP parts are used in embedded scenarios that might run in isolation (so no vulnerability) and that the barrier we are using is not that lightweight I think that we should have a way to disable it. Diana
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
On Tue, 2018-05-29 at 15:22 +, Diana Madalina Craciun wrote: > Hi Scott, > > Thanks for the review. > > On 05/22/2018 11:31 PM, Scott Wood wrote: > > On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote: > > > Implement the barrier_nospec as a isync;sync instruction sequence. > > > The implementation uses the infrastructure built for BOOK3S 64 > > > with the difference that for NXP platforms there is no firmware involved > > > and the need for a speculation barrier is read from the device tree. > > > I have used the same name for the property: > > > fsl,needs-spec-barrier-for-bounds-check > > > > Using the device tree this way means that anyone without an updated device > > tree won't get the protection. I also don't see any device tree updates > > -- > > which chips are affected? > > I was planning to have the device tree changes in a different patch-set. > The affected cores are e500, e500mc, e5500, e6500. So, all supported FSL/NXP book E chips. Why not just enable the workaround unconditionally (and revisit if NXP ever produces a book E chip that doesn't need it and/or e200 is ever supported if that's simple enough to be immune)? > > Why patch nops in if not enabled? Aren't those locations already > > nops? For > > that matter, how can this function even be called on FSL_BOOK3E with > > enable != > > true? > > There is some code in arch/powerpc/kernel/security.c which allows > control of barrier_nospec via debugfs. OK. > > Should there be a way for the user to choose not to enable this (editing > > the > > device tree doesn't count), for a use case that is not sufficiently > > security > > sensitive to justify the performance loss? What is the performance impact > > of > > this patch? > > My reason was that on the other architectures Spectre variant 1 > mitigations are not disabled either. But I think that it might be a good > idea to add a bootarg parameter to disable the barrier. Is there a specific policy reason why they allow spectre v2 to be disabled but not v1, or just a matter of not having a mechanism to disable it, or the parts which could practically be disabled not impacting performance much? -Scott
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
Hi Scott,
Thanks for the review.
On 05/22/2018 11:31 PM, Scott Wood wrote:
> On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:
>> Implement the barrier_nospec as a isync;sync instruction sequence.
>> The implementation uses the infrastructure built for BOOK3S 64
>> with the difference that for NXP platforms there is no firmware involved
>> and the need for a speculation barrier is read from the device tree.
>> I have used the same name for the property:
>> fsl,needs-spec-barrier-for-bounds-check
> Using the device tree this way means that anyone without an updated device
> tree won't get the protection. I also don't see any device tree updates --
> which chips are affected?
I was planning to have the device tree changes in a different patch-set.
The affected cores are e500, e500mc, e5500, e6500.
> Wouldn't it be more robust to just have the kernel
> check the CPU type, especially given that it already does so for a lot of
> other purposes?
Yes, I think that it might be a better solution not to use the device
tree at all.
>
>> +#ifdef CONFIG_PPC_BOOK3S_64
>> void setup_barrier_nospec(void)
>> {
>> bool enable;
>> @@ -44,6 +46,12 @@ void setup_barrier_nospec(void)
>>
>> enable_barrier_nospec(enable);
>> }
>> +#elif CONFIG_PPC_FSL_BOOK3E
>> +void setup_barrier_nospec(void)
>> +{
>> +enable_barrier_nospec(true);
>> +}
>> +#endif
> The call site is in FSL_BOOK3E-specific code so why not call
> enable_barrier_nospec() directly from there?
OK
>
>> +#ifdef CONFIG_PPC_BOOK3S_64
>> ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute
>> *attr, char *buf)
>> {
>> bool thread_priv;
>> @@ -155,3 +164,4 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct
>> device_attribute *attr, c
>>
>> return s.len;
>> }
>> +#endif
> No equivalent of this for FSL?
There will be an equivalent for this for FSL as well. I was planning to
send a different patch for this.
>
>> +#ifdef CONFIG_PPC_FSL_BOOK3E
>> +void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void
>> *fixup_end)
>> +{
>> +unsigned int instr[2], *dest;
>> +long *start, *end;
>> +int i;
>> +
>> +start = fixup_start;
>> +end = fixup_end;
>> +
>> +instr[0] = PPC_INST_NOP; /* nop */
>> +instr[1] = PPC_INST_NOP; /* nop */
>> +
>> +if (enable) {
>> +pr_info("barrier_nospec: using isync; sync as a speculation
>> barrier\n");
>> +instr[0] = PPC_INST_ISYNC;
>> +instr[1] = PPC_INST_SYNC;
>> +}
>> +
>> +for (i = 0; start < end; start++, i++) {
>> +dest = (void *)start + *start;
>> +pr_devel("patching dest %lx\n", (unsigned long)dest);
>> +
>> +patch_instruction(dest, instr[0]);
>> +patch_instruction(dest + 1, instr[1]);
>> +
>> +}
>> +
>> +pr_debug("barrier-nospec: patched %d locations\n", i);
> Why patch nops in if not enabled? Aren't those locations already nops? For
> that matter, how can this function even be called on FSL_BOOK3E with enable !=
> true?
There is some code in arch/powerpc/kernel/security.c which allows
control of barrier_nospec via debugfs.
>
>> diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c
>> b/arch/powerpc/platforms/85xx/corenet_generic.c
>> index ac191a7..11bce3d 100644
>> --- a/arch/powerpc/platforms/85xx/corenet_generic.c
>> +++ b/arch/powerpc/platforms/85xx/corenet_generic.c
>> @@ -59,6 +59,21 @@ void __init corenet_gen_pic_init(void)
>> }
>> }
>>
>> +static void setup_spec_barrier(void)
>> +{
>> +struct device_node *np = of_find_node_by_name(NULL, "cpus");
>> +
>> +if (np) {
>> +struct property *prop;
>> +
>> +prop = of_find_property(np,
>> + "fsl,needs-spec-barrier-for-bounds-check", NULL);
>> +if (prop)
>> +setup_barrier_nospec();
>> +of_node_put(np);
>> +}
>> +}
> Why is this in board code?
I will move it away from the board code.
>
> Should there be a way for the user to choose not to enable this (editing the
> device tree doesn't count), for a use case that is not sufficiently security
> sensitive to justify the performance loss? What is the performance impact of
> this patch?
My reason was that on the other architectures Spectre variant 1
mitigations are not disabled either. But I think that it might be a good
idea to add a bootarg parameter to disable the barrier.
Regards,
Diana
[RESEND RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
Implement the barrier_nospec as a isync;sync instruction sequence.
The implementation uses the infrastructure built for BOOK3S 64
with the difference that for NXP platforms there is no firmware involved
and the need for a speculation barrier is read from the device tree.
I have used the same name for the property:
fsl,needs-spec-barrier-for-bounds-check
Signed-off-by: Diana Craciun
---
The patches were created on top of the BOOK3S 64 patches:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-April/172137.html
arch/powerpc/include/asm/barrier.h| 12 -
arch/powerpc/include/asm/setup.h | 2 +-
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/module.c | 2 ++
arch/powerpc/kernel/security.c| 12 -
arch/powerpc/kernel/vmlinux.lds.S | 2 ++
arch/powerpc/lib/feature-fixups.c | 38 +--
arch/powerpc/platforms/85xx/corenet_generic.c | 17
8 files changed, 81 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/barrier.h
b/arch/powerpc/include/asm/barrier.h
index f67b3f6..1379386 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -86,7 +86,17 @@ do {
\
// This also acts as a compiler barrier due to the memory clobber.
#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
-#else /* !CONFIG_PPC_BOOK3S_64 */
+#elif defined(CONFIG_PPC_FSL_BOOK3E)
+/*
+ * Prevent the execution of subsequent instructions speculatively using a
+ * isync;sync instruction sequence.
+ */
+#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop; nop
+
+// This also acts as a compiler barrier due to the memory clobber.
+#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
+
+#else /* !CONFIG_PPC_BOOK3S_64 && !CONFIG_PPC_FSL_BOOK3E */
#define barrier_nospec_asm
#define barrier_nospec()
#endif
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index aeb175e8..fbc3ef7 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -55,7 +55,7 @@ void do_rfi_flush_fixups(enum l1d_flush_type types);
void setup_barrier_nospec(void);
void do_barrier_nospec_fixups(bool enable);
-#ifdef CONFIG_PPC_BOOK3S_64
+#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_FSL_BOOK3E)
void do_barrier_nospec_fixups_range(bool enable, void *start, void *end);
#else
static inline void do_barrier_nospec_fixups_range(bool enable, void *start,
void *end) { };
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 2b4c40b2..d9dee43 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -76,7 +76,7 @@ endif
obj64-$(CONFIG_HIBERNATION)+= swsusp_asm64.o
obj-$(CONFIG_MODULES) += module.o module_$(BITS).o
obj-$(CONFIG_44x) += cpu_setup_44x.o
-obj-$(CONFIG_PPC_FSL_BOOK3E) += cpu_setup_fsl_booke.o
+obj-$(CONFIG_PPC_FSL_BOOK3E) += cpu_setup_fsl_booke.o security.o
obj-$(CONFIG_PPC_DOORBELL) += dbell.o
obj-$(CONFIG_JUMP_LABEL) += jump_label.o
diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
index a72698c..ede64a5 100644
--- a/arch/powerpc/kernel/module.c
+++ b/arch/powerpc/kernel/module.c
@@ -72,7 +72,9 @@ int module_finalize(const Elf_Ehdr *hdr,
do_feature_fixups(powerpc_firmware_features,
(void *)sect->sh_addr,
(void *)sect->sh_addr + sect->sh_size);
+#endif
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC_FSL_BOOK3E)
sect = find_section(hdr, sechdrs, "__spec_barrier_fixup");
if (sect != NULL)
do_barrier_nospec_fixups_range(true,
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index d1b9639..01b6c55 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -12,8 +12,9 @@
#include
#include
-
+#ifdef CONFIG_PPC_BOOK3S_64
unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT;
+#endif
static bool barrier_nospec_enabled;
@@ -23,6 +24,7 @@ static void enable_barrier_nospec(bool enable)
do_barrier_nospec_fixups(enable);
}
+#ifdef CONFIG_PPC_BOOK3S_64
void setup_barrier_nospec(void)
{
bool enable;
@@ -44,6 +46,12 @@ void setup_barrier_nospec(void)
enable_barrier_nospec(enable);
}
+#elif CONFIG_PPC_FSL_BOOK3E
+void setup_barrier_nospec(void)
+{
+ enable_barrier_nospec(true);
+}
+#endif
#ifdef CONFIG_DEBUG_FS
static int barrier_nospec_set(void *data, u64 val)
@@ -82,6 +90,7 @@ static __init int barrier_nospec_debugfs_init(void)
device_initcall(barrier_nospec_debugfs_init);
#endif /* CONFIG_DEBUG_FS */
+#ifdef CONFIG_PPC_BOOK3S_64
ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr,
char *buf
Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E
On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:
> Implement the barrier_nospec as a isync;sync instruction sequence.
> The implementation uses the infrastructure built for BOOK3S 64
> with the difference that for NXP platforms there is no firmware involved
> and the need for a speculation barrier is read from the device tree.
> I have used the same name for the property:
> fsl,needs-spec-barrier-for-bounds-check
Using the device tree this way means that anyone without an updated device
tree won't get the protection. I also don't see any device tree updates --
which chips are affected? Wouldn't it be more robust to just have the kernel
check the CPU type, especially given that it already does so for a lot of
other purposes?
> +#ifdef CONFIG_PPC_BOOK3S_64
> void setup_barrier_nospec(void)
> {
> bool enable;
> @@ -44,6 +46,12 @@ void setup_barrier_nospec(void)
>
> enable_barrier_nospec(enable);
> }
> +#elif CONFIG_PPC_FSL_BOOK3E
> +void setup_barrier_nospec(void)
> +{
> + enable_barrier_nospec(true);
> +}
> +#endif
The call site is in FSL_BOOK3E-specific code so why not call
enable_barrier_nospec() directly from there?
>
> +#ifdef CONFIG_PPC_BOOK3S_64
> ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute
> *attr, char *buf)
> {
> bool thread_priv;
> @@ -155,3 +164,4 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct
> device_attribute *attr, c
>
> return s.len;
> }
> +#endif
No equivalent of this for FSL?
> +#ifdef CONFIG_PPC_FSL_BOOK3E
> +void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void
> *fixup_end)
> +{
> + unsigned int instr[2], *dest;
> + long *start, *end;
> + int i;
> +
> + start = fixup_start;
> + end = fixup_end;
> +
> + instr[0] = PPC_INST_NOP; /* nop */
> + instr[1] = PPC_INST_NOP; /* nop */
> +
> + if (enable) {
> + pr_info("barrier_nospec: using isync; sync as a speculation
> barrier\n");
> + instr[0] = PPC_INST_ISYNC;
> + instr[1] = PPC_INST_SYNC;
> + }
> +
> + for (i = 0; start < end; start++, i++) {
> + dest = (void *)start + *start;
> + pr_devel("patching dest %lx\n", (unsigned long)dest);
> +
> + patch_instruction(dest, instr[0]);
> + patch_instruction(dest + 1, instr[1]);
> +
> + }
> +
> + pr_debug("barrier-nospec: patched %d locations\n", i);
Why patch nops in if not enabled? Aren't those locations already nops? For
that matter, how can this function even be called on FSL_BOOK3E with enable !=
true?
> diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c
> b/arch/powerpc/platforms/85xx/corenet_generic.c
> index ac191a7..11bce3d 100644
> --- a/arch/powerpc/platforms/85xx/corenet_generic.c
> +++ b/arch/powerpc/platforms/85xx/corenet_generic.c
> @@ -59,6 +59,21 @@ void __init corenet_gen_pic_init(void)
> }
> }
>
> +static void setup_spec_barrier(void)
> +{
> + struct device_node *np = of_find_node_by_name(NULL, "cpus");
> +
> + if (np) {
> + struct property *prop;
> +
> + prop = of_find_property(np,
> +"fsl,needs-spec-barrier-for-bounds-check", NULL);
> + if (prop)
> + setup_barrier_nospec();
> + of_node_put(np);
> + }
> +}
Why is this in board code?
Should there be a way for the user to choose not to enable this (editing the
device tree doesn't count), for a use case that is not sufficiently security
sensitive to justify the performance loss? What is the performance impact of
this patch?
-Scott
