diff mbox series

[4/4] powerpc/powernv: Remove POWER9 PVR version check for entry and uaccess flushes

Message ID 20210503130243.891868-5-npiggin@gmail.com (mailing list archive)
State Accepted
Headers show
Series powerpc/security mitigation updates | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (e3a9b9d6a03f5fbf99b540e863b001d46ba1735c)
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-pmac32 success Build succeeded
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 15 lines checked
snowpatch_ozlabs/needsstable success Patch has no Fixes tags

Commit Message

Nicholas Piggin May 3, 2021, 1:02 p.m. UTC
These aren't necessarily POWER9 only, and it's not to say some new
vulnerability may not get discovered on other processors for which
we would like the flexibility of having the workaround enabled by
firmware.

Remove the restriction that they only apply to POWER9.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/setup.c | 9 ---------
 1 file changed, 9 deletions(-)

Comments

Joel Stanley May 4, 2021, 12:51 a.m. UTC | #1
On Mon, 3 May 2021 at 13:04, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> These aren't necessarily POWER9 only, and it's not to say some new
> vulnerability may not get discovered on other processors for which
> we would like the flexibility of having the workaround enabled by
> firmware.
>
> Remove the restriction that they only apply to POWER9.

I was wondering how these worked which led me to reviewing your patch.
From what I could see, these are enabled by default (SEC_FTR_DEFAULT
in arch/powerpc/include/asm/security_features.h), so unless all
non-POWER9 machines have set the "please don't" bit in their firmware
this patch will enable the feature for those machines. Is that what
you wanted?

>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/platforms/powernv/setup.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index a8db3f153063..6ec67223f8c7 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -122,15 +122,6 @@ static void pnv_setup_security_mitigations(void)
>                         type = L1D_FLUSH_ORI;
>         }
>
> -       /*
> -        * If we are non-Power9 bare metal, we don't need to flush on kernel
> -        * entry or after user access: they fix a P9 specific vulnerability.
> -        */
> -       if (!pvr_version_is(PVR_POWER9)) {
> -               security_ftr_clear(SEC_FTR_L1D_FLUSH_ENTRY);
> -               security_ftr_clear(SEC_FTR_L1D_FLUSH_UACCESS);
> -       }
> -
>         enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) && \
>                  (security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR)   || \
>                   security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV));
> --
> 2.23.0
>
Nicholas Piggin May 4, 2021, 9:16 a.m. UTC | #2
Excerpts from Joel Stanley's message of May 4, 2021 10:51 am:
> On Mon, 3 May 2021 at 13:04, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> These aren't necessarily POWER9 only, and it's not to say some new
>> vulnerability may not get discovered on other processors for which
>> we would like the flexibility of having the workaround enabled by
>> firmware.
>>
>> Remove the restriction that they only apply to POWER9.
> 
> I was wondering how these worked which led me to reviewing your patch.
> From what I could see, these are enabled by default (SEC_FTR_DEFAULT
> in arch/powerpc/include/asm/security_features.h), so unless all
> non-POWER9 machines have set the "please don't" bit in their firmware
> this patch will enable the feature for those machines. Is that what
> you wanted?

Yes. POWER7/8 should be affected (it's similar mechanism that requires
the meltdown RFI flush, which those processors need).

POWER10 we haven't released a bare metal firmware with the right bits
yet. Not urgent at the moment but wouldn't hurt to specify them and
add the Linux code for them.

Thanks,
Nick

> 
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  arch/powerpc/platforms/powernv/setup.c | 9 ---------
>>  1 file changed, 9 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
>> index a8db3f153063..6ec67223f8c7 100644
>> --- a/arch/powerpc/platforms/powernv/setup.c
>> +++ b/arch/powerpc/platforms/powernv/setup.c
>> @@ -122,15 +122,6 @@ static void pnv_setup_security_mitigations(void)
>>                         type = L1D_FLUSH_ORI;
>>         }
>>
>> -       /*
>> -        * If we are non-Power9 bare metal, we don't need to flush on kernel
>> -        * entry or after user access: they fix a P9 specific vulnerability.
>> -        */
>> -       if (!pvr_version_is(PVR_POWER9)) {
>> -               security_ftr_clear(SEC_FTR_L1D_FLUSH_ENTRY);
>> -               security_ftr_clear(SEC_FTR_L1D_FLUSH_UACCESS);
>> -       }
>> -
>>         enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) && \
>>                  (security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR)   || \
>>                   security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV));
>> --
>> 2.23.0
>>
>
Joel Stanley May 5, 2021, 1:43 a.m. UTC | #3
On Tue, 4 May 2021 at 09:16, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Excerpts from Joel Stanley's message of May 4, 2021 10:51 am:
> > On Mon, 3 May 2021 at 13:04, Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> These aren't necessarily POWER9 only, and it's not to say some new
> >> vulnerability may not get discovered on other processors for which
> >> we would like the flexibility of having the workaround enabled by
> >> firmware.
> >>
> >> Remove the restriction that they only apply to POWER9.
> >
> > I was wondering how these worked which led me to reviewing your patch.
> > From what I could see, these are enabled by default (SEC_FTR_DEFAULT
> > in arch/powerpc/include/asm/security_features.h), so unless all
> > non-POWER9 machines have set the "please don't" bit in their firmware
> > this patch will enable the feature for those machines. Is that what
> > you wanted?
>
> Yes. POWER7/8 should be affected (it's similar mechanism that requires
> the meltdown RFI flush, which those processors need).
>
> POWER10 we haven't released a bare metal firmware with the right bits
> yet. Not urgent at the moment but wouldn't hurt to specify them and
> add the Linux code for them.

