Re: [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access

[Lidong Chen]

On Apr 20, 2023, at 3:07 PM, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com> wrote:

On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com> wrote:

When an invalid node size is detected in grub_hfsplus_mount(), data pinter
is freed. Thus, file->data is not set. The code should also set the
grub error when that happens to indicate an error and to avoid accessing
the unintialized file->data in grub_file_close().

file->data is set to NULL in grub_file_open as part of zalloc. What
sets it to something else?

Thanks for the review.  Here is the flow that causes invalid access to file->data in grub_file_close():

static struct grub_hfsplus_data *

grub_hfsplus_mount (grub_disk_t disk)

{

  data->catalog_tree.nodesize = grub_be_to_cpu16 (header.nodesize);

  

  if (data->catalog_tree.nodesize < 2)   

    goto fail;    // grub_errno is not set to indicate the failure here.

 fail:

  grub_free (data);

  return 0;

}

static grub_err_t

grub_hfsplus_open (struct grub_file *file, const char *name)

{

  data = "" (file->device->disk); 

  if (!data)

    goto fail;

 fail:

   grub_free (data);

   return grub_errno; // return GRUB_ERR_NONE

}

grub_file_t

grub_file_open (const char *name, enum grub_file_type type)

{

  if ((file->fs->fs_open) (file, file_name) != GRUB_ERR_NONE) // grub_hfsplus_open()

    goto fail;

  file->name = grub_strdup (name);

  grub_errno = GRUB_ERR_NONE;

  return file;

}

static grub_err_t

grub_hfsplus_close (grub_file_t file)

{

  struct grub_hfsplus_data *data ="">

    (struct grub_hfsplus_data *) file->data;

  grub_free (data->opened_file.cbuf); // SIGSEGV here

  return GRUB_ERR_NONE;

}

Regards,

Lidong

Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/hfsplus.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 9c1f12574..cf13e8a63 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -357,7 +357,10 @@ grub_hfsplus_mount (grub_disk_t disk)
                         (header.key_compare == GRUB_HFSPLUSX_BINARYCOMPARE));

  if (data->catalog_tree.nodesize < 2)
-    goto fail;
+    {
+      grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
+      goto fail;
+    }

  if (grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, 0,
                             sizeof (struct grub_hfsplus_btnode),
@@ -374,7 +377,10 @@ grub_hfsplus_mount (grub_disk_t disk)
  data->extoverflow_tree.nodesize = grub_be_to_cpu16 (header.nodesize);

  if (data->extoverflow_tree.nodesize < 2)
-    goto fail;
+    {
+      grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
+      goto fail;
+    }

  data->extoverflow_tree_ready = 1;

--
2.39.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

-- 
Regards
Vladimir 'phcoder' Serbinenko