[PATCH v3 0/5] fs: Test failure fixes and fuzzer fixes

[PATCH v3 0/5] fs: Test failure fixes and fuzzer fixes

[Andrew Hamilton]

Correct some NTFS test failures introduced by previous fixes for CVEs. 
With these changes, the NTFS tests run successfully.

Also correct some fuzzer identified crashes and hangs (in NTFS and one
that may in theory impact HFS too).

Changes since v2:
 - Daniel Kiper review comments addressed:
  - v3 patch 1: Improve commentary and make code cleaner.
  - v3 patch 2: Correct coding style and improve comments.
  - v3 patch 2: Simplify one condition by using grub_max.
  - v3 patch 2-4: Move variable declarations to top of function.
  - v3 patch 4,5: Swap order of these patches.
  - v3 patch 3,4: Split these into two from v2 patch 4.

Andrew Hamilton (5):
  fs/ntfs: Correct regression with run list calculation
  fs/ntfs: Correct attribute vs attribute list validation
  fs/ntfs: Correct possible access violations
  fs/ntfs: Correct possible infinite loops / hangs
  fs/fshelp: Avoid possible NULL pointer deference

 grub-core/fs/fshelp.c |   5 +-
 grub-core/fs/ntfs.c   | 169 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 143 insertions(+), 31 deletions(-)

-- 
2.39.5


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

[PATCH v3 1/5] fs/ntfs: Correct regression with run list calculation

[Andrew Hamilton]

Correct ntfs_test test failures around attempting to validate attribute
run list values. The calculation was incorrect for the 'curr' variable.
With previous calculation, some file systems would fail validation
despite being well-formed and valid. This was caused by incrementing
'curr' by min_size which included both the (already accounted for)
min_size as well as the size of the run list. Correct by making a new
variable 'run_size' to denote the current run list size to increment
both 'curr' and 'min_size' separately.

Fixes: 067b6d225 (fs/ntfs: Implement attribute verification)

Signed-off-by: Andrew Hamilton <adhamilt@gmail.com>
---
 grub-core/fs/ntfs.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
index b3117bf92..27d9bb11c 100644
--- a/grub-core/fs/ntfs.c
+++ b/grub-core/fs/ntfs.c
@@ -83,6 +83,7 @@ validate_attribute (grub_uint8_t *attr, void *end)
 {
   grub_size_t attr_size = 0;
   grub_size_t min_size = 0;
+  grub_size_t run_size = 0;
   grub_size_t spare = (grub_uint8_t *) end - attr;
   /*
    * Just used as a temporary variable to try and deal with cases where someone
@@ -172,11 +173,15 @@ validate_attribute (grub_uint8_t *attr, void *end)
 	   * to the number of bytes used to store the total length of the
 	   * data run, and the number of bytes used to store the offset.
 	   * These directly follow the header byte, so we use them to update
-	   * the minimum size.
+	   * the minimum size. Increment by one more than run size to move on
+	   * to the next run size header byte. An example is a run size field
+	   * value of 0x32, 3 + 2 = 5 bytes follow the run size. Increment
+	   * by 5 to get to the end of this data run then one more to get to
+	   * the start of the next run size byte.
 	   */
-	  min_size += (attr[curr] & 0x7) + ((attr[curr] >> 4) & 0x7);
-	  curr += min_size;
-	  min_size++;
+	  run_size = (attr[curr] & 0x7) + ((attr[curr] >> 4) & 0x7);
+	  curr += (run_size + 1);
+	  min_size += (run_size + 1);
 	  if (min_size > attr_size)
 	    goto fail;
 	}
-- 
2.39.5


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

[PATCH v3 2/5] fs/ntfs: Correct attribute vs attribute list validation

[Andrew Hamilton]

Correct ntfs_test test failures around attempting to validate attribute
list entries as attributes. The NTFS code uses common logic in some
places to parse both attributes and attribute_lists which complicates
validation. Attribute lists contain different headers including a
different size of the length field (2 bytes) at offset 4 instead of the
4 byte length field used in attributes at offset 4. There are other
differences as well, but attempting to validate attribute list types
using attribute header validation was causing failure of the NTFS test
suite. This change restores some of the validation logic which may be
shared between attributes and attribute lists to be closer to the
original logic prior to fixes for previous CVEs. A following commit will
address some of the implications of removing this validation logic by
correcting some fuzzer failures (some which are exposed by removing the
validation in some of the cases).

Fixes: 067b6d225 (fs/ntfs: Implement attribute verification)

Signed-off-by: Andrew Hamilton <adhamilt@gmail.com>
---
 grub-core/fs/ntfs.c | 59 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
index 27d9bb11c..1000a8c4e 100644
--- a/grub-core/fs/ntfs.c
+++ b/grub-core/fs/ntfs.c
@@ -221,7 +221,7 @@ validate_attribute (grub_uint8_t *attr, void *end)
 
 /* Return the next attribute if it exists, otherwise return NULL. */
 static grub_uint8_t *
-next_attribute (grub_uint8_t *curr_attribute, void *end)
+next_attribute (grub_uint8_t *curr_attribute, void *end, bool validate)
 {
   grub_uint8_t *next = curr_attribute;
 
@@ -233,7 +233,7 @@ next_attribute (grub_uint8_t *curr_attribute, void *end)
     return NULL;
 
   next += u16at (curr_attribute, 4);
-  if (validate_attribute (next, end) == false)
+  if (validate && validate_attribute (next, end) == false)
     return NULL;
 
   return next;
@@ -317,14 +317,21 @@ static grub_uint8_t *
 find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 {
   grub_uint8_t *mft_end;
+  grub_uint16_t nsize;
+  grub_uint16_t nxt_offset;
 
+  /* GRUB_NTFS_AF_ALST indicates the attribute list type */
   if (at->flags & GRUB_NTFS_AF_ALST)
     {
     retry:
       while (at->attr_nxt)
 	{
 	  at->attr_cur = at->attr_nxt;
-	  at->attr_nxt = next_attribute (at->attr_cur, at->attr_end);
+	  /*
+	   * Go to the next attribute in the list but do not validate
+	   * because this is the attribute list type.
+	   */
+	  at->attr_nxt = next_attribute (at->attr_cur, at->attr_end, false);
 	  if ((*at->attr_cur == attr) || (attr == 0))
 	    {
 	      grub_uint8_t *new_pos, *end;
@@ -380,7 +387,11 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 		    {
 		      return new_pos;
 		    }
-		  new_pos = next_attribute (new_pos, end);
+		    /*
+		     * Go to the next attribute in the list but do not validate
+		     * because this is the attribute list type.
+		     */
+		    new_pos = next_attribute (new_pos, end, false);
 		}
 	      grub_error (GRUB_ERR_BAD_FS,
 			  "can\'t find 0x%X in attribute list",
@@ -394,7 +405,20 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
   mft_end = at->mft->buf + (at->mft->data->mft_size << GRUB_NTFS_BLK_SHR);
   while (at->attr_cur >= at->mft->buf && at->attr_cur < mft_end && *at->attr_cur != 0xFF)
     {
-      at->attr_nxt = next_attribute (at->attr_cur, at->end);
+      /*
+       * We can't use validate_attribute here because this logic
+       * seems to be used for both parsing through attributes
+       * and attribute lists.
+       */
+      nsize = u16at (at->attr_cur, 4);
+      if (at->attr_cur + grub_max (GRUB_NTFS_ATTRIBUTE_HEADER_SIZE, nsize) >= at->end)
+      {
+        at->attr_nxt = at->attr_cur;
+        break;
+      }
+      else
+        at->attr_nxt = at->attr_cur + nsize;
+
       if (*at->attr_cur == GRUB_NTFS_AT_ATTRIBUTE_LIST)
 	at->attr_end = at->attr_cur;
       if ((*at->attr_cur == attr) || (attr == 0))
@@ -441,14 +465,25 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
       /* From this point on pa_end is the end of the buffer */
       at->end = pa_end;
 
-      if (validate_attribute (at->attr_nxt, pa_end) == false)
-	return NULL;
+      if (at->attr_end >= pa_end || at->attr_nxt >= pa_end)
+        return NULL;
 
       while (at->attr_nxt)
 	{
 	  if ((*at->attr_nxt == attr) || (attr == 0))
 	    break;
-	  at->attr_nxt = next_attribute (at->attr_nxt, pa_end);
+
+	  nxt_offset = u16at (at->attr_nxt, 4);
+	  at->attr_nxt += nxt_offset;
+
+	  /*
+	   * Stop and set attr_nxt to NULL when either the next offset is zero,
+	   * or when the pointer is within four bytes of the end of the buffer
+	   * since we could attempt to access attr_nxt + 4 bytes offset above to
+	   * get the next 16-bit 'nxt_offset' value.
+	   */
+	  if (nxt_offset == 0 || at->attr_nxt >= (pa_end - 4))
+	    at->attr_nxt = NULL;
 	}
 
       if (at->attr_nxt >= at->attr_end || at->attr_nxt == NULL)
@@ -473,7 +508,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 						  + 1));
 	  pa = at->attr_nxt + u16at (pa, 4);
 
-	  if (validate_attribute (pa, pa_end) == true)
+	  if (pa >= pa_end)
 	    pa = NULL;
 
 	  while (pa)
@@ -492,7 +527,9 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 		   u32at (pa, 0x10) * (at->mft->data->mft_size << GRUB_NTFS_BLK_SHR),
 		   at->mft->data->mft_size << GRUB_NTFS_BLK_SHR, 0, 0, 0))
 		return NULL;
-	      pa = next_attribute (pa, pa_end);
+	      pa += u16at (pa, 4);
+	      if (pa >= pa_end)
+	        pa = NULL;
 	    }
 	  at->attr_nxt = at->attr_cur;
 	  at->flags &= ~GRUB_NTFS_AF_GPOS;
@@ -739,7 +776,7 @@ read_attr (struct grub_ntfs_attr *at, grub_uint8_t *dest, grub_disk_addr_t ofs,
 	  if (u32at (pa, 8) > vcn)
 	    break;
 	  at->attr_nxt = pa;
-	  pa = next_attribute (pa, at->attr_end);
+	  pa = next_attribute (pa, at->attr_end, true);
 	}
     }
   pp = find_attr (at, attr);