Thanks for the explanation. This could go in the commit message if you re-spin.

Reviewed-by: Joel Stanley <joel@jms.id.au>
Nicholas Piggin May 8, 2021, 10 a.m. UTC | #4
Excerpts from Joel Stanley's message of May 5, 2021 11:43 am:
> On Tue, 4 May 2021 at 09:16, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Excerpts from Joel Stanley's message of May 4, 2021 10:51 am:
>> > On Mon, 3 May 2021 at 13:04, Nicholas Piggin <npiggin@gmail.com> wrote:
>> >>
>> >> These aren't necessarily POWER9 only, and it's not to say some new
>> >> vulnerability may not get discovered on other processors for which
>> >> we would like the flexibility of having the workaround enabled by
>> >> firmware.
>> >>
>> >> Remove the restriction that they only apply to POWER9.
>> >
>> > I was wondering how these worked which led me to reviewing your patch.
>> > From what I could see, these are enabled by default (SEC_FTR_DEFAULT
>> > in arch/powerpc/include/asm/security_features.h), so unless all
>> > non-POWER9 machines have set the "please don't" bit in their firmware
>> > this patch will enable the feature for those machines. Is that what
>> > you wanted?
>>
>> Yes. POWER7/8 should be affected (it's similar mechanism that requires
>> the meltdown RFI flush, which those processors need).
>>
>> POWER10 we haven't released a bare metal firmware with the right bits
>> yet. Not urgent at the moment but wouldn't hurt to specify them and
>> add the Linux code for them.
> 
> Thanks for the explanation. This could go in the commit message if you re-spin.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 

I was talking about the same thing with Michael and he dug up an old
email chain that proves me wrong. P7/8 are actually slightly different.
I'm not sure what I can explain of it in public unfortunately.

How about this?

---

These aren't necessarily POWER9 only, and it's not to say some new
vulnerability may not get discovered on other processors for which
we would like the flexibility of having the workaround enabled by
firmware.

Remove the restriction that the workarounds only apply to POWER9.

However POWER7 and POWER8 are not affected, and they may not have
older firmware that does not advertise this, so clear these workarounds
manually.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/powernv/setup.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index a8db3f153063..874fb016384a 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -123,10 +123,14 @@ static void pnv_setup_security_mitigations(void)
 	}
 
 	/*
-	 * If we are non-Power9 bare metal, we don't need to flush on kernel
-	 * entry or after user access: they fix a P9 specific vulnerability.
+	 * The issues addressed by the entry and uaccess flush don't affect P7
+	 * or P8, so on bare metal disable them explicitly in case firmware
+	 * does not include these bits. POWER9 and newer processors should
+	 * have the right firmware bits.
 	 */
-	if (!pvr_version_is(PVR_POWER9)) {
+	if (pvr_version_is(PVR_POWER7) || pvr_version_is(PVR_POWER7p) ||
+	    pvr_version_is(PVR_POWER8E) || pvr_version_is(PVR_POWER8NVL) ||
+	    pvr_version_is(PVR_POWER8)) {
 		security_ftr_clear(SEC_FTR_L1D_FLUSH_ENTRY);
 		security_ftr_clear(SEC_FTR_L1D_FLUSH_UACCESS);
 	}
diff mbox series

Patch

diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index a8db3f153063..6ec67223f8c7 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -122,15 +122,6 @@  static void pnv_setup_security_mitigations(void)
 			type = L1D_FLUSH_ORI;
 	}
 
-	/*
-	 * If we are non-Power9 bare metal, we don't need to flush on kernel
-	 * entry or after user access: they fix a P9 specific vulnerability.
-	 */
-	if (!pvr_version_is(PVR_POWER9)) {
-		security_ftr_clear(SEC_FTR_L1D_FLUSH_ENTRY);
-		security_ftr_clear(SEC_FTR_L1D_FLUSH_UACCESS);
-	}
-
 	enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) && \
 		 (security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR)   || \
 		  security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV));