efi: Fix some malformed device path arithmetic errors

Several places we take the length of a device path and subtract 4 from
it, without ever checking that it's >= 4. There are also cases where
this kind of malformation will result in unpredictable iteration,
including treating the length from one dp node as the type in the next
node. These are all errors, no matter where the data comes from.

This patch adds a checking macro, GRUB_EFI_DEVICE_PATH_VALID(), which
can be used in several places, and makes GRUB_EFI_NEXT_DEVICE_PATH()
return NULL and GRUB_EFI_END_ENTIRE_DEVICE_PATH() evaluate as true when
the length is too small. Additionally, it makes several places in the
code check for and return errors in these cases.

Signed-off-by: Peter Jones <pjones@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index b2d07a3fa153137b7f8d8f5fc4c112c4e65cd695..535e29424e90e98cc054d6f6cfb134fee5280199 100644 (file)
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -342,7 +342,7 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0)
 
   dp = dp0;
 
-  while (1)
+  while (dp)
     {
       grub_efi_uint8_t type = GRUB_EFI_DEVICE_PATH_TYPE (dp);
       grub_efi_uint8_t subtype = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp);
@@ -352,9 +352,15 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0)
       if (type == GRUB_EFI_MEDIA_DEVICE_PATH_TYPE
               && subtype == GRUB_EFI_FILE_PATH_DEVICE_PATH_SUBTYPE)
        {
-         grub_efi_uint16_t len;
-         len = ((GRUB_EFI_DEVICE_PATH_LENGTH (dp) - 4)
-                / sizeof (grub_efi_char16_t));
+         grub_efi_uint16_t len = GRUB_EFI_DEVICE_PATH_LENGTH (dp);
+
+         if (len < 4)
+           {
+             grub_error (GRUB_ERR_OUT_OF_RANGE,
+                         "malformed EFI Device Path node has length=%d", len);
+             return NULL;
+           }
+         len = (len - 4) / sizeof (grub_efi_char16_t);
          filesize += GRUB_MAX_UTF8_PER_UTF16 * len + 2;
        }
 
@@ -370,7 +376,7 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0)
   if (!name)
     return NULL;
 
-  while (1)
+  while (dp)
     {
       grub_efi_uint8_t type = GRUB_EFI_DEVICE_PATH_TYPE (dp);
       grub_efi_uint8_t subtype = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp);
@@ -386,8 +392,15 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0)
 
          *p++ = '/';
 
-         len = ((GRUB_EFI_DEVICE_PATH_LENGTH (dp) - 4)
-                / sizeof (grub_efi_char16_t));
+         len = GRUB_EFI_DEVICE_PATH_LENGTH (dp);
+         if (len < 4)
+           {
+             grub_error (GRUB_ERR_OUT_OF_RANGE,
+                         "malformed EFI Device Path node has length=%d", len);
+             return NULL;
+           }
+
+         len = (len - 4) / sizeof (grub_efi_char16_t);
          fp = (grub_efi_file_path_device_path_t *) dp;
          /* According to EFI spec Path Name is NULL terminated */
          while (len > 0 && fp->path_name[len - 1] == 0)
@@ -462,7 +475,26 @@ grub_efi_duplicate_device_path (const grub_efi_device_path_t *dp)
        ;
        p = GRUB_EFI_NEXT_DEVICE_PATH (p))
     {
-      total_size += GRUB_EFI_DEVICE_PATH_LENGTH (p);
+      grub_size_t len = GRUB_EFI_DEVICE_PATH_LENGTH (p);
+
+      /*
+       * In the event that we find a node that's completely garbage, for
+       * example if we get to 0x7f 0x01 0x02 0x00 ... (EndInstance with a size
+       * of 2), GRUB_EFI_END_ENTIRE_DEVICE_PATH() will be true and
+       * GRUB_EFI_NEXT_DEVICE_PATH() will return NULL, so we won't continue,
+       * and neither should our consumers, but there won't be any error raised
+       * even though the device path is junk.
+       *
+       * This keeps us from passing junk down back to our caller.
+       */
+      if (len < 4)
+       {
+         grub_error (GRUB_ERR_OUT_OF_RANGE,
+                     "malformed EFI Device Path node has length=%d", len);
+         return NULL;
+       }
+
+      total_size += len;
       if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (p))
        break;
     }