-- 
2.39.5


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

[PATCH v3 3/5] fs/ntfs: Correct possible access violations

[Andrew Hamilton]

Correct several memory access violations found during fuzzing.
The issues fixed here could occur if certain specific malformed NTFS
file systems were presented to GRUB. Currently, GRUB does not allow NTFS
file system access when lockdown mode is enforced, so these should be of
minimal impact.

The changes made in this commit generally correct issues where pointers
into data buffers were being calculated using lengths read from the
NTFS file system without sufficient bounds / sanity checking; or
attempting to access elements of a structure to free them, when the
structure pointer is NULL.

Signed-off-by: Andrew Hamilton <adhamilt@gmail.com>
---
 grub-core/fs/ntfs.c | 64 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
index 1000a8c4e..5f7a1ecc9 100644
--- a/grub-core/fs/ntfs.c
+++ b/grub-core/fs/ntfs.c
@@ -248,6 +248,7 @@ fixup (grub_uint8_t *buf, grub_size_t len, const grub_uint8_t *magic)
   grub_uint16_t ss;
   grub_uint8_t *pu;
   grub_uint16_t us;
+  grub_uint16_t pu_offset;
 
   COMPILE_TIME_ASSERT ((1 << GRUB_NTFS_BLK_SHR) == GRUB_DISK_SECTOR_SIZE);
 
