[PATCH RFC] function probe_roms accessing improper addresses on UEFI systems

[PATCH RFC] function probe_roms accessing improper addresses on UEFI systems

[Randy Wright]

The code in function probe_roms accesses the legacy adapter space (in
the physical address range 640k-1mb) without regard to how this range is
described in the memory map. The assumption in the code is that the
result of an access to unpopulated space is a soft fail, returning -1.
This assumption is not universally true; some systems may cause a hard
error (MCE) when accessing an unpopulated physical address.

The following proposed patch takes advantage of the fact that on EFI
systems, the memory map provides a better description of the physical
space than on pre-EFI legacy systems. If the efi_enabled state variable
indicates the kernel is running on an UEFI system, the patch will use
information from the UEFI memory map so as not to access addresses that
should avoided according to the UEFI specification.

See also a discussion underway in the context of a patch to devmem_is_allowed 
  https://lkml.org/lkml/2012/10/2/458
which has the same overall goal of restricting access to 
inappropriate ranges of the EFI memory map.

Following is my proposed patch based on v3.6. I would appreciate input
on the following points.  

1. Is the implementation properly interpreting the UEFI specification by
disallowing probes of addresses for which the type is EFI_RESERVED_TYPE,
EFI_UNUSABLE_MEMORY, or otherwise unknown?

2. The patch includes some simple test cases in function test_probes
that can be customized to the testing needs of a particular target
system.  This test code is conditionally enabled based on the
pre-processor symbol DO_TEST_PROBES; if that is not defined, the body of
function test_probes() is empty.  This code can be enabled and modified
as needed to test specific addresses of interest.  Is this code useful
to include in an upstream patch?  If not, is there a more generally
useful testing strategy?

Please CC me directly with comments or questions.

Signed-off-by: Randy Wright <rwright@hp.com>

---
 arch/x86/kernel/probe_roms.c |   71 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c
index d5f15c3..76c20c9 100644
--- a/arch/x86/kernel/probe_roms.c
+++ b/arch/x86/kernel/probe_roms.c
@@ -11,6 +11,7 @@
 #include <linux/pfn.h>
 #include <linux/pci.h>
 #include <linux/export.h>
+#include <linux/efi.h>		/* efi_enabled, efi_mem_type */
 
 #include <asm/probe_roms.h>
 #include <asm/pci-direct.h>
@@ -177,11 +178,46 @@ EXPORT_SYMBOL(pci_biosrom_size);
 
 #define ROMSIGNATURE 0xaa55
 
+/*
+ * efi_paddr_valid returns 1 if the input physical address
+ * is valid in the efi memory map, 0 otherwise.
+ */
+static int __init efi_paddr_valid(phys_addr_t paddr)
+{
+	u32 type = efi_mem_type((unsigned long)paddr);
+	printk(KERN_DEBUG "efi_paddr_valid(0x%lx): type 0x%lx\n",
+			(unsigned long)paddr, (unsigned long)type);
+	if (type == EFI_RESERVED_TYPE
+	||  type == EFI_UNUSABLE_MEMORY
+	||  type >= EFI_MAX_MEMORY_TYPE)
+		return 0;
+	return 1;
+}
+
+/*
+ * efi_vaddr_valid translates itsinput va to a pa, then
+ * returns the result of efi_paddr_valid applied to that pa.
+ */
+
+static int __init efi_vaddr_valid(const unsigned char *vaddr)
+{
+	phys_addr_t paddr = virt_to_phys((volatile void *)vaddr);
+	return efi_paddr_valid(paddr);
+}
+
 static int __init romsignature(const unsigned char *rom)
 {
 	const unsigned short * const ptr = (const unsigned short *)rom;
 	unsigned short sig;
 
+	if (efi_enabled) {
+		int rv = efi_vaddr_valid(rom);
+		printk(KERN_DEBUG
+			"romsignature: efi_vaddr_valid(0x%llx) returned %d\n",
+			 (unsigned long long)rom, rv);
+		if (0 == rv)
+			return 0;
+	}
 	return probe_kernel_address(ptr, sig) == 0 && sig == ROMSIGNATURE;
 }
 
