[PATCH] Validate EFI memory descriptors

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

It turns out that these can be invalid in various ways.  Based on code
Ard Biesheuvel contributed for Linux.

Co-developed-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
---
 xen/common/efi/boot.c    | 11 +++++------
 xen/common/efi/efi.h     | 14 ++++++++++++++
 xen/common/efi/runtime.c | 13 ++++++-------
 xen/include/xen/types.h  |  1 +
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 
b3de1011ee58a67a82a94da050eb1343f4e37faa..dd0376fdf930c2fee35e79a2f2821361c5e15d33
 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -591,15 +591,14 @@ static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
 
 static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
 {
-    size_t available_len, len;
+    UINT64 available_len, len = efi_memory_descriptor_len(desc);
     const UINTN physical_start = desc->PhysicalStart;
     const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
 
-    len = desc->NumberOfPages << EFI_PAGE_SHIFT;
     if ( esrt == EFI_INVALID_TABLE_ADDR )
-        return 0;
+        return 0; /* invalid ESRT */
     if ( physical_start > esrt || esrt - physical_start >= len )
-        return 0;
+        return 0; /* ESRT not in this memory region */
     /*
      * The specification requires EfiBootServicesData, but also accept
      * EfiRuntimeServicesData (for compatibility with buggy firmware)
@@ -609,10 +608,10 @@ static size_t __init get_esrt_size(const 
EFI_MEMORY_DESCRIPTOR *desc)
     if ( (desc->Type != EfiRuntimeServicesData) &&
          (desc->Type != EfiBootServicesData) &&
          (desc->Type != EfiACPIReclaimMemory) )
-        return 0;
+        return 0; /* memory region cannot contain ESRT */
     available_len = len - (esrt - physical_start);
     if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
-        return 0;
+        return 0; /* ESRT header does not fit in memory region */
     available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
     esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
     if ( (esrt_ptr->FwResourceVersion !=
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index 
c9aa65d506b14de69c90b6538c934747fcf0fb80..f4875138415c23eac42616131a738b7f8e33e56b
 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -51,3 +51,17 @@ void free_ebmalloc_unused_mem(void);
 
 const void *pe_find_section(const void *image_base, const size_t image_size,
                             const CHAR16 *section_name, UINTN *size_out);
+
+static inline UINT64
+efi_memory_descriptor_len(const EFI_MEMORY_DESCRIPTOR *desc)
+{
+    uint64_t remaining_space = UINT64_MAX - desc->PhysicalStart;
+
+    if ( desc->PhysicalStart & (EFI_PAGE_SIZE - 1) )
+        return 0; /* misaligned start address */
+
+    if ( desc->NumberOfPages > (remaining_space >> EFI_PAGE_SHIFT) )
+        return 0; /* too big */
+
+    return desc->NumberOfPages << EFI_PAGE_SHIFT;
+}
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 
13b0975866e3a80c46a7e37788012a716a455b6a..b99de230a5464c46b0b6c19b073685b4e0298343
 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -270,18 +270,17 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
         for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
         {
             EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
-            u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+            uint64_t size, len = efi_memory_descriptor_len(desc);
 
             if ( info->mem.addr >= desc->PhysicalStart &&
-                 info->mem.addr < desc->PhysicalStart + len )
+                 info->mem.addr - desc->PhysicalStart < len )
             {
                 info->mem.type = desc->Type;
                 info->mem.attr = desc->Attribute;
-                if ( info->mem.addr + info->mem.size < info->mem.addr ||
-                     info->mem.addr + info->mem.size >
-                     desc->PhysicalStart + len )
-                    info->mem.size = desc->PhysicalStart + len -
-                                     info->mem.addr;
+                size = desc->PhysicalStart + len - info->mem.addr;
+                if ( info->mem.size > size )
+                    info->mem.size = size;
+
                 return 0;
             }
         }
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 
03f0fe612ed96118614505c39d0e33b946288d6c..255f5a3c91c6be3e8ba4584902563c11bee7f98d
 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -25,6 +25,7 @@
 #define UINT8_MAX       (255)
 #define UINT16_MAX      (65535)
 #define UINT32_MAX      (4294967295U)
+#define UINT64_MAX      (0xFFFFFFFFFFFFFFFFULL)
 
 #define INT_MAX         ((int)(~0U>>1))
 #define INT_MIN         (-INT_MAX - 1)
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab