- 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);

- Patch0345: 0345-fs-xfs-Fix-XFS-directory-extent-parsing.patch

- Patch0346: 0346-fs-xfs-Add-large-extent-counters-incompat-feature-su.patch

- Patch0347: 0347-chainloader-remove-device-path-debug-message.patch 

- Release:	109%{?dist}