| 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: <Y7XgdFOEfHei9xEK@yaz-fattaah>
Date: Wed, 4 Jan 2023 20:24:20 +0000
From: Yazen Ghannam <yazen.ghannam@....com>
To: Tony Luck <tony.luck@...el.com>
Cc: Borislav Petkov <bp@...en8.de>,
Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>,
x86@...nel.org, linux-kernel@...r.kernel.org,
Isaku Yamahata <isaku.yamahata@...el.com>,
Fan Du <fan.du@...el.com>
Subject: Re: [PATCH] x86/mce: Mask out non-address bits from machine check
bank
On Tue, Jan 03, 2023 at 02:34:16PM -0800, Tony Luck wrote:
> Systems that support various memory encryption schemes (MKTME, TDX, SEV)
> use high order physical address bits to indicate which key should be
> used for a specific memory location.
>
> When a memory error is reported, some systems may report those key
> bits in the IA32_MCi_ADDR machine check MSR. This is legitimate because
> the Intel SDM has a footnote for the contents of the address register
> that says: "Useful bits in this field depend on the address methodology
> in use when the register state is saved."
>
> Note: I don't know if any AMD systems include key bits in the reported
> address, if they do, they also need this fix. If not, it is harmless.
>
The following note is in the description of the MCA_ADDR[ErrorAddr] field in
the AMD Processor Programming Reference.
For physical addresses, the most significant bit is given by
Core::X86::Cpuid::LongModeInfo[PhysAddrSize].
And I see that x86_phys_bits does get fixed up in early_detect_mem_encrypt().
I'm not sure if key bits are included in the reported address, or if the HW
automatically masks them off. But in any case, I think this patch is valid as
you stated above.
> Add a new #define MCI_ADDR_PHYSADDR for the mask of valid physical
> address bits within the machine check bank address register. Use this
> mask for recoverable machine check handling and in the EDAC driver to
> ignore any key bits that may be present.
>
> [Credit: fix is based on those proposed by Fan Du and Isaku Yamahata]
>
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> Reported-by: Isaku Yamahata <isaku.yamahata@...el.com>
> Reported-by: Fan Du <fan.du@...el.com>
> ---
> arch/x86/include/asm/mce.h | 3 +++
> arch/x86/kernel/cpu/mce/core.c | 14 +++++++++-----
> drivers/edac/skx_common.c | 2 +-
> 3 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 6e986088817d..a8eef87fb12a 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -88,6 +88,9 @@
> #define MCI_MISC_ADDR_MEM 3 /* memory address */
> #define MCI_MISC_ADDR_GENERIC 7 /* generic */
>
> +/* MCi_ADDR register defines */
> +#define MCI_ADDR_PHYSADDR GENMASK(boot_cpu_data.x86_phys_bits - 1, 0)
Should this use GENMASK_ULL in case we're running in 32-bit mode?
> +
> /* CTL2 register defines */
> #define MCI_CTL2_CMCI_EN BIT_ULL(30)
> #define MCI_CTL2_CMCI_THRESHOLD_MASK 0x7fffULL
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 2c8ec5c71712..949705bdb2f3 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -579,7 +579,7 @@ static int uc_decode_notifier(struct notifier_block *nb, unsigned long val,
> mce->severity != MCE_DEFERRED_SEVERITY)
> return NOTIFY_DONE;
>
> - pfn = mce->addr >> PAGE_SHIFT;
> + pfn = (mce->addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
> if (!memory_failure(pfn, 0)) {
> set_mce_nospec(pfn);
> mce->kflags |= MCE_HANDLED_UC;
> @@ -1308,6 +1308,7 @@ static void kill_me_maybe(struct callback_head *cb)
> {
> struct task_struct *p = container_of(cb, struct task_struct, mce_kill_me);
> int flags = MF_ACTION_REQUIRED;
> + unsigned long pfn;
> int ret;
>
> p->mce_count = 0;
> @@ -1316,9 +1317,10 @@ static void kill_me_maybe(struct callback_head *cb)
> if (!p->mce_ripv)
> flags |= MF_MUST_KILL;
>
> - ret = memory_failure(p->mce_addr >> PAGE_SHIFT, flags);
> + pfn = (p->mce_addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
> + ret = memory_failure(pfn, flags);
> if (!ret) {
> - set_mce_nospec(p->mce_addr >> PAGE_SHIFT);
> + set_mce_nospec(pfn);
> sync_core();
> return;
> }
> @@ -1340,11 +1342,13 @@ static void kill_me_maybe(struct callback_head *cb)
> static void kill_me_never(struct callback_head *cb)
> {
> struct task_struct *p = container_of(cb, struct task_struct, mce_kill_me);
> + unsigned long pfn;
>
> p->mce_count = 0;
> pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);
> - if (!memory_failure(p->mce_addr >> PAGE_SHIFT, 0))
> - set_mce_nospec(p->mce_addr >> PAGE_SHIFT);
> + pfn = (p->mce_addr & MCI_ADDR_PHYSADDR) >> PAGE_SHIFT;
> + if (!memory_failure(pfn, 0))
> + set_mce_nospec(pfn);
> }
>
> static void queue_task_work(struct mce *m, char *msg, void (*func)(struct callback_head *))
> diff --git a/drivers/edac/skx_common.c b/drivers/edac/skx_common.c
> index f0f8e98f6efb..806986f03177 100644
> --- a/drivers/edac/skx_common.c
> +++ b/drivers/edac/skx_common.c
> @@ -657,7 +657,7 @@ int skx_mce_check_error(struct notifier_block *nb, unsigned long val,
>
> memset(&res, 0, sizeof(res));
> res.mce = mce;
> - res.addr = mce->addr;
> + res.addr = mce->addr & MCI_ADDR_PHYSADDR;
>
> /* Try driver decoder first */
> if (!(driver_decode && driver_decode(&res))) {
> --
The address decode in the AMD64 EDAC module operates on a non-physical
address, so this update is not needed there. The MCE recovery changes look good
to me.
Reviewed-by: Yazen Ghannam <yazen.ghannam@....com>
Thanks,
Yazen
Powered by blists - more mailing lists