@@ -507,7 +539,7 @@ dump_vendor_path (const char *type, grub_efi_vendor_device_path_t *vendor)
 void
 grub_efi_print_device_path (grub_efi_device_path_t *dp)
 {
-  while (1)
+  while (GRUB_EFI_DEVICE_PATH_VALID (dp))
     {
       grub_efi_uint8_t type = GRUB_EFI_DEVICE_PATH_TYPE (dp);
       grub_efi_uint8_t subtype = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp);
@@ -919,7 +951,10 @@ grub_efi_compare_device_paths (const grub_efi_device_path_t *dp1,
     /* Return non-zero.  */
     return 1;
 
-  while (1)
+  if (dp1 == dp2)
+    return 0;
+
+  while (GRUB_EFI_DEVICE_PATH_VALID (dp1) && GRUB_EFI_DEVICE_PATH_VALID (dp2))
     {
       grub_efi_uint8_t type1, type2;
       grub_efi_uint8_t subtype1, subtype2;
@@ -955,5 +990,14 @@ grub_efi_compare_device_paths (const grub_efi_device_path_t *dp1,
       dp2 = (grub_efi_device_path_t *) ((char *) dp2 + len2);
     }
 
+  /*
+   * There's no "right" answer here, but we probably don't want to call a valid
+   * dp and an invalid dp equal, so pick one way or the other.
+   */
+  if (GRUB_EFI_DEVICE_PATH_VALID (dp1) && !GRUB_EFI_DEVICE_PATH_VALID (dp2))
+    return 1;
+  else if (!GRUB_EFI_DEVICE_PATH_VALID (dp1) && GRUB_EFI_DEVICE_PATH_VALID (dp2))
+    return -1;
+
   return 0;
 }

diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
index daf8c6b541df8cb9a2388be788964eaf70f06c97..a8d7b91558a6a8cb93024d82e8fcc72a0a764698 100644 (file)
--- a/grub-core/loader/efi/chainloader.c
+++ b/grub-core/loader/efi/chainloader.c
@@ -156,9 +156,18 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename)
 
   size = 0;
   d = dp;
-  while (1)
+  while (d)
     {
-      size += GRUB_EFI_DEVICE_PATH_LENGTH (d);
+      grub_size_t len = GRUB_EFI_DEVICE_PATH_LENGTH (d);
+
+      if (len < 4)
+       {
+         grub_error (GRUB_ERR_OUT_OF_RANGE,
+                     "malformed EFI Device Path node has length=%d", len);
+         return NULL;
+       }
+
+      size += len;
       if ((GRUB_EFI_END_ENTIRE_DEVICE_PATH (d)))
        break;
       d = GRUB_EFI_NEXT_DEVICE_PATH (d);

diff --git a/grub-core/loader/i386/xnu.c b/grub-core/loader/i386/xnu.c
index e9e11925972204b98a906640dccb2e0729fbc7b0..a7009360732a663d1a212e8ed6141305a9744139 100644 (file)
--- a/grub-core/loader/i386/xnu.c
+++ b/grub-core/loader/i386/xnu.c
@@ -515,14 +515,15 @@ grub_cmd_devprop_load (grub_command_t cmd __attribute__ ((unused)),
 
       devhead = buf;
       buf = devhead + 1;
-      dpstart = buf;
+      dp = dpstart = buf;
 
-      do
+      while (GRUB_EFI_DEVICE_PATH_VALID (dp) && buf < bufend)
        {
-         dp = buf;
          buf = (char *) buf + GRUB_EFI_DEVICE_PATH_LENGTH (dp);
+         if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (dp))
+           break;
+         dp = buf;
        }
-      while (!GRUB_EFI_END_ENTIRE_DEVICE_PATH (dp) && buf < bufend);
 
       dev = grub_xnu_devprop_add_device (dpstart, (char *) buf
                                         - (char *) dpstart);

diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 937058d6899bc3fcc92b99e738c1dcbad0afa2ff..1dcaa12f59e8c34a0a8aa593c6cafd743543e422 100644 (file)
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -630,6 +630,7 @@ typedef struct grub_efi_device_path grub_efi_device_path_protocol_t;
 #define GRUB_EFI_DEVICE_PATH_TYPE(dp)          ((dp)->type & 0x7f)
 #define GRUB_EFI_DEVICE_PATH_SUBTYPE(dp)       ((dp)->subtype)
 #define GRUB_EFI_DEVICE_PATH_LENGTH(dp)                ((dp)->length)
+#define GRUB_EFI_DEVICE_PATH_VALID(dp)         ((dp) != NULL && GRUB_EFI_DEVICE_PATH_LENGTH (dp) >= 4)
 
 /* The End of Device Path nodes.  */
 #define GRUB_EFI_END_DEVICE_PATH_TYPE                  (0xff & 0x7f)
@@ -638,13 +639,16 @@ typedef struct grub_efi_device_path grub_efi_device_path_protocol_t;
 #define GRUB_EFI_END_THIS_DEVICE_PATH_SUBTYPE          0x01
 
 #define GRUB_EFI_END_ENTIRE_DEVICE_PATH(dp)    \
-  (GRUB_EFI_DEVICE_PATH_TYPE (dp) == GRUB_EFI_END_DEVICE_PATH_TYPE \
-   && (GRUB_EFI_DEVICE_PATH_SUBTYPE (dp) \
-       == GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE))
+  (!GRUB_EFI_DEVICE_PATH_VALID (dp) || \
+   (GRUB_EFI_DEVICE_PATH_TYPE (dp) == GRUB_EFI_END_DEVICE_PATH_TYPE \
+    && (GRUB_EFI_DEVICE_PATH_SUBTYPE (dp) \
+       == GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE)))
 
 #define GRUB_EFI_NEXT_DEVICE_PATH(dp)  \
-  ((grub_efi_device_path_t *) ((char *) (dp) \
-                               + GRUB_EFI_DEVICE_PATH_LENGTH (dp)))
+  (GRUB_EFI_DEVICE_PATH_VALID (dp) \
+   ? ((grub_efi_device_path_t *) \
+      ((char *) (dp) + GRUB_EFI_DEVICE_PATH_LENGTH (dp))) \
+   : NULL)
 
 /* Hardware Device Path.  */
 #define GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE             1