@@ -257,7 +258,10 @@ fixup (grub_uint8_t *buf, grub_size_t len, const grub_uint8_t *magic)
   ss = u16at (buf, 6) - 1;
   if (ss != len)
     return grub_error (GRUB_ERR_BAD_FS, "size not match");
-  pu = buf + u16at (buf, 4);
+  pu_offset = u16at (buf, 4);
+  if (pu_offset >= (len * GRUB_DISK_SECTOR_SIZE - (2 * ss)))
+    return grub_error (GRUB_ERR_BAD_FS, "pu offset size incorrect");
+  pu = buf + pu_offset;
   us = u16at (pu, 0);
   buf -= 2;
   while (ss > 0)
@@ -319,6 +323,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
   grub_uint8_t *mft_end;
   grub_uint16_t nsize;
   grub_uint16_t nxt_offset;
+  grub_uint32_t edat_offset;
 
   /* GRUB_NTFS_AF_ALST indicates the attribute list type */
   if (at->flags & GRUB_NTFS_AF_ALST)
@@ -379,6 +384,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 
 	      new_pos = &at->emft_buf[first_attr_off (at->emft_buf)];
 	      end = &at->emft_buf[emft_buf_size];
+	      at->end = end;
 
 	      while (new_pos && *new_pos != 0xFF)
 		{
@@ -403,7 +409,8 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
     }
   at->attr_cur = at->attr_nxt;
   mft_end = at->mft->buf + (at->mft->data->mft_size << GRUB_NTFS_BLK_SHR);
-  while (at->attr_cur >= at->mft->buf && at->attr_cur < mft_end && *at->attr_cur != 0xFF)
+  while (at->attr_cur >= at->mft->buf && at->attr_cur < (mft_end - 4)
+         && *at->attr_cur != 0xFF)
     {
       /*
        * We can't use validate_attribute here because this logic
@@ -451,13 +458,25 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 	      return NULL;
 	    }
 	  at->attr_nxt = at->edat_buf;
-	  at->attr_end = at->edat_buf + u32at (pa, 0x30);
+	  edat_offset = u32at (pa, 0x30);
+	  if (edat_offset >= n)
+	    {
+	      grub_error (GRUB_ERR_BAD_FS, "edat offset is out of bounds");
+	      return NULL;
+	    }
+	  at->attr_end = at->edat_buf + edat_offset;
 	  pa_end = at->edat_buf + n;
 	}
       else
 	{
 	  at->attr_nxt = at->attr_end + res_attr_data_off (pa);
-	  at->attr_end = at->attr_end + u32at (pa, 4);
+	  edat_offset = u32at (pa, 4);
+	  if ((at->attr_end + edat_offset) >= (at->end))
+	    {
+	      grub_error (GRUB_ERR_BAD_FS, "edat offset is out of bounds");
+	      return NULL;
+	    }
+	  at->attr_end = at->attr_end + edat_offset;
 	  pa_end = at->mft->buf + (at->mft->data->mft_size << GRUB_NTFS_BLK_SHR);
 	}
       at->flags |= GRUB_NTFS_AF_ALST;
@@ -486,7 +505,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 	    at->attr_nxt = NULL;
 	}
 
-      if (at->attr_nxt >= at->attr_end || at->attr_nxt == NULL)
+      if ((at->attr_nxt + GRUB_NTFS_ATTRIBUTE_HEADER_SIZE) >= at->attr_end || at->attr_nxt == NULL)
 	return NULL;
 
       if ((at->flags & GRUB_NTFS_AF_MMFT) && (attr == GRUB_NTFS_AT_DATA))
@@ -655,6 +674,8 @@ read_data (struct grub_ntfs_attr *at, grub_uint8_t *pa, grub_uint8_t *dest,
 	   grub_disk_read_hook_t read_hook, void *read_hook_data)
 {
   struct grub_ntfs_rlst cc, *ctx;
+  grub_uint8_t *end_ptr = (pa + len);
+  grub_uint16_t run_offset;
 
   if (len == 0)
     return 0;
@@ -687,7 +708,11 @@ read_data (struct grub_ntfs_attr *at, grub_uint8_t *pa, grub_uint8_t *dest,
       return 0;
     }
 
-  ctx->cur_run = pa + u16at (pa, 0x20);
+  run_offset = u16at (pa, 0x20);
+  if ((run_offset + pa) >= end_ptr || ((run_offset + pa) >= (at->end)))
+      return grub_error (GRUB_ERR_BAD_FS, "run offset out of range");
+
+  ctx->cur_run = pa + run_offset;
 
   ctx->next_vcn = u32at (pa, 0x10);
   ctx->curr_lcn = 0;
@@ -750,6 +775,8 @@ read_attr (struct grub_ntfs_attr *at, grub_uint8_t *dest, grub_disk_addr_t ofs,
   grub_uint8_t *pp;
   grub_err_t ret;
 
+  if (at == NULL || at->attr_cur == NULL)
+    return grub_error (GRUB_ERR_BAD_FS, "attribute not found");
   save_cur = at->attr_cur;
   at->attr_nxt = at->attr_cur;
   attr = *at->attr_nxt;
@@ -846,8 +873,11 @@ init_file (struct grub_ntfs_file *mft, grub_uint64_t mftno)
 static void
 free_file (struct grub_ntfs_file *mft)
 {
-  free_attr (&mft->attr);
-  grub_free (mft->buf);
+  if (mft)
+  {
+    free_attr (&mft->attr);
+    grub_free (mft->buf);
+  }
 }
 
 static char *
@@ -1051,6 +1081,7 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
   int ret = 0;
   grub_size_t bitmap_len;
   struct grub_ntfs_file *mft;
+  grub_uint32_t tmp_len;
 
   mft = (struct grub_ntfs_file *) dir;
 
@@ -1082,6 +1113,8 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
 	  (u32at (cur_pos, 0x1C) != 0x300033))
 	continue;
       cur_pos += res_attr_data_off (cur_pos);
+      if(cur_pos >= at->end)
+        continue;
       if (*cur_pos != 0x30)	/* Not filename index */
 	continue;
       break;
@@ -1151,7 +1184,15 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
                               "fails to read non-resident $BITMAP");
                   goto done;
                 }
