| 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
| ||
|
Message-ID: <6cda2827-af75-589d-3e43-a287d6683e7a@amd.com>
Date: Mon, 18 Apr 2022 20:24:35 -0700
From: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: x86@...nel.org, linux-edac@...r.kernel.org,
linux-kernel@...r.kernel.org, Tony Luck <tony.luck@...el.com>,
"H . Peter Anvin" <hpa@...or.com>,
Yazen Ghannam <yazen.ghannam@....com>
Subject: Re: [PATCH v4 1/2] x86/mce: Check for writes ignored in MCA_STATUS
register
On 4/6/2022 9:08 AM, Borislav Petkov wrote:
> On Mon, Feb 14, 2022 at 05:36:39PM -0600, Smita Koralahalli wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
>> index 5fbd7ffb3233..43ba63b7dc73 100644
>> --- a/arch/x86/kernel/cpu/mce/inject.c
>> +++ b/arch/x86/kernel/cpu/mce/inject.c
>> @@ -470,11 +470,36 @@ static void toggle_nb_mca_mst_cpu(u16 nid)
>> __func__, PCI_FUNC(F3->devfn), NBCFG);
>> }
>>
>> +struct mce_err_handler {
>> + struct mce *mce;
>> + int err;
>> +};
> Well, we already have a struct mce i_mce which serves as a sort-of
> global container for injection data which gets dumped into the hardware
> upon injection time.
>
> Now you're adding another struct which contains additional info and are
> adding a pointer to that i_mce.
>
> But this is not a clean design - when you do stuff like that you need to
> unify all those global-info-containing structs and make the code dealing
> with them straight-forward . I.e., if you don't look at the big picture
> of a design, it soon grows into an unwieldy mess which some poor sod
> would need to clean up afterwards and that poor sod is in most cases the
> maintainer.
>
> So I went and did that ontop of your patch, this is one possible way to
> do it but it.
>
> Here it is, a combined diff ontop of tip:ras/core. Please split it into
> patches: first patch converts to the new descriptor and then a second
> one adds the MCA_STATUS checking bits.
>
> There are a couple of other changes in there, if they're not clear, feel
> free to ask.
>
> Thx.
>
> ---
> diff --git a/arch/x86/kernel/cpu/mce/inject.c b/arch/x86/kernel/cpu/mce/inject.c
> index 43ba63b7dc73..0fd1eea2f754 100644
> --- a/arch/x86/kernel/cpu/mce/inject.c
> +++ b/arch/x86/kernel/cpu/mce/inject.c
> @@ -33,10 +33,14 @@
>
> #include "internal.h"
>
> -/*
> - * Collect all the MCi_XXX settings
> - */
> -static struct mce i_mce;
> +static bool hw_injection_possible = true;
> +
> +/* Collect all the MCi_XXX settings */
> +static struct inject_desc {
> + struct mce m;
> + int err;
> +} inj_desc;
> +
> static struct dentry *dfs_inj;
>
> #define MAX_FLAG_OPT_SIZE 4
> @@ -110,9 +114,11 @@ static int inj_ipid_set(void *data, u64 val)
>
> DEFINE_SIMPLE_ATTRIBUTE(ipid_fops, inj_ipid_get, inj_ipid_set, "%llx\n");
>
> -static void setup_inj_struct(struct mce *m)
> +static void setup_inj_struct(void)
> {
> - memset(m, 0, sizeof(struct mce));
> + struct mce *m = &inj_desc.m;
> +
> + memset(&inj_desc, 0, sizeof(struct inject_desc));
>
> m->cpuvendor = boot_cpu_data.x86_vendor;
> m->time = ktime_get_real_seconds();
> @@ -470,84 +476,78 @@ static void toggle_nb_mca_mst_cpu(u16 nid)
> __func__, PCI_FUNC(F3->devfn), NBCFG);
> }
>
> -struct mce_err_handler {
> - struct mce *mce;
> - int err;
> -};
> -
> -static struct mce_err_handler mce_err;
> -
> -static bool prepare_mca_status(struct mce *m)
> +static bool prepare_mca_status(void)
> {
> - u32 status_reg = mca_msr_reg(m->bank, MCA_STATUS);
> - u64 status_val = m->status;
> + u32 status_reg = mca_msr_reg(inj_desc.m.bank, MCA_STATUS);
> + u64 status_val = inj_desc.m.status;
>
> wrmsrl(status_reg, status_val);
> rdmsrl(status_reg, status_val);
>
> - return status_val;
> + return status_val == inj_desc.m.status;
> }
>
> -static void prepare_msrs(void *info)
> +static void prepare_msrs(void *unused)
> {
> - struct mce_err_handler *i_mce_err = ((struct mce_err_handler *)info);
> - struct mce m = *i_mce_err->mce;
> - u8 b = m.bank;
> + struct mce *m = &inj_desc.m;
> + u8 b = inj_desc.m.bank;
>
> - if (!prepare_mca_status(&m)) {
> + if (!prepare_mca_status()) {
> pr_err("Platform does not allow error injection, try using APEI EINJ instead.\n");
> - i_mce_err->err = -EINVAL;
> + inj_desc.err = -EINVAL;
> + hw_injection_possible = false;
> return;
> }
>
> - wrmsrl(MSR_IA32_MCG_STATUS, m.mcgstatus);
> + wrmsrl(MSR_IA32_MCG_STATUS, m->mcgstatus);
>
> if (boot_cpu_has(X86_FEATURE_SMCA)) {
> - if (m.inject_flags == DFR_INT_INJ) {
> - wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m.status);
> - wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m.addr);
> + if (m->inject_flags == DFR_INT_INJ) {
> + wrmsrl(MSR_AMD64_SMCA_MCx_DESTAT(b), m->status);
> + wrmsrl(MSR_AMD64_SMCA_MCx_DEADDR(b), m->addr);
> } else {
> - wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m.status);
> - wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m.addr);
> + wrmsrl(MSR_AMD64_SMCA_MCx_STATUS(b), m->status);
> + wrmsrl(MSR_AMD64_SMCA_MCx_ADDR(b), m->addr);
> }
>
> - wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m.misc);
> - wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m.synd);
> + wrmsrl(MSR_AMD64_SMCA_MCx_MISC(b), m->misc);
> + wrmsrl(MSR_AMD64_SMCA_MCx_SYND(b), m->synd);
> } else {
> - wrmsrl(MSR_IA32_MCx_STATUS(b), m.status);
> - wrmsrl(MSR_IA32_MCx_ADDR(b), m.addr);
> - wrmsrl(MSR_IA32_MCx_MISC(b), m.misc);
> + wrmsrl(MSR_IA32_MCx_STATUS(b), m->status);
> + wrmsrl(MSR_IA32_MCx_ADDR(b), m->addr);
> + wrmsrl(MSR_IA32_MCx_MISC(b), m->misc);
> }
> }
>
> -static void do_inject(void)
> +static int do_inject(void)
> {
> + struct mce *m = &inj_desc.m;
> + unsigned int cpu = m->extcpu;
> u64 mcg_status = 0;
> - unsigned int cpu = i_mce.extcpu;
> - u8 b = i_mce.bank;
> + u8 b = m->bank;
>
> - mce_err.mce = &i_mce;
> - mce_err.err = 0;
> + m->tsc = rdtsc_ordered();
>
> - i_mce.tsc = rdtsc_ordered();
> + m->status |= MCI_STATUS_VAL;
>
> - i_mce.status |= MCI_STATUS_VAL;
> + if (m->misc)
> + m->status |= MCI_STATUS_MISCV;
>
> - if (i_mce.misc)
> - i_mce.status |= MCI_STATUS_MISCV;
> -
> - if (i_mce.synd)
> - i_mce.status |= MCI_STATUS_SYNDV;
> + if (m->synd)
> + m->status |= MCI_STATUS_SYNDV;
>
> if (inj_type == SW_INJ) {
> - mce_log(&i_mce);
> - return;
> + mce_log(m);
> + return 0;
> }
>
> + if (!hw_injection_possible)
> + return -EINVAL;
Why are we checking this here? This flag (hw_injection_possible)
is set to false inside prepare_msrs() called from
smp_call_function_single().
Should this check be done after the call to smp_call_function_single()?
Also, we already have inj_desc.err which returns error code to userspace
when WRIG in status registers. Why is this flag needed?
Thanks,
Smita
> +
> /* prep MCE global settings for the injection */
> mcg_status = MCG_STATUS_MCIP | MCG_STATUS_EIPV;
>
> - if (!(i_mce.status & MCI_STATUS_PCC))
> + if (!(m->status & MCI_STATUS_PCC))
> mcg_status |= MCG_STATUS_RIPV;
>
> /*
> @@ -556,8 +556,8 @@ static void do_inject(void)
> * - MCx_STATUS[UC] cleared: deferred errors are _not_ UC
> */
> if (inj_type == DFR_INT_INJ) {
> - i_mce.status |= MCI_STATUS_DEFERRED;
> - i_mce.status &= ~MCI_STATUS_UC;
> + m->status |= MCI_STATUS_DEFERRED;
> + m->status &= ~MCI_STATUS_UC;
> }
>
> /*
> @@ -578,13 +578,13 @@ static void do_inject(void)
>
> toggle_hw_mce_inject(cpu, true);
>
> - i_mce.mcgstatus = mcg_status;
> - i_mce.inject_flags = inj_type;
> - smp_call_function_single(cpu, prepare_msrs, &mce_err, 0);
> + m->mcgstatus = mcg_status;
> + m->inject_flags = inj_type;
> + smp_call_function_single(cpu, prepare_msrs, NULL, 0);
>
> toggle_hw_mce_inject(cpu, false);
>
> - if (mce_err.err)
> + if (inj_desc.err)
> goto err;
>
> switch (inj_type) {
> @@ -601,6 +601,7 @@ static void do_inject(void)
> err:
> cpus_read_unlock();
>
> + return inj_desc.err;
> }
>
> /*
> @@ -611,6 +612,7 @@ static int inj_bank_set(void *data, u64 val)
> {
> struct mce *m = (struct mce *)data;
> u8 n_banks;
> + int err;
> u64 cap;
>
> /* Get bank count on target CPU so we can handle non-uniform values. */
> @@ -650,12 +652,12 @@ static int inj_bank_set(void *data, u64 val)
> }
>
> inject:
> - do_inject();
> + err = do_inject();
>
> /* Reset injection struct */
> - setup_inj_struct(&i_mce);
> + setup_inj_struct();
>
> - return mce_err.err;
> + return err;
> }
>
> MCE_INJECT_GET(bank);
> @@ -745,7 +747,7 @@ static void __init debugfs_init(void)
>
> for (i = 0; i < ARRAY_SIZE(dfs_fls); i++)
> debugfs_create_file(dfs_fls[i].name, dfs_fls[i].perm, dfs_inj,
> - &i_mce, dfs_fls[i].fops);
> + &inj_desc.m, dfs_fls[i].fops);
> }
>
> static int __init inject_init(void)
> @@ -758,7 +760,7 @@ static int __init inject_init(void)
> register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, "mce_notify");
> mce_register_injector_chain(&inject_nb);
>
> - setup_inj_struct(&i_mce);
> + setup_inj_struct();
>
> pr_info("Machine check injector initialized\n");
>
> diff --git a/arch/x86/kernel/cpu/mce/internal.h b/arch/x86/kernel/cpu/mce/internal.h
> index 4ae0e603f7fa..7e03f5b7f6bd 100644
> --- a/arch/x86/kernel/cpu/mce/internal.h
> +++ b/arch/x86/kernel/cpu/mce/internal.h
> @@ -211,7 +211,7 @@ noinstr u64 mce_rdmsrl(u32 msr);
>
> static __always_inline u32 mca_msr_reg(int bank, enum mca_msr reg)
> {
> - if (mce_flags.smca) {
> + if (cpu_feature_enabled(X86_FEATURE_SMCA)) {
> switch (reg) {
> case MCA_CTL: return MSR_AMD64_SMCA_MCx_CTL(bank);
> case MCA_ADDR: return MSR_AMD64_SMCA_MCx_ADDR(bank);
>
Powered by blists - more mailing lists