fs/hfsplus: Don't use uninitialized data on corrupt filesystems

Valgrind identified the following use of uninitialized data:

  ==2782220== Conditional jump or move depends on uninitialised value(s)
  ==2782220==    at 0x42B364: grub_hfsplus_btree_search (hfsplus.c:566)
  ==2782220==    by 0x42B21D: grub_hfsplus_read_block (hfsplus.c:185)
  ==2782220==    by 0x42A693: grub_fshelp_read_file (fshelp.c:386)
  ==2782220==    by 0x42C598: grub_hfsplus_read_file (hfsplus.c:219)
  ==2782220==    by 0x42C598: grub_hfsplus_mount (hfsplus.c:330)
  ==2782220==    by 0x42B8C5: grub_hfsplus_dir (hfsplus.c:958)
  ==2782220==    by 0x4C1AE6: grub_fs_probe (fs.c:73)
  ==2782220==    by 0x407C94: grub_ls_list_files (ls.c:186)
  ==2782220==    by 0x407C94: grub_cmd_ls (ls.c:284)
  ==2782220==    by 0x4D7130: grub_extcmd_dispatcher (extcmd.c:55)
  ==2782220==    by 0x4045A6: execute_command (grub-fstest.c:59)
  ==2782220==    by 0x4045A6: fstest (grub-fstest.c:433)
  ==2782220==    by 0x4045A6: main (grub-fstest.c:772)
  ==2782220==  Uninitialised value was created by a heap allocation
  ==2782220==    at 0x483C7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==2782220==    by 0x4C0305: grub_malloc (mm.c:42)
  ==2782220==    by 0x42C21D: grub_hfsplus_mount (hfsplus.c:239)
  ==2782220==    by 0x42B8C5: grub_hfsplus_dir (hfsplus.c:958)
  ==2782220==    by 0x4C1AE6: grub_fs_probe (fs.c:73)
  ==2782220==    by 0x407C94: grub_ls_list_files (ls.c:186)
  ==2782220==    by 0x407C94: grub_cmd_ls (ls.c:284)
  ==2782220==    by 0x4D7130: grub_extcmd_dispatcher (extcmd.c:55)
  ==2782220==    by 0x4045A6: execute_command (grub-fstest.c:59)
  ==2782220==    by 0x4045A6: fstest (grub-fstest.c:433)
  ==2782220==    by 0x4045A6: main (grub-fstest.c:772)

This happens when the process of reading the catalog file goes sufficiently
wrong that there's an attempt to read the extent overflow file, which has
not yet been loaded. Keep track of when the extent overflow file is
fully loaded and refuse to use it before then.

The load valgrind doesn't like is btree->nodesize, and that's then used
to allocate a data structure. It looks like there are subsequently a lot
of reads based on that pointer so OOB reads are likely, and indeed crashes
(albeit difficult-to-replicate ones) have been observed in fuzzing.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 1c7791b027e8a2440ae73a32ef1b5c096f14af71..361e5be492e50ddc5f29befeaa6d35e6b098c502 100644 (file)
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -177,6 +177,17 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
          break;
        }
 
+      /*
+       * If the extent overflow tree isn't ready yet, we can't look
+       * in it. This can happen where the catalog file is corrupted.
+       */
+      if (!node->data->extoverflow_tree_ready)
+       {
+         grub_error (GRUB_ERR_BAD_FS,
+                     "attempted to read extent overflow tree before loading");
+         break;
+       }
+
       /* Set up the key to look for in the extent overflow file.  */
       extoverflow.extkey.fileid = node->fileid;
       extoverflow.extkey.type = 0;
@@ -241,6 +252,7 @@ grub_hfsplus_mount (grub_disk_t disk)
     return 0;
 
   data->disk = disk;
+  data->extoverflow_tree_ready = 0;
 
   /* Read the bootblock.  */
   grub_disk_read (disk, GRUB_HFSPLUS_SBLOCK, 0, sizeof (volheader),
@@ -357,6 +369,8 @@ grub_hfsplus_mount (grub_disk_t disk)
   if (data->extoverflow_tree.nodesize < 2)
     goto fail;
 
+  data->extoverflow_tree_ready = 1;
+
   if (grub_hfsplus_read_file (&data->attr_tree.file, 0, 0,
                              sizeof (struct grub_hfsplus_btnode),
                              sizeof (header), (char *) &header) <= 0)

diff --git a/include/grub/hfsplus.h b/include/grub/hfsplus.h
index 117740ae26968a85094e9dd2c007449b443a33ef..e14dd31ff54c1c59d8e7c95868207a76ec56de68 100644 (file)
--- a/include/grub/hfsplus.h
+++ b/include/grub/hfsplus.h
@@ -113,6 +113,8 @@ struct grub_hfsplus_data
   struct grub_hfsplus_btree extoverflow_tree;
   struct grub_hfsplus_btree attr_tree;
 
+  int extoverflow_tree_ready;
+
   struct grub_hfsplus_file dirroot;
   struct grub_hfsplus_file opened_file;