Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

[Scott Wood]

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

[RESEND RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

[Diana Craciun]

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 <diana.craciun@nxp.com>
---
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 <asm/security_features.h>
 #include <asm/setup.h>
 
-
+#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)
 {
 	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
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index ff73f49..e3c0ef2 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -139,7 +139,9 @@ SECTIONS
 		*(__rfi_flush_fixup)
 		__stop___rfi_flush_fixup = .;
 	}
+#endif
 
+#if defined(CONFIG_PPC64) || defined (CONFIG_PPC_FSL_BOOK3E)
 	. = ALIGN(8);
 	__spec_barrier_fixup : AT(ADDR(__spec_barrier_fixup) - LOAD_OFFSET) {
 		__start___barrier_nospec_fixup = .;
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 3b37529..033ef28 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -162,7 +162,6 @@ void do_rfi_flush_fixups(enum l1d_flush_type types)
 		(types &  L1D_FLUSH_MTTRIG)     ? "mttrig type"
 						: "unknown");
 }
-
 void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_end)
 {
 	unsigned int instr, *dest;
@@ -188,7 +187,9 @@ void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_
 
 	printk(KERN_DEBUG "barrier-nospec: patched %d locations\n", i);
 }
+#endif /* CONFIG_PPC_BOOK3S_64 */
 
+#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_FSL_BOOK3E)
 void do_barrier_nospec_fixups(bool enable)
 {
 	void *start, *end;
@@ -199,7 +200,40 @@ void do_barrier_nospec_fixups(bool enable)
 	do_barrier_nospec_fixups_range(enable, start, end);
 }
 
-#endif /* CONFIG_PPC_BOOK3S_64 */
+#endif /* CONFIG_PPC_BOOK3S_64 || CONFIG_PPC_FSL_BOOK3E */
+
+#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);
+
+}
+#endif /* CONFIG_PPC_FSL_BOOK3E */
 
 void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
 {
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);
+	}
+}
+
 /*
  * Setup the architecture
  */
@@ -80,6 +95,8 @@ void __init corenet_gen_setup_arch(void)
 
 	pr_info("%s board\n", ppc_md.name);
 
+	setup_spec_barrier();
+
 	mpc85xx_qe_init();
 }
 
-- 
2.5.5

Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

[Diana Madalina Craciun]

Hi Scott,=0A=
=0A=
Thanks for the review.=0A=
=0A=
On 05/22/2018 11:31 PM, Scott Wood wrote:=0A=
> On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:=0A=
>> Implement the barrier_nospec as a isync;sync instruction sequence.=0A=
>> The implementation uses the infrastructure built for BOOK3S 64=0A=
>> with the difference that for NXP platforms there is no firmware involved=
=0A=
>> and the need for a speculation barrier is read from the device tree.=0A=
>> I have used the same name for the property:=0A=
>> fsl,needs-spec-barrier-for-bounds-check=0A=
> Using the device tree this way means that anyone without an updated devic=
e=0A=
> tree won't get the protection.  I also don't see any device tree updates =
--=0A=
> which chips are affected?=0A=
=0A=
I was planning to have the device tree changes in a different patch-set.=0A=
The affected cores are e500, e500mc, e5500, e6500.=0A=
=0A=
>   Wouldn't it be more robust to just have the kernel=0A=
> check the CPU type, especially given that it already does so for a lot of=
=0A=
> other purposes?=0A=
=0A=
Yes, I think that it might be a better solution not to use the device=0A=
tree at all.=0A=
=0A=
>=0A=
>> +#ifdef CONFIG_PPC_BOOK3S_64=0A=
>>  void setup_barrier_nospec(void)=0A=
>>  {=0A=
>>  	bool enable;=0A=
>> @@ -44,6 +46,12 @@ void setup_barrier_nospec(void)=0A=
>>  =0A=
>>  	enable_barrier_nospec(enable);=0A=
>>  }=0A=
>> +#elif CONFIG_PPC_FSL_BOOK3E=0A=
>> +void setup_barrier_nospec(void)=0A=
>> +{=0A=
>> +	enable_barrier_nospec(true);=0A=
>> +}=0A=
>> +#endif=0A=
> The call site is in FSL_BOOK3E-specific code so why not call=0A=
> enable_barrier_nospec() directly from there?=0A=
=0A=
OK=0A=
=0A=
>   =0A=
>> +#ifdef CONFIG_PPC_BOOK3S_64=0A=
>>  ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute=
=0A=
>> *attr, char *buf)=0A=
>>  {=0A=
>>  	bool thread_priv;=0A=
>> @@ -155,3 +164,4 @@ ssize_t cpu_show_spectre_v2(struct device *dev, stru=
ct=0A=
>> device_attribute *attr, c=0A=
>>  =0A=
>>  	return s.len;=0A=
>>  }=0A=
>> +#endif=0A=
> No equivalent of this for FSL?=0A=
=0A=
There will be an equivalent for this for FSL as well. I was planning to=0A=
send a different patch for this.=0A=
=0A=
>=0A=
>> +#ifdef CONFIG_PPC_FSL_BOOK3E=0A=
>> +void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, voi=
d=0A=
>> *fixup_end)=0A=
>> +{=0A=
>> +	unsigned int instr[2], *dest;=0A=
>> +	long *start, *end;=0A=
>> +	int i;=0A=
>> +=0A=
>> +	start =3D fixup_start;=0A=
>> +	end =3D fixup_end;=0A=
>> +=0A=
>> +	instr[0] =3D PPC_INST_NOP; /* nop */=0A=
>> +	instr[1] =3D PPC_INST_NOP; /* nop */=0A=
>> +=0A=
>> +	if (enable) {=0A=
>> +		pr_info("barrier_nospec: using isync; sync as a speculation=0A=
>> barrier\n");=0A=
>> +		instr[0] =3D PPC_INST_ISYNC;=0A=
>> +		instr[1] =3D PPC_INST_SYNC;=0A=
>> +	}=0A=
>> +=0A=
>> +	for (i =3D 0; start < end; start++, i++) {=0A=
>> +		dest =3D (void *)start + *start;=0A=
>> +		pr_devel("patching dest %lx\n", (unsigned long)dest);=0A=
>> +=0A=
>> +		patch_instruction(dest, instr[0]);=0A=
>> +		patch_instruction(dest + 1, instr[1]);=0A=
>> +=0A=
>> +	}=0A=
>> +=0A=
>> +	pr_debug("barrier-nospec: patched %d locations\n", i);=0A=
> Why patch nops in if not enabled?  Aren't those locations already nops?  =
For=0A=
> that matter, how can this function even be called on FSL_BOOK3E with enab=
le !=3D=0A=
> true?=0A=
=0A=
There is some code in arch/powerpc/kernel/security.c which allows=0A=
control of barrier_nospec via debugfs.=0A=
=0A=
>=0A=
>> diff --git a/arch/powerpc/platforms/85xx/corenet_generic.c=0A=
>> b/arch/powerpc/platforms/85xx/corenet_generic.c=0A=
>> index ac191a7..11bce3d 100644=0A=
>> --- a/arch/powerpc/platforms/85xx/corenet_generic.c=0A=
>> +++ b/arch/powerpc/platforms/85xx/corenet_generic.c=0A=
>> @@ -59,6 +59,21 @@ void __init corenet_gen_pic_init(void)=0A=
>>  	}=0A=
>>  }=0A=
>>  =0A=
>> +static void setup_spec_barrier(void)=0A=
>> +{=0A=
>> +	struct device_node *np =3D of_find_node_by_name(NULL, "cpus");=0A=
>> +=0A=
>> +	if (np) {=0A=
>> +		struct property *prop;=0A=
>> +=0A=
>> +		prop =3D of_find_property(np,=0A=
>> +		       "fsl,needs-spec-barrier-for-bounds-check", NULL);=0A=
>> +		if (prop)=0A=
>> +			setup_barrier_nospec();=0A=
>> +		of_node_put(np);=0A=
>> +	}=0A=
>> +}=0A=
> Why is this in board code?=0A=
=0A=
I will move it away from the board code.=0A=
=0A=
>=0A=
> Should there be a way for the user to choose not to enable this (editing =
the=0A=
> device tree doesn't count), for a use case that is not sufficiently secur=
ity=0A=
> sensitive to justify the performance loss?  What is the performance impac=
t of=0A=
> this patch?=0A=
=0A=
My reason was that on the other architectures Spectre variant 1=0A=
mitigations are not disabled either. But I think that it might be a good=0A=
idea to add a bootarg parameter to disable the barrier.=0A=
=0A=
Regards,=0A=
=0A=
Diana=0A=
=0A=

Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

[Scott Wood]

On Tue, 2018-05-29 at 15:22 +0000, 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

[Diana Madalina Craciun]

On 05/29/2018 10:16 PM, Scott Wood wrote:=0A=
> On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:=0A=
>> Hi Scott,=0A=
>>=0A=
>> Thanks for the review.=0A=
>>=0A=
>> On 05/22/2018 11:31 PM, Scott Wood wrote:=0A=
>>> On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:=0A=
>>>> Implement the barrier_nospec as a isync;sync instruction sequence.=0A=
>>>> The implementation uses the infrastructure built for BOOK3S 64=0A=
>>>> with the difference that for NXP platforms there is no firmware involv=
ed=0A=
>>>> and the need for a speculation barrier is read from the device tree.=
=0A=
>>>> I have used the same name for the property:=0A=
>>>> fsl,needs-spec-barrier-for-bounds-check=0A=
>>> Using the device tree this way means that anyone without an updated dev=
ice=0A=
>>> tree won't get the protection.  I also don't see any device tree update=
s=0A=
>>> --=0A=
>>> which chips are affected?=0A=
>> I was planning to have the device tree changes in a different patch-set.=
=0A=
>> The affected cores are e500, e500mc, e5500, e6500.=0A=
> So, all supported FSL/NXP book E chips.  Why not just enable the workarou=
nd=0A=
> unconditionally (and revisit if NXP ever produces a book E chip that does=
n't=0A=
> need it and/or e200 is ever supported if that's simple enough to be immun=
e)?=0A=
=0A=
I think it makes sense having in mind that all the NXP book E chips are=0A=
vulnerable. e200 is not vulnerable, but it is not properly supported in=0A=
the kernel anyway. So I guess I can enable the workaround=0A=
unconditionally. I am wondering if it does make sense patching the=0A=
instructions at all (instead just use the barrier as an isync; sync=0A=
sequence always), but in this case we will loose the possibility of=0A=
controlling it via debugfs at runtime.=0A=
=0A=
>=0A=
>>> Why patch nops in if not enabled?  Aren't those locations already=0A=
>>> nops?  For=0A=
>>> that matter, how can this function even be called on FSL_BOOK3E with=0A=
>>> enable !=3D=0A=
>>> true?=0A=
>> There is some code in arch/powerpc/kernel/security.c which allows=0A=
>> control of barrier_nospec via debugfs.=0A=
> OK.=0A=
>=0A=
>>> Should there be a way for the user to choose not to enable this (editin=
g=0A=
>>> the=0A=
>>> device tree doesn't count), for a use case that is not sufficiently=0A=
>>> security=0A=
>>> sensitive to justify the performance loss?  What is the performance imp=
act=0A=
>>> of=0A=
>>> this patch?=0A=
>> My reason was that on the other architectures Spectre variant 1=0A=
>> mitigations are not disabled either. But I think that it might be a good=
=0A=
>> idea to add a bootarg parameter to disable the barrier.=0A=
> Is there a specific policy reason why they allow spectre v2 to be disable=
d but=0A=
> not v1, or just a matter of not having a mechanism to disable it, or the =
parts=0A=
> which could practically be disabled not impacting performance much?=0A=
=0A=
I do not know for sure but I can speculate. The other architectures read=0A=
some flags set by the firmware, so I might think that they might use=0A=
different versions of firmware if they do not want the mitigations. On=0A=
the other hand the other architectures defined special barriers just for=0A=
the purpose of preventing speculations which might be more lightweight=0A=
and maybe they are not impacting the performance much. But having in=0A=
mind that the NXP parts are used in embedded scenarios that might run in=0A=
isolation (so no vulnerability) and that the barrier we are using is not=0A=
that lightweight I think that we should have a way to disable it.=0A=
=0A=
Diana=0A=
=0A=

Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

[Michael Ellerman]

Scott Wood <oss@buserror.net> writes:
> On Tue, 2018-05-29 at 15:22 +0000, 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

[Diana Madalina Craciun]

On 5/31/2018 5:21 PM, Michael Ellerman wrote:=0A=
> Scott Wood <oss@buserror.net> writes:=0A=
>> On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:=0A=
>>> On 05/22/2018 11:31 PM, Scott Wood wrote:=0A=
>>>> Should there be a way for the user to choose not to enable this (editi=
ng=0A=
>>>> the=0A=
>>>> device tree doesn't count), for a use case that is not sufficiently=0A=
>>>> security=0A=
>>>> sensitive to justify the performance loss?  What is the performance im=
pact=0A=
>>>> of=0A=
>>>> this patch?=0A=
>>> My reason was that on the other architectures Spectre variant 1=0A=
>>> mitigations are not disabled either. But I think that it might be a goo=
d=0A=
>>> idea to add a bootarg parameter to disable the barrier.=0A=
>> Is there a specific policy reason why they allow spectre v2 to be disabl=
ed but=0A=
>> not v1,=0A=
> No.=0A=
>=0A=
>> or just a matter of not having a mechanism to disable it,=0A=
> Yes and no. Some of the v1 mitigation is done via masking which can't be=
=0A=
> easily patched. eg. array_index_nospec()=0A=
>=0A=
>> or the parts which could practically be disabled not impacting=0A=
>> performance much?=0A=
> That's the mean reason AIUI.=0A=
>=0A=
> We can add a nospectre_v1 command line option if necessary.=0A=
=0A=
What about nobarrier_nospec (or similar) instead of nospectre_v1 command=0A=
line? We are not disabling all the v1 mitigations, the masking part will=0A=
remain unchanged.=0A=
=0A=
Diana=0A=
=0A=
=0A=

Re: [RFC PATCH] powerpc/fsl: Add barrier_nospec implementation for NXP PowerPC Book E

[Scott Wood]

On Thu, 2018-05-31 at 14:35 +0000, 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

[Michael Ellerman]

Scott Wood <oss@buserror.net> writes:

> On Thu, 2018-05-31 at 14:35 +0000, 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

[Diana Madalina Craciun]

On 6/1/2018 1:40 PM, Michael Ellerman wrote:=0A=
> Scott Wood <oss@buserror.net> writes:=0A=
>=0A=
>> On Thu, 2018-05-31 at 14:35 +0000, Diana Madalina Craciun wrote:=0A=
>>> On 5/31/2018 5:21 PM, Michael Ellerman wrote:=0A=
>>>> We can add a nospectre_v1 command line option if necessary.=0A=
>>> What about nobarrier_nospec (or similar) instead of nospectre_v1 comman=
d=0A=
>>> line? We are not disabling all the v1 mitigations, the masking part wil=
l=0A=
>>> remain unchanged.=0A=
>> I think nospectre_v1 makes more sense as it's about the user's intention=
s=0A=
>> rather than the implementation.  The user is giving the kernel permissio=
n to=0A=
>> not defend against spectre v1, and it's up to the implementation which=
=0A=
>> mitigations (if any) to disable in response to that, same as any other=
=0A=
>> optimization.=0A=
> Yeah I agree. We also have `nospectre_v2` on x86/s390 so I think keeping=
=0A=
> consistency with that is a must.=0A=
>=0A=
> cheers=0A=
>=0A=
OK=0A=
=0A=
Diana=0A=
=0A=