Re: [PATCH] fs/xfs: Avoid unreadble filesystem if V4 superblock

[Carlos Maiolino]

On Thu, Aug 26, 2021 at 09:54:59AM +0200, Erwan Velu wrote:
>    I should have cc Carlos Maiolino who authored this patch.
>    This is now done.
> 
>    Le mer. 25 août 2021 à 15:32, Erwan Velu <[1]erwanaliasr1@gmail.com> a
>    écrit :
> 
>      Commit 8b1e5d1936fffc490510e85c95f93248453586c1 introduced the
>      support
>      of bigtime by adding the some features in inodes V3.
>      This change extended grub_xfs_inode struct by 76 bytes but also
>      changed the
>      computation of XFS_V3_INODE_SIZE & XFS_V2_INODE_SIZE.
>      Prior this commit, XFS_V2_INODE_SIZE was 100 bytes, after the commit
>      it's 84 bytes.
>      XFS_V2_INODE_SIZE becomes 16 bytes too small.
>      As a result, the data structure aren't properly aligned and
>      generates
>      "attempt to read or write outside of partition" errors when trying
>      to
>      read the filesystem.

Thanks for spotting this!

>                                   GNU GRUB  version 2.11
>              ....
>              grub> set debug=efi,gpt,xfs
>              grub> insmod part_gpt
>              grub> ls (hd0,gpt1)/
>              partmap/gpt.c:93: Read a valid GPT header
>              partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
>              fs/xfs.c:931: Reading sb
>              fs/xfs.c:270: Validating superblock
>              fs/xfs.c:295: XFS v4 superblock detected
>              fs/xfs.c:962: Reading root ino 128
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (739521961424144223) -
>      344365866970255880, 3840
>              error: attempt to read or write outside of partition.
>      This commit change the XFS_V2_INODE_SIZE computation by substracting
>      76
>      instead of 92 from the actual size of grub_xfs_inode.
>      This 76 value is coming from the added :
>              20 uint8   unused5
>               1 uint64  flags2
>              48 uint8   unused6
>      This patch explicit the split between the v2 and v3 parts of
>      structure.
>      The unused4 is still ending to the v2 structures and the v3 starts
>      at unused5.
>      This will avoid future corruption of v2 or v3.
>      The XFS_V2_INODE_SIZE is returning to its expected size and the
>      filesystem is back to a readable state.
>                            GNU GRUB  version 2.11
>              ....
>              grub> set debug=efi,gpt,xfs
>              grub> insmod part_gpt
>              grub> ls (hd0,gpt1)/
>              partmap/gpt.c:93: Read a valid GPT header
>              partmap/gpt.c:115: GPT entry 0: start=4096, length=1953125
>              fs/xfs.c:931: Reading sb
>              fs/xfs.c:270: Validating superblock
>              fs/xfs.c:295: XFS v4 superblock detected
>              fs/xfs.c:962: Reading root ino 128
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:931: Reading sb
>              fs/xfs.c:270: Validating superblock
>              fs/xfs.c:295: XFS v4 superblock detected
>              fs/xfs.c:962: Reading root ino 128
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (128) - 64, 0
>              fs/xfs.c:515: Reading inode (131) - 64, 768
>              efi/ fs/xfs.c:515: Reading inode (3145856) - 1464904, 0
>              grub2/ fs/xfs.c:515: Reading inode (132) - 64, 1024
>              grub/ fs/xfs.c:515: Reading inode (139) - 64, 2816
>              grub>
>      Signed-off-by: Erwan Velu <[2]e.velu@criteo.com>
>      ---
>       grub-core/fs/xfs.c | 10 ++++++----
>       1 file changed, 6 insertions(+), 4 deletions(-)
>      diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
>      index 0f524c3a8a61..becea5bcdf61 100644
>      --- a/grub-core/fs/xfs.c
>      +++ b/grub-core/fs/xfs.c
>      @@ -192,6 +192,7 @@ struct grub_xfs_time_legacy
>         grub_uint32_t nanosec;
>       } GRUB_PACKED;
>      +// From
>      [3]https://mirrors.edge.kernel.org/pub/linux/utils/fs/xfs/docs/xfs_f
>      ilesystem_structure.pdf about 'struct xfs_dinode_core'
>       struct grub_xfs_inode
>       {
>         grub_uint8_t magic[2];
>      @@ -208,14 +209,15 @@ struct grub_xfs_inode
>         grub_uint32_t nextents;
>         grub_uint16_t unused3;
>         grub_uint8_t fork_offset;
>      -  grub_uint8_t unused4[37];
>      +  grub_uint8_t unused4[17]; // Last member of inode v2
>      +  grub_uint8_t unused5[20]; // First member of inode v3
>         grub_uint64_t flags2;
>      -  grub_uint8_t unused5[48];
>      +  grub_uint8_t unused6[48]; // Last member of inode v3
>       } GRUB_PACKED;
>       #define XFS_V3_INODE_SIZE      sizeof(struct grub_xfs_inode)
>      -/* Size of struct grub_xfs_inode until fork_offset (included). */
>      -#define XFS_V2_INODE_SIZE      (XFS_V3_INODE_SIZE - 92)
>      +/* Size of struct grub_xfs_inode v2, up to unused4 included */
>      +#define XFS_V2_INODE_SIZE      (XFS_V3_INODE_SIZE - 76)
>       struct grub_xfs_dirblock_tail
>       {
>      --
>      2.25.1
> 
> References
> 
>    1. mailto:erwanaliasr1@gmail.com
>    2. mailto:e.velu@criteo.com
>    3. 
> https://mirrors.edge.kernel.org/pub/linux/utils/fs/xfs/docs/xfs_filesystem_structure.pdf

-- 
Carlos