| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Feb 2022 08:03:24 -0800
From: Jue Wang <juew@...gle.com>
To: Borislav Petkov <bp@...en8.de>
Cc: Tony Luck <tony.luck@...el.com>, x86@...nel.org,
linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH v2] x86/mce: work around an erratum on fast string copy instructions.
On Fri, Feb 18, 2022 at 7:07 AM Borislav Petkov <bp@...en8.de> wrote:
>
> On Thu, Feb 17, 2022 at 05:32:09PM -0800, Jue Wang wrote:
> > +static noinstr bool quirk_skylake_repmov(void)
> > +{
> > + u64 mcgstatus = mce_rdmsrl(MSR_IA32_MCG_STATUS);
> > + u64 misc_enable = __rdmsr(MSR_IA32_MISC_ENABLE);
> > +
> > + /*
> > + * Apply the quirk only to local machine checks, i.e., no broadcast
> > + * sync is needed.
> > + */
> > + if ((mcgstatus & MCG_STATUS_LMCES) &&
> > + unlikely(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) {
> > + u64 mc1_status = mce_rdmsrl(MSR_IA32_MCx_STATUS(1));
> > +
> > + /* Check for a software-recoverable data fetch error. */
> > + if ((mc1_status &
> > + (MCI_STATUS_VAL | MCI_STATUS_OVER | MCI_STATUS_UC | MCI_STATUS_EN |
> > + MCI_STATUS_ADDRV | MCI_STATUS_MISCV | MCI_STATUS_PCC |
> > + MCI_STATUS_AR | MCI_STATUS_S)) ==
> > + (MCI_STATUS_VAL | MCI_STATUS_UC | MCI_STATUS_EN |
> > + MCI_STATUS_ADDRV | MCI_STATUS_MISCV |
> > + MCI_STATUS_AR | MCI_STATUS_S)) {
> > + misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> > + __wrmsr(MSR_IA32_MISC_ENABLE,
> > + (u32)misc_enable, (u32)(misc_enable >> 32));
>
> "You're going to have to use the mce_{rd,wr}msrl() routines."
>
> I actually really meant that.
Thanks.
Since MSR_IA32_MISC_ENABLE is not a MCA register, I wonder if we want
to mix its read/write with the injected MCE code. I was a bit concerned about
potential race with mce-inject and the read/write to MSR_IA32_MISC_ENABLE.
>
> And I went and simplified this a bit more so that it is more readable,
> diff ontop.
>
> Also, Tony, I think the clearing of MCG_STATUS should happen last.
>
> ---
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index c1a41da99975..1741be9b9464 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -831,34 +831,35 @@ quirk_sandybridge_ifu(int bank, struct mce *m, struct pt_regs *regs)
> */
> static noinstr bool quirk_skylake_repmov(void)
> {
> - u64 mcgstatus = mce_rdmsrl(MSR_IA32_MCG_STATUS);
> - u64 misc_enable = __rdmsr(MSR_IA32_MISC_ENABLE);
> + u64 mcgstatus = mce_rdmsrl(MSR_IA32_MCG_STATUS);
> + u64 misc_enable = mce_rdmsrl(MSR_IA32_MISC_ENABLE);
> + u64 mc1_status;
>
> /*
> * Apply the quirk only to local machine checks, i.e., no broadcast
> * sync is needed.
> */
> - if ((mcgstatus & MCG_STATUS_LMCES) &&
> - (misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) {
> - u64 mc1_status = mce_rdmsrl(MSR_IA32_MCx_STATUS(1));
> -
> - /* Check for a software-recoverable data fetch error. */
> - if ((mc1_status &
> - (MCI_STATUS_VAL | MCI_STATUS_OVER | MCI_STATUS_UC | MCI_STATUS_EN |
> - MCI_STATUS_ADDRV | MCI_STATUS_MISCV | MCI_STATUS_PCC |
> - MCI_STATUS_AR | MCI_STATUS_S)) ==
> - (MCI_STATUS_VAL | MCI_STATUS_UC | MCI_STATUS_EN |
> - MCI_STATUS_ADDRV | MCI_STATUS_MISCV |
> - MCI_STATUS_AR | MCI_STATUS_S)) {
> - misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> - __wrmsr(MSR_IA32_MISC_ENABLE,
> - (u32)misc_enable, (u32)(misc_enable >> 32));
> - mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
> - mce_wrmsrl(MSR_IA32_MCx_STATUS(1), 0);
> - pr_err_once("Errata detected, disable fast string copy instructions.\n");
> - return true;
> - }
> + if (!(mcgstatus & MCG_STATUS_LMCES) ||
> + !(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING))
> + return false;
> +
> + mc1_status = mce_rdmsrl(MSR_IA32_MCx_STATUS(1));
> +
> + /* Check for a software-recoverable data fetch error. */
> + if ((mc1_status &
> + (MCI_STATUS_VAL | MCI_STATUS_OVER | MCI_STATUS_UC | MCI_STATUS_EN |
> + MCI_STATUS_ADDRV | MCI_STATUS_MISCV | MCI_STATUS_PCC |
> + MCI_STATUS_AR | MCI_STATUS_S)) ==
> + (MCI_STATUS_VAL | MCI_STATUS_UC | MCI_STATUS_EN |
> + MCI_STATUS_ADDRV | MCI_STATUS_MISCV |
> + MCI_STATUS_AR | MCI_STATUS_S)) {
> + misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> + mce_wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> + mce_wrmsrl(MSR_IA32_MCx_STATUS(1), 0);
> + pr_err_once("Erratum detected, disable fast string copy instructions.\n");
> + return true;
> }
> +
> return false;
> }
>
> @@ -1432,7 +1433,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
> return unexpected_machine_check(regs);
>
> if (mce_flags.skx_repmov_quirk && quirk_skylake_repmov())
> - return;
> + goto clear;
>
> /*
> * Establish sequential order between the CPUs entering the machine
> @@ -1576,6 +1577,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
> out:
> instrumentation_end();
>
> +clear:
> mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
> }
> EXPORT_SYMBOL_GPL(do_machine_check);
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists