[v3,2/2] nfit, mce: validate the mce->addr before using it

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> I recommended the split so the fixes can be tracked and / or reverted
> independently if they cause problems.

Do you have anything concrete in mind that might go wrong or just a
general cautiousness?

Or do you think the hw might not spit what mce_usable_address() is
checking for, for NVDIMM MCEs, so that you'd like to be able to revert
that second patch?

Thx.

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Tue, Nov 06, 2018 at 10:02:46AM -0800, Dan Williams wrote:
> Just general cautiousness, I'm not opposed to squashing them.

Nah, I can keep them separate - I was just wondering why.

> mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM
> MCEs.

Ok.

Thx.

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 9:53 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Nov 06, 2018 at 08:20:38AM -0800, Dan Williams wrote:
> > I recommended the split so the fixes can be tracked and / or reverted
> > independently if they cause problems.
>
> Do you have anything concrete in mind that might go wrong or just a
> general cautiousness?

Just general cautiousness, I'm not opposed to squashing them.

> Or do you think the hw might not spit what mce_usable_address() is
> checking for, for NVDIMM MCEs, so that you'd like to be able to revert
> that second patch?

mce_usable_address() should not have any sensitivity to NVDIMM vs DRAM MCEs.

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Dan Williams]

On Tue, Nov 6, 2018 at 6:51 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> > The NFIT machine check handler uses the physical address from the 'mce'
> > structure, and compares it against information in the ACPI NFIT table to
> > determine whether that location lies on an NVDIMM. The mce->addr field
> > however may not always be valid, and this is indicated by the
> > MCI_STATUS_ADDRV bit in the status field.
> >
> > Export mce_usable_address() which already performs validation for the
> > address, and use it in the NFIT handler.
> >
> > Reported-by: Robert Elliott <elliott@hpe.com>
> > Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> > Cc: stable@vger.kernel.org
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> > ---
> >  arch/x86/include/asm/mce.h       | 1 +
> >  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
> >  drivers/acpi/nfit/mce.c          | 4 ++++
> >  3 files changed, 7 insertions(+), 1 deletion(-)
>
> Is there any particular reason why is this a separate patch and not part
> of the first one?

I recommended the split so the fixes can be tracked and / or reverted
independently if they cause problems.

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Borislav Petkov]

On Thu, Oct 25, 2018 at 06:37:29PM -0600, Vishal Verma wrote:
> The NFIT machine check handler uses the physical address from the 'mce'
> structure, and compares it against information in the ACPI NFIT table to
> determine whether that location lies on an NVDIMM. The mce->addr field
> however may not always be valid, and this is indicated by the
> MCI_STATUS_ADDRV bit in the status field.
> 
> Export mce_usable_address() which already performs validation for the
> address, and use it in the NFIT handler.
> 
> Reported-by: Robert Elliott <elliott@hpe.com>
> Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
> Cc: stable@vger.kernel.org
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  arch/x86/include/asm/mce.h       | 1 +
>  arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
>  drivers/acpi/nfit/mce.c          | 4 ++++
>  3 files changed, 7 insertions(+), 1 deletion(-)

Is there any particular reason why is this a separate patch and not part
of the first one?

Also, do s/mce/MCE/g.

Thx.

[v3,2/2] nfit, mce: validate the mce->addr before using it

[Vishal Verma]

The NFIT machine check handler uses the physical address from the 'mce'
structure, and compares it against information in the ACPI NFIT table to
determine whether that location lies on an NVDIMM. The mce->addr field
however may not always be valid, and this is indicated by the
MCI_STATUS_ADDRV bit in the status field.

Export mce_usable_address() which already performs validation for the
address, and use it in the NFIT handler.

Reported-by: Robert Elliott <elliott@hpe.com>
Fixes: 6839a6d96f4e ("nfit: do an ARS scrub on hitting a latent media error")
Cc: stable@vger.kernel.org
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 arch/x86/include/asm/mce.h       | 1 +
 arch/x86/kernel/cpu/mcheck/mce.c | 3 ++-
 drivers/acpi/nfit/mce.c          | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

v3: Add this patch to address the address validation (Robert)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 3111b3cee2ee..eb786f90f2d3 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -217,6 +217,7 @@ static inline int umc_normaddr_to_sysaddr(u64 norm_addr, u16 nid, u8 umc, u64 *s
 int mce_available(struct cpuinfo_x86 *c);
 bool mce_is_memory_error(struct mce *m);
 bool mce_is_correctable(struct mce *m);
+int mce_usable_address(struct mce *m);
 
 DECLARE_PER_CPU(unsigned, mce_exception_count);
 DECLARE_PER_CPU(unsigned, mce_poll_count);
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 27015948bc41..cdbedeb3f3db 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -485,7 +485,7 @@ static void mce_report_event(struct pt_regs *regs)
  * be somewhat complicated (e.g. segment offset would require an instruction
  * parser). So only support physical addresses up to page granuality for now.
  */
-static int mce_usable_address(struct mce *m)
+int mce_usable_address(struct mce *m)
 {
 	if (!(m->status & MCI_STATUS_ADDRV))
 		return 0;
@@ -505,6 +505,7 @@ static int mce_usable_address(struct mce *m)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(mce_usable_address);
 
 bool mce_is_memory_error(struct mce *m)
 {
diff --git a/drivers/acpi/nfit/mce.c b/drivers/acpi/nfit/mce.c
index 7a51707f87e9..d943ed50f0b4 100644
--- a/drivers/acpi/nfit/mce.c
+++ b/drivers/acpi/nfit/mce.c
@@ -29,6 +29,10 @@ static int nfit_handle_mce(struct notifier_block *nb, unsigned long val,
 	if (!mce_is_memory_error(mce) || mce_is_correctable(mce))
 		return NOTIFY_DONE;
 
+	/* Verify the address reported in the mce is valid */
+	if (!mce_usable_address(mce))
+		return NOTIFY_DONE;
+
 	/*
 	 * mce->addr contains the physical addr accessed that caused the
 	 * machine check. We need to walk through the list of NFITs, and see