@@ -194,6 +230,39 @@ static int __init romchecksum(const unsigned char *rom, unsigned long length)
 	return !length && !sum;
 }
 
+/* #define DO_TEST_PROBES "include customizable test code" */
+static void __init test_probes(void)
+{
+#ifdef	DO_TEST_PROBES
+	static struct {
+		phys_addr_t	paddr;
+		int		ptype;
+	} plist[] = {
+		{0x000000003e760000ULL,	0},	/* RESERVED		*/
+		{0x0000009ffda9f000ULL,	1},	/* LOADER CODE		*/
+		{0x000000000005e000ULL,	2},	/* LOADER DATA		*/
+		{0x0000000000060000ULL,	3},	/* BOOT CODE (why after EBS?) */
+		{0x0000000036400000ULL,	4},	/* BOOT DATA (why after EBS?) */
+		{0x0000009fffc96000ULL,	5},	/* RUNTIME CODE		*/
+		{0x0000009fffcd3000ULL,	6},	/* RUNTIME DATA		*/
+		{0x0000000000001000ULL,	7},	/* CONVENTIONAL MEM	*/
+		{0x000000003e75f000ULL,	9},	/* ACPI RECLAIM		*/
+		{0x000000000008c000ULL,	10},	/* ACPI NVS		*/
+		{0x0000000040000000ULL,	11},	/* MMIO			*/
+		{0,			-1}	/* terminate list	*/
+	};
+	int i, valid;
+	phys_addr_t paddr;
+	for (i = 0; plist[i].ptype >= 0; i++) {
+		paddr = plist[i].paddr;
+		valid = efi_paddr_valid(paddr);
+		printk(KERN_DEBUG
+		      "test_probes: efi_paddr_valid(0x%llx) returned %d\n",
+			(unsigned long long)plist[i].paddr, valid);
+	}
+#endif
+}
+
 void __init probe_roms(void)
 {
 	const unsigned char *rom;
@@ -201,6 +270,8 @@ void __init probe_roms(void)
 	unsigned char c;
 	int i;
 
+	test_probes();
+
 	/* video rom */
 	upper = adapter_rom_resources[0].start;
 	for (start = video_rom_resource.start; start < upper; start += 2048) {
-- 
1.7.9.5

Re: [PATCH RFC] function probe_roms accessing improper addresses on UEFI systems

[Matthew Garrett]

On Wed, Oct 03, 2012 at 05:53:46PM -0600, Randy Wright wrote:

> The following proposed patch takes advantage of the fact that on EFI
> systems, the memory map provides a better description of the physical
> space than on pre-EFI legacy systems. If the efi_enabled state variable
> indicates the kernel is running on an UEFI system, the patch will use
> information from the UEFI memory map so as not to access addresses that
> should avoided according to the UEFI specification.

This turns out to be awkward. Some (mostly older) EFI platforms still 
only provide the video ROM through the 0xc0000 window, and that's 
sometimes needed even if the platform isn't using int10 for anything 
(for instance, some Intel graphics machines only provide the VBT through 
the video ROM and don't provide that via the PCI BAR). And, of course, 
they have an EFI memory map that just shows a hole there.

So we can't distinguish between the two cases easily. The only thing I 
can think of would be to push that policy out to the graphics drivers 
and have them trigger a scan only if they can't get the required 
information from any other source. I suspect that this patch as is would 
break graphics on a reasonable number of EFI platforms.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH RFC] function probe_roms accessing improper addresses

[Randy Wright]

> Date: Thu, 4 Oct 2012 20:22:56 +0100
> From: Matthew Garrett <mjg59@srcf.ucam.org>
> To: rwright@hp.com
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFC] function probe_roms accessing improper addresses
>  on UEFI systems
> Message-ID: <20121004192256.GA6521@srcf.ucam.org>
> References: <201210032353.q93NrkNi018443@filesys1.fc.hp.com>
> 
> On Wed, Oct 03, 2012 at 05:53:46PM -0600, Randy Wright wrote:
> 
> > The following proposed patch takes advantage of the fact that on EFI
> > systems, the memory map provides a better description of the physical
> > space than on pre-EFI legacy systems. If the efi_enabled state variable
> > indicates the kernel is running on an UEFI system, the patch will use
> > information from the UEFI memory map so as not to access addresses that
> > should avoided according to the UEFI specification.
> 
> This turns out to be awkward. Some (mostly older) EFI platforms still 
> only provide the video ROM through the 0xc0000 window, and that's 
> sometimes needed even if the platform isn't using int10 for anything 
> (for instance, some Intel graphics machines only provide the VBT through 
> the video ROM and don't provide that via the PCI BAR). And, of course, 
> they have an EFI memory map that just shows a hole there.
> 
> So we can't distinguish between the two cases easily. The only thing I 
> can think of would be to push that policy out to the graphics drivers 
> and have them trigger a scan only if they can't get the required 
> information from any other source. I suspect that this patch as is would 
> break graphics on a reasonable number of EFI platforms.
> -- 
> Matthew Garrett | mjg59@srcf.ucam.org

Hi Matthew, 

I appreciate your description of the problems with my approach, as well
as the reply from hpa@zytor.com (H. Peter Anvin) in response to my mention
of this patch in another thread.  His reply contained a couple of
suggestions that initially appeal to me more than an approach requiring
a change to a set of video drivers, the size of which I don't quite
know how to scope.  In that other thread, hpa said:

| One option would be to quirk it; obviously there is some piece of
| hardware which does cause this #MC and hopefully we could use that to
| detect that specific regions should be excluded; another option would be
| to trap the #MC during ROM probing.

I definitely see the appeal of trapping the #MC and triggering a
solution from that, if it can be made to work. I've spent some time
evaluating that, and I see these issues:

1. I don't believe the kernel's MC handler is initialized early enough
to handle a machine check occurring as early as probe_roms.  Probe_roms
is called very early in boot.  I see this as the call stack:
  start_kernel->setup_arch->probe_roms
Whereas the machine check initialization for x86 appears to come later:
  start_kernel->check_bugs->identify_boot_cpu->identify_cpu->mcheck_cpu_init
At present, I do not want to tackle such a major reordering of
intialization as would be required to change this.

2. For all platforms, is the setup of chipset and cpu address decoding
robust enough to allow the OS to handle the resulting machine check and
recover?  I've worked with some platforms in the past where this was not
always the case, the result being that for some unpopulated address
ranges, the resulting machine check would not be recoverable.

Because of the above difficulties with the MC handler idea, I have
focused my thoughts more on the quirk idea that hpa mentioned. I've been
investigating some existing examples in the kernel, and trying to
understand some of the issues involved with designing a new one.

1. Can the interface be chosen to present the needed interface to all callers? 
I recognize this as a challenge if a single interface is to be used both
in early boot (e.g. probe_roms) and later runtime (e.g.
devmem_is_allowed).  Something like a new member added to the
x86_platform_ops structure?

2. How can it automatically be activated for platforms that need it? I
see quite a few quirks selected by cpu id, but that's probably not
appropriate here.  Again, activating it by hitting the #MC in probe_roms
would be cool, but I see it as involving a major reordering of
initialization code.  So I'm left thinking about something in keying off
the dmi platform strings, which fortunately are initialized thusly:
  start_kernel->setup_arch->dmi_scan_machine
convenient, as it's just before probe_roms is called.

3. Can it be activated on demand for testing on other platforms? A
kernel boot command line parameter could be added, for example. How does
the community feel about adding more of those?

What are other design issues I'm overlooking?

Are there are existing quirks that strike you as particularly
good models for this case?  

--
Randy Wright            Hewlett-Packard Company
Phone: (970) 898-0998   Mail: rwright@hp.com