* [PATCH] powerpc/security: Fix spectre_v2 reporting
@ 2019-03-21 4:24 Michael Ellerman
2019-03-21 6:29 ` Michael Neuling
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Michael Ellerman @ 2019-03-21 4:24 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, diana.craciun, andrew.donnellan, msuchanek, dja
When I updated the spectre_v2 reporting to handle software count cache
flush I got the logic wrong when there's no software count cache
enabled at all.
The result is that on systems with the software count cache flush
disabled we print:
Mitigation: Indirect branch cache disabled, Software count cache flush
Which correctly indicates that the count cache is disabled, but
incorrectly says the software count cache flush is enabled.
The root of the problem is that we are trying to handle all
combinations of options. But we know now that we only expect to see
the software count cache flush enabled if the other options are false.
So split the two cases, which simplifies the logic and fixes the bug.
We were also missing a space before "(hardware accelerated)".
The result is we see one of:
Mitigation: Indirect branch serialisation (kernel only)
Mitigation: Indirect branch cache disabled
Mitigation: Software count cache flush
Mitigation: Software count cache flush (hardware accelerated)
Fixes: ee13cb249fab ("powerpc/64s: Add support for software count cache flush")
Cc: stable@vger.kernel.org # v4.19+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/security.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 9b8631533e02..b33bafb8fcea 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -190,29 +190,22 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
bcs = security_ftr_enabled(SEC_FTR_BCCTRL_SERIALISED);
ccd = security_ftr_enabled(SEC_FTR_COUNT_CACHE_DISABLED);
- if (bcs || ccd || count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
- bool comma = false;
+ if (bcs || ccd) {
seq_buf_printf(&s, "Mitigation: ");
- if (bcs) {
+ if (bcs)
seq_buf_printf(&s, "Indirect branch serialisation (kernel only)");
- comma = true;
- }
- if (ccd) {
- if (comma)
- seq_buf_printf(&s, ", ");
- seq_buf_printf(&s, "Indirect branch cache disabled");
- comma = true;
- }
-
- if (comma)
+ if (bcs && ccd)
seq_buf_printf(&s, ", ");
- seq_buf_printf(&s, "Software count cache flush");
+ if (ccd)
+ seq_buf_printf(&s, "Indirect branch cache disabled");
+ } else if (count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
+ seq_buf_printf(&s, "Mitigation: Software count cache flush");
if (count_cache_flush_type == COUNT_CACHE_FLUSH_HW)
- seq_buf_printf(&s, "(hardware accelerated)");
+ seq_buf_printf(&s, " (hardware accelerated)");
} else if (btb_flush_enabled) {
seq_buf_printf(&s, "Mitigation: Branch predictor state flush");
} else {
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/security: Fix spectre_v2 reporting
2019-03-21 4:24 [PATCH] powerpc/security: Fix spectre_v2 reporting Michael Ellerman
@ 2019-03-21 6:29 ` Michael Neuling
2019-03-21 9:59 ` Diana Madalina Craciun
2019-03-22 12:21 ` Michael Ellerman
2 siblings, 0 replies; 4+ messages in thread
From: Michael Neuling @ 2019-03-21 6:29 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
Cc: diana.craciun, msuchanek, andrew.donnellan, dja
On Thu, 2019-03-21 at 15:24 +1100, Michael Ellerman wrote:
> When I updated the spectre_v2 reporting to handle software count cache
> flush I got the logic wrong when there's no software count cache
> enabled at all.
>
> The result is that on systems with the software count cache flush
> disabled we print:
>
> Mitigation: Indirect branch cache disabled, Software count cache flush
>
> Which correctly indicates that the count cache is disabled, but
> incorrectly says the software count cache flush is enabled.
>
> The root of the problem is that we are trying to handle all
> combinations of options. But we know now that we only expect to see
> the software count cache flush enabled if the other options are false.
>
> So split the two cases, which simplifies the logic and fixes the bug.
> We were also missing a space before "(hardware accelerated)".
>
> The result is we see one of:
>
> Mitigation: Indirect branch serialisation (kernel only)
> Mitigation: Indirect branch cache disabled
> Mitigation: Software count cache flush
> Mitigation: Software count cache flush (hardware accelerated)
>
> Fixes: ee13cb249fab ("powerpc/64s: Add support for software count cache flush")
> Cc: stable@vger.kernel.org # v4.19+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
LGTM
Reviewed-by: Michael Neuling <mikey@neuling.org>
> ---
> arch/powerpc/kernel/security.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index 9b8631533e02..b33bafb8fcea 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -190,29 +190,22 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
> bcs = security_ftr_enabled(SEC_FTR_BCCTRL_SERIALISED);
> ccd = security_ftr_enabled(SEC_FTR_COUNT_CACHE_DISABLED);
>
> - if (bcs || ccd || count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
> - bool comma = false;
> + if (bcs || ccd) {
> seq_buf_printf(&s, "Mitigation: ");
>
> - if (bcs) {
> + if (bcs)
> seq_buf_printf(&s, "Indirect branch serialisation (kernel only)");
> - comma = true;
> - }
>
> - if (ccd) {
> - if (comma)
> - seq_buf_printf(&s, ", ");
> - seq_buf_printf(&s, "Indirect branch cache disabled");
> - comma = true;
> - }
> -
> - if (comma)
> + if (bcs && ccd)
> seq_buf_printf(&s, ", ");
>
> - seq_buf_printf(&s, "Software count cache flush");
> + if (ccd)
> + seq_buf_printf(&s, "Indirect branch cache disabled");
> + } else if (count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
> + seq_buf_printf(&s, "Mitigation: Software count cache flush");
>
> if (count_cache_flush_type == COUNT_CACHE_FLUSH_HW)
> - seq_buf_printf(&s, "(hardware accelerated)");
> + seq_buf_printf(&s, " (hardware accelerated)");
> } else if (btb_flush_enabled) {
> seq_buf_printf(&s, "Mitigation: Branch predictor state flush");
> } else {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/security: Fix spectre_v2 reporting
2019-03-21 4:24 [PATCH] powerpc/security: Fix spectre_v2 reporting Michael Ellerman
2019-03-21 6:29 ` Michael Neuling
@ 2019-03-21 9:59 ` Diana Madalina Craciun
2019-03-22 12:21 ` Michael Ellerman
2 siblings, 0 replies; 4+ messages in thread
From: Diana Madalina Craciun @ 2019-03-21 9:59 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev@ozlabs.org
Cc: mikey@neuling.org, msuchanek@suse.de,
andrew.donnellan@au1.ibm.com, dja@axtens.net
Reviewed-by: Diana Craciun <diana.craciun@nxp.com>
On 3/21/2019 6:24 AM, Michael Ellerman wrote:
> When I updated the spectre_v2 reporting to handle software count cache
> flush I got the logic wrong when there's no software count cache
> enabled at all.
>
> The result is that on systems with the software count cache flush
> disabled we print:
>
> Mitigation: Indirect branch cache disabled, Software count cache flush
>
> Which correctly indicates that the count cache is disabled, but
> incorrectly says the software count cache flush is enabled.
>
> The root of the problem is that we are trying to handle all
> combinations of options. But we know now that we only expect to see
> the software count cache flush enabled if the other options are false.
>
> So split the two cases, which simplifies the logic and fixes the bug.
> We were also missing a space before "(hardware accelerated)".
>
> The result is we see one of:
>
> Mitigation: Indirect branch serialisation (kernel only)
> Mitigation: Indirect branch cache disabled
> Mitigation: Software count cache flush
> Mitigation: Software count cache flush (hardware accelerated)
>
> Fixes: ee13cb249fab ("powerpc/64s: Add support for software count cache flush")
> Cc: stable@vger.kernel.org # v4.19+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kernel/security.c | 23 ++++++++---------------
> 1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index 9b8631533e02..b33bafb8fcea 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -190,29 +190,22 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
> bcs = security_ftr_enabled(SEC_FTR_BCCTRL_SERIALISED);
> ccd = security_ftr_enabled(SEC_FTR_COUNT_CACHE_DISABLED);
>
> - if (bcs || ccd || count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
> - bool comma = false;
> + if (bcs || ccd) {
> seq_buf_printf(&s, "Mitigation: ");
>
> - if (bcs) {
> + if (bcs)
> seq_buf_printf(&s, "Indirect branch serialisation (kernel only)");
> - comma = true;
> - }
>
> - if (ccd) {
> - if (comma)
> - seq_buf_printf(&s, ", ");
> - seq_buf_printf(&s, "Indirect branch cache disabled");
> - comma = true;
> - }
> -
> - if (comma)
> + if (bcs && ccd)
> seq_buf_printf(&s, ", ");
>
> - seq_buf_printf(&s, "Software count cache flush");
> + if (ccd)
> + seq_buf_printf(&s, "Indirect branch cache disabled");
> + } else if (count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
> + seq_buf_printf(&s, "Mitigation: Software count cache flush");
>
> if (count_cache_flush_type == COUNT_CACHE_FLUSH_HW)
> - seq_buf_printf(&s, "(hardware accelerated)");
> + seq_buf_printf(&s, " (hardware accelerated)");
> } else if (btb_flush_enabled) {
> seq_buf_printf(&s, "Mitigation: Branch predictor state flush");
> } else {
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: powerpc/security: Fix spectre_v2 reporting
2019-03-21 4:24 [PATCH] powerpc/security: Fix spectre_v2 reporting Michael Ellerman
2019-03-21 6:29 ` Michael Neuling
2019-03-21 9:59 ` Diana Madalina Craciun
@ 2019-03-22 12:21 ` Michael Ellerman
2 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2019-03-22 12:21 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
Cc: diana.craciun, mikey, msuchanek, andrew.donnellan, dja
On Thu, 2019-03-21 at 04:24:33 UTC, Michael Ellerman wrote:
> When I updated the spectre_v2 reporting to handle software count cache
> flush I got the logic wrong when there's no software count cache
> enabled at all.
>
> The result is that on systems with the software count cache flush
> disabled we print:
>
> Mitigation: Indirect branch cache disabled, Software count cache flush
>
> Which correctly indicates that the count cache is disabled, but
> incorrectly says the software count cache flush is enabled.
>
> The root of the problem is that we are trying to handle all
> combinations of options. But we know now that we only expect to see
> the software count cache flush enabled if the other options are false.
>
> So split the two cases, which simplifies the logic and fixes the bug.
> We were also missing a space before "(hardware accelerated)".
>
> The result is we see one of:
>
> Mitigation: Indirect branch serialisation (kernel only)
> Mitigation: Indirect branch cache disabled
> Mitigation: Software count cache flush
> Mitigation: Software count cache flush (hardware accelerated)
>
> Fixes: ee13cb249fab ("powerpc/64s: Add support for software count cache flush")
> Cc: stable@vger.kernel.org # v4.19+
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Michael Neuling <mikey@neuling.org>
> Reviewed-by: Diana Craciun <diana.craciun@nxp.com>
Applied to powerpc fixes.
https://git.kernel.org/powerpc/c/92edf8df0ff2ae86cc632eeca0e651fd
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-22 12:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-03-21 4:24 [PATCH] powerpc/security: Fix spectre_v2 reporting Michael Ellerman
2019-03-21 6:29 ` Michael Neuling
2019-03-21 9:59 ` Diana Madalina Craciun
2019-03-22 12:21 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox