[PATCH v4] fs/xfs: Fix XFS directory extent parsing

* [PATCH v4] fs/xfs: Fix XFS directory extent parsing
@ 2023-10-18  3:03 Jon DeVree
  2023-10-18 16:23 ` Daniel Kiper
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Jon DeVree @ 2023-10-18  3:03 UTC (permalink / raw)
  To: grub-devel; +Cc: Jon DeVree

The XFS directory entry parsing code has never been completely correct
for extent based directories. The parser correctly handles the case
where the directory is contained in a single extent, but then mistakenly
assumes the data blocks for the multiple extent case are each identical
to the single extent case. The difference in the format of the data
blocks between the two cases is tiny enough that its gone unnoticed for
a very long time.

A recent change introduced some additional bounds checking into the XFS
parser. Like GRUB's existing parser, it is correct for the single extent
case but incorrect for the multiple extent case. When parsing a
directory with multiple extents, this new bounds checking is sometimes
(but not always) tripped and triggers an "invalid XFS diretory entry"
error. This probably would have continued to go unnoticed but the
/boot/grub/<arch> directory is large enough that it often has multiple
extents.

The difference between the two cases is that when there are multiple
extents, the data blocks do not contain a trailer nor do they contain
any leaf information. That information is stored in a separate set of
extents dedicated to just the leaf information. These extents come after
the directory entry extents and are not included in the inode size. So
the existing parser already ignores the leaf extents.

The only reason to read the trailer/leaf information at all is so that
the parser can avoid misinterpreting that data as directory entries. So
this updates the parser as follows:

For the single extent case the parser doesn't change much:
1. Read the size of the leaf information from the trailer
2. Set the end pointer for the parser to the start of the leaf
   information. (The previous bounds checking set the end pointer to the
   start of the trailer, so this is actually a small improvement.)
3. Set the entries variable to the expected number of directory entries.

For the multiple extent case:
1. Set the end pointer to the end of the block.
2. Do not set up the entries variable. Figuring out how many entries are
   in each individual block is complex and does not seem worth it when
   it appears to be safe to just iterate over the entire block.

The bounds check itself was also dependent upon the faulty XFS parser
because it accidentally used "filename + length - 1". Presumably this
was able to pass the fuzzer because in the old parser there was always 8
bytes of slack space between the tail pointer and the actual end of the
block. Since this is no longer the case the bounds check needs to be
updated to "filename + length + 1" in order to prevent a regressionn in
the handling of corrupt fliesystems.

Notes:
* When there is only one extent there will only ever be one block. If
  more than one block is required then XFS will always switch to holding
  leaf information in a separate extent.
* B-tree based directories seems to be parsed properly by the same code
  that handles multiple extents. This is unlikely to ever occur within
  /boot though because its only used when there are an extremely large
  number of directory entries.

Fixes: ef7850c75 (fs/xfs: Fix issues found while fuzzing the XFS filesystem)
Fixes: b2499b29c (Adds support for the XFS filesystem.)
Fixes: https://savannah.gnu.org/bugs/?64376

Signed-off-by: Jon DeVree <nuxi@vault24.org>
---
 grub-core/fs/xfs.c | 51 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index b91cd32b4..acdfb1a7b 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -223,6 +223,12 @@ struct grub_xfs_inode
 /* Size of struct grub_xfs_inode v2, up to unused4 member included. */
 #define XFS_V2_INODE_SIZE	(XFS_V3_INODE_SIZE - 76)
 
+struct grub_xfs_dir_leaf_entry
+{
+  grub_uint32_t hashval;
+  grub_uint32_t address;
+} GRUB_PACKED;
+
 struct grub_xfs_dirblock_tail
 {
   grub_uint32_t leaf_count;
@@ -877,9 +883,8 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 	  {
 	    struct grub_xfs_dir2_entry *direntry =
 					grub_xfs_first_de(dir->data, dirblock);
-	    int entries;
-	    struct grub_xfs_dirblock_tail *tail =
-					grub_xfs_dir_tail(dir->data, dirblock);
+	    int entries = -1;
+	    char *end = dirblock + dirblk_size;
 
 	    numread = grub_xfs_read_file (dir, 0, 0,
 					  blk << dirblk_log2,
@@ -890,14 +895,27 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 	        return 0;
 	      }
 
-	    entries = (grub_be_to_cpu32 (tail->leaf_count)
-		       - grub_be_to_cpu32 (tail->leaf_stale));
+	    /* leaf and tail information are only in the data block if the number
+	     * of extents is 1 */
+	    if (dir->inode.nextents == grub_cpu_to_be32_compile_time (1))
+	      {
+		struct grub_xfs_dirblock_tail *tail =
+		                               grub_xfs_dir_tail(dir->data, dirblock);
+		end = (char *)tail;
 
-	    if (!entries)
-	      continue;
+		/* subtract the space used by leaf nodes */
+		end -= grub_be_to_cpu32 (tail->leaf_count) *
+		       sizeof (struct grub_xfs_dir_leaf_entry);
+
+		entries = (grub_be_to_cpu32 (tail->leaf_count)
+		           - grub_be_to_cpu32 (tail->leaf_stale));
+
+		if (!entries)
+		  continue;
+	      }
 
 	    /* Iterate over all entries within this block.  */
-	    while ((char *)direntry < (char *)tail)
+	    while ((char *)direntry < (char *)end)
 	      {
 		grub_uint8_t *freetag;
 		char *filename;
@@ -917,7 +935,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		  }
 
 		filename = (char *)(direntry + 1);
-		if (filename + direntry->len - 1 > (char *) tail)
+		if (filename + direntry->len + 1 > (char *) end)
 		  return grub_error (GRUB_ERR_BAD_FS, "invalid XFS directory entry");
 
 		/* The byte after the filename is for the filetype, padding, or
@@ -931,11 +949,16 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 		    return 1;
 		  }
 
-		/* Check if last direntry in this block is
-		   reached.  */
-		entries--;
-		if (!entries)
-		  break;
+		/* the expected number of directory entries is only tracked for the
+		 * single extent case */
+		if (dir->inode.nextents == grub_cpu_to_be32_compile_time (1))
+		  {
+		    /* Check if last direntry in this block is
+		       reached.  */
+		    entries--;
+		    if (!entries)
+		      break;
+		  }
 
 		/* Select the next directory entry.  */
 		direntry = grub_xfs_next_de(dir->data, direntry);
-- 
2.42.0


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread