From: Scott Wood <oss@buserror.net> Date: 2018-05-22 21:17:30
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_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?
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
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=
quoted
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=
quoted
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=
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=
From: Scott Wood <oss@buserror.net> Date: 2018-05-29 19:16:21
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:
quoted
On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:
quoted
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)?
quoted
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.
quoted
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
On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:=0A=
quoted
Hi Scott,=0A=
=0A=
Thanks for the review.=0A=
=0A=
On 05/22/2018 11:31 PM, Scott Wood wrote:=0A=
quoted
On Tue, 2018-05-22 at 10:10 +0300, Diana Craciun wrote:=0A=
quoted
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=
quoted
quoted
quoted
and the need for a speculation barrier is read from the device tree.=
=0A=
quoted
quoted
quoted
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=
quoted
quoted
tree won't get the protection. I also don't see any device tree update=
s=0A=
quoted
quoted
--=0A=
which chips are affected?=0A=
I was planning to have the device tree changes in a different patch-set.=
=0A=
quoted
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=
quoted
quoted
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=
quoted
quoted
Should there be a way for the user to choose not to enable this (editin=
g=0A=
quoted
quoted
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=
quoted
quoted
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=
quoted
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=
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-05-31 14:21:08
Scott Wood [off-list ref] writes:
On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:
quoted
On 05/22/2018 11:31 PM, Scott Wood wrote:
quoted
quoted
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
On Tue, 2018-05-29 at 15:22 +0000, Diana Madalina Craciun wrote:=0A=
quoted
On 05/22/2018 11:31 PM, Scott Wood wrote:=0A=
quoted
Should there be a way for the user to choose not to enable this (editi=
ng=0A=
quoted
quoted
quoted
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=
quoted
quoted
quoted
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=
quoted
quoted
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=
quoted
not v1,=0A=
No.=0A=
=0A=
quoted
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=
quoted
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=
From: Scott Wood <oss@buserror.net> Date: 2018-05-31 22:05:52
On Thu, 2018-05-31 at 14:35 +0000, Diana Madalina Craciun wrote:
On 5/31/2018 5:21 PM, Michael Ellerman wrote:
quoted
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-06-01 10:40:12
Scott Wood [off-list ref] writes:
On Thu, 2018-05-31 at 14:35 +0000, Diana Madalina Craciun wrote:
quoted
On 5/31/2018 5:21 PM, Michael Ellerman wrote:
quoted
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