From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001

From: Jon DeVree <nuxi@vault24.org>

Date: Tue, 17 Oct 2023 23:03:47 -0400

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

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 directory 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 regression 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>

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

Tested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Tested-by: Marta Lewandowska <mlewando@redhat.com>

---

 grub-core/fs/xfs.c | 52 ++++++++++++++++++++++++++++++++++++++--------------

 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c

index ebf962793fa7..18edfcff486c 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;

@@ -874,9 +880,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,

@@ -887,14 +892,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);

-	    if (!entries)

-	      continue;

+		end = (char *) tail;

+

+		/* 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;

@@ -914,7 +932,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

@@ -928,11 +946,17 @@ 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);