-              bitmap_len = u32at (cur_pos, 0x30);
+                tmp_len = u32at (cur_pos, 0x30);
+                if (tmp_len <= bitmap_len)
+                  bitmap_len = tmp_len;
+                else
+                {
+                  grub_error (GRUB_ERR_BAD_FS,
+                    "bitmap len too large for non-resident $BITMAP");
+                  goto done;
+                }
             }
 
           bitmap = bmp;
@@ -1496,7 +1537,7 @@ grub_ntfs_label (grub_device_t device, char **label)
 
   pa = find_attr (&mft->attr, GRUB_NTFS_AT_VOLUME_NAME);
 
-  if (pa >= mft->buf + (mft->data->mft_size << GRUB_NTFS_BLK_SHR))
+  if (pa == NULL || pa >= mft->buf + (mft->data->mft_size << GRUB_NTFS_BLK_SHR))
     {
       grub_error (GRUB_ERR_BAD_FS, "can\'t parse volume label");
       goto fail;
@@ -1514,7 +1555,8 @@ grub_ntfs_label (grub_device_t device, char **label)
 
       len = res_attr_data_len (pa) / 2;
       pa += res_attr_data_off (pa);
-      if (mft->buf + (mft->data->mft_size << GRUB_NTFS_BLK_SHR) - pa >= 2 * len)
+      if (mft->buf + (mft->data->mft_size << GRUB_NTFS_BLK_SHR) - pa >= 2 * len &&
+          pa >= mft->buf && (pa + len < (mft->buf + (mft->data->mft_size << GRUB_NTFS_BLK_SHR))))
         *label = get_utf8 (pa, len);
       else
         grub_error (GRUB_ERR_BAD_FS, "can\'t parse volume label");
-- 
2.39.5


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

[PATCH v3 4/5] fs/ntfs: Correct possible infinite loops / hangs

[Andrew Hamilton]

Correct several infinite loops / hangs found during fuzzing.
The issues fixed here could occur if certain specific malformed NTFS
file systems were presented to GRUB. Currently, GRUB does not allow NTFS
file system access when lockdown mode is enforced, so these should be of
minimal impact.

The changes made in this commit generally correct issues such as attempting
to iterate through a buffer using a length read from the NTFS file system
without confirming the length is larger than 0.

Signed-off-by: Andrew Hamilton <adhamilt@gmail.com>
---
 grub-core/fs/ntfs.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
index 5f7a1ecc9..5b0a18f3d 100644
--- a/grub-core/fs/ntfs.c
+++ b/grub-core/fs/ntfs.c
@@ -428,7 +428,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr)
 
       if (*at->attr_cur == GRUB_NTFS_AT_ATTRIBUTE_LIST)
 	at->attr_end = at->attr_cur;
