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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Diana Craciun-2
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

Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Diana Craciun-2
On 05/29/2018 10:16 PM, Scott Wood wrote:

> 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)?

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

Reply | Threaded
Open this post in threaded view
|

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

Michael Ellerman-2
In reply to this post by Scott Wood
Scott Wood <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Diana Craciun-2
On 5/31/2018 5:21 PM, Michael Ellerman wrote:

> Scott Wood <[hidden email]> 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.

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


Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Michael Ellerman-2
Scott Wood <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Diana Craciun-2
On 6/1/2018 1:40 PM, Michael Ellerman wrote:

> Scott Wood <[hidden email]> 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
>
OK

Diana