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

[Marta Lewandowska <mlewando@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6820 bytes --]

On 10/18/23 05:03, Jon DeVree wrote:

> 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);
Tested-by: Marta Lewandowska <mlewando@redhat.com>

[-- Attachment #1.2: Type: text/html, Size: 7203 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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