-      if ((*at->attr_cur == attr) || (attr == 0))
+      if ((*at->attr_cur == attr) || (attr == 0) || (nsize == 0))
 	return at->attr_cur;
       at->attr_cur = at->attr_nxt;
     }
@@ -563,6 +563,7 @@ locate_attr (struct grub_ntfs_attr *at, struct grub_ntfs_file *mft,
 	     grub_uint8_t attr)
 {
   grub_uint8_t *pa;
+  grub_uint8_t *last_pa;
 
   if (init_attr (at, mft) != GRUB_ERR_NONE)
     return NULL;
@@ -572,13 +573,16 @@ locate_attr (struct grub_ntfs_attr *at, struct grub_ntfs_file *mft,
     return NULL;
   if ((at->flags & GRUB_NTFS_AF_ALST) == 0)
     {
+      /* Used to make sure we're not stuck in a loop. */
+      last_pa = NULL;
       while (1)
 	{
 	  pa = find_attr (at, attr);
-	  if (pa == NULL)
+	  if (pa == NULL || pa == last_pa)
 	    break;
 	  if (at->flags & GRUB_NTFS_AF_ALST)
 	    return pa;
+	  last_pa = pa;
 	}
       grub_errno = GRUB_ERR_NONE;
       free_attr (at);
@@ -908,6 +912,7 @@ list_file (struct grub_ntfs_file *diro, grub_uint8_t *pos, grub_uint8_t *end_pos
 {
   grub_uint8_t *np;
   int ns;
+  grub_uint16_t pos_incr;
 
   while (1)
     {
@@ -970,7 +975,12 @@ list_file (struct grub_ntfs_file *diro, grub_uint8_t *pos, grub_uint8_t *end_pos
 
 	  grub_free (ustr);
 	}
-      pos += u16at (pos, 8);
+	pos_incr = u16at (pos, 8);
+	if (pos_incr > 0)
+	  pos += pos_incr;
+	else
+	  return 0;
+
     }
   return 0;
 }
@@ -1081,6 +1091,8 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
   int ret = 0;
   grub_size_t bitmap_len;
   struct grub_ntfs_file *mft;
+  /* Used to make sure we're not stuck in a loop. */
+  grub_uint8_t *last_pos = NULL;
   grub_uint32_t tmp_len;
 
   mft = (struct grub_ntfs_file *) dir;
@@ -1101,11 +1113,12 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
   while (1)
     {
       cur_pos = find_attr (at, GRUB_NTFS_AT_INDEX_ROOT);
-      if (cur_pos == NULL)
+      if (cur_pos == NULL || cur_pos == last_pos)
 	{
 	  grub_error (GRUB_ERR_BAD_FS, "no $INDEX_ROOT");
 	  goto done;
 	}
+      last_pos = cur_pos;
 
       /* Resident, Namelen=4, Offset=0x18, Flags=0x00, Name="$I30" */
       if ((u32at (cur_pos, 8) != 0x180400) ||
@@ -1133,10 +1146,18 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
   /* No need to check errors here, as it will already be fine */
   init_attr (at, mft);
 
+  last_pos = NULL;
   while ((cur_pos = find_attr (at, GRUB_NTFS_AT_BITMAP)) != NULL)
     {
       int ofs;
 
+      if (cur_pos == last_pos)
+      {
+        grub_error (GRUB_ERR_BAD_FS, "bitmap attribute loop");
+        goto done;
+      }
+      last_pos = cur_pos;
+
       ofs = cur_pos[0xA];
       /* Namelen=4, Name="$I30" */
       if ((cur_pos[9] == 4) &&
@@ -1201,6 +1222,7 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
     }
 
   free_attr (at);
+  last_pos = NULL;
   cur_pos = locate_attr (at, mft, GRUB_NTFS_AT_INDEX_ALLOCATION);
   while (cur_pos != NULL)
     {
@@ -1210,6 +1232,9 @@ grub_ntfs_iterate_dir (grub_fshelp_node_t dir,
 	  (u32at (cur_pos, 0x44) == 0x300033))
 	break;
       cur_pos = find_attr (at, GRUB_NTFS_AT_INDEX_ALLOCATION);
+      if (cur_pos == last_pos)
+        break;
+      last_pos = cur_pos;
     }
 
   if ((!cur_pos) && (bitmap))
-- 
2.39.5


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

[PATCH v3 5/5] fs/fshelp: Avoid possible NULL pointer deference

[Andrew Hamilton]

Avoid attempting to defererence a NULL pointer to call read_symlink when
the given filesystem does not provide a read_symlink function. This could
be triggered if the calling filesystem had a file marked as a symlink.
This appears possible for HFS and was observed during fuzzing of NTFS.

Signed-off-by: Andrew Hamilton <adhamilt@gmail.com>
Reviewed-by: Vladimir Serbinenko <phcoder@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/fs/fshelp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/fshelp.c b/grub-core/fs/fshelp.c
index cb41934b4..15278fb80 100644
--- a/grub-core/fs/fshelp.c
+++ b/grub-core/fs/fshelp.c
@@ -226,7 +226,10 @@ find_file (char *currpath,
 	    return grub_error (GRUB_ERR_SYMLINK_LOOP,
 			       N_("too deep nesting of symlinks"));
 
-	  symlink = read_symlink (ctx->currnode->node);
+	  if (read_symlink != NULL)
+	    symlink = read_symlink (ctx->currnode->node);
+	  else
+	    return grub_error (GRUB_ERR_BAD_FS, "read_symlink is NULL");
 
 	  if (!symlink)
 	    return grub_errno;
-- 
2.39.5


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

Re: [PATCH v3 0/5] fs: Test failure fixes and fuzzer fixes

[Daniel Kiper via Grub-devel]

On Wed, May 21, 2025 at 10:20:36PM -0500, Andrew Hamilton wrote:
> Correct some NTFS test failures introduced by previous fixes for CVEs.
> With these changes, the NTFS tests run successfully.
>
> Also correct some fuzzer identified crashes and hangs (in NTFS and one
> that may in theory impact HFS too).
>
> Changes since v2:
>  - Daniel Kiper review comments addressed:
>   - v3 patch 1: Improve commentary and make code cleaner.
>   - v3 patch 2: Correct coding style and improve comments.
>   - v3 patch 2: Simplify one condition by using grub_max.
>   - v3 patch 2-4: Move variable declarations to top of function.
>   - v3 patch 4,5: Swap order of these patches.
>   - v3 patch 3,4: Split these into two from v2 patch 4.
>
> Andrew Hamilton (5):
>   fs/ntfs: Correct regression with run list calculation
>   fs/ntfs: Correct attribute vs attribute list validation
>   fs/ntfs: Correct possible access violations
>   fs/ntfs: Correct possible infinite loops / hangs
>   fs/fshelp: Avoid possible NULL pointer deference

For all patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Thank you for fixing all these issues!

Daniel

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