Re: [PATCH v10 0/2] Introduce EROFS support

[Gao Xiang]

Hi Daniel, Glenn, Vladimir,

On 2024/5/2 15:01, Yifan Zhao wrote:
> EROFS [1] is a lightweight read-only filesystem designed for performance
> which has already been shipped in most Linux distributions as well as widely
> used in several scenarios, such as Android system partitions, container
> images, and rootfs for embedded devices.
>
> This patch brings EROFS uncompressed support together with related tests.
> Now, it's possible to boot directly through GRUB with an EROFS rootfs.
>
> EROFS compressed files will be supported later since it has more work to
> polish.
>
> [1] https://erofs.docs.kernel.org
>
> changelog since v9:
> - fix overflow and style issues according to Serbinenko's review comments
> - add a upper bound for symlink length according to Daniel's advice
>
> Tested-by Link (Commit 1): 
> https://lists.gnu.org/archive/html/grub-devel/2024-05/msg00001.html
> Reviewed-by Link (Commit 2): 
> https://lists.gnu.org/archive/html/grub-devel/2024-04/msg00101.html

Does this version look good you? Could you help take some
time look at the latest version if possible?

Thanks,
Gao Xiang

> Yifan Zhao (2):
>    fs/erofs: Add support for EROFS
>    fs/erofs: Add tests for EROFS in grub-fs-tester
>
>   .gitignore                   |    1 +
>   INSTALL                      |    8 +-
>   Makefile.util.def            |    7 +
>   docs/grub.texi               |    3 +-
>   grub-core/Makefile.core.def  |    5 +
>   grub-core/fs/erofs.c         | 1007 ++++++++++++++++++++++++++++++++++
>   tests/erofs_test.in          |   20 +
>   tests/util/grub-fs-tester.in |   32 +-
>   8 files changed, 1071 insertions(+), 12 deletions(-)
>   create mode 100644 grub-core/fs/erofs.c
>   create mode 100644 tests/erofs_test.in
>
> Interdiff against v9:
> diff --git a/grub-core/fs/erofs.c b/grub-core/fs/erofs.c
> index 9c2678796..b82212b16 100644
> --- a/grub-core/fs/erofs.c
> +++ b/grub-core/fs/erofs.c
> @@ -101,6 +101,7 @@ struct grub_erofs_inode_chunk_info
>   
>   #define EROFS_NULL_ADDR			1
>   #define EROFS_NAME_LEN                        255
> +#define EROFS_PATH_LEN                 4096
>   #define EROFS_MIN_LOG2_BLOCK_SIZE     9
>   #define EROFS_MAX_LOG2_BLOCK_SIZE     16
>   
> @@ -160,6 +161,12 @@ struct grub_erofs_inode_extended
>     grub_uint8_t i_reserved2[16];
>   } GRUB_PACKED;
>   
> +union grub_erofs_inode
> +{
> +  struct grub_erofs_inode_compact c;
> +  struct grub_erofs_inode_extended e;
> +} GRUB_PACKED;
> +
>   #define EROFS_FT_UNKNOWN      0
>   #define EROFS_FT_REG_FILE     1
>   #define EROFS_FT_DIR          2
> @@ -197,7 +204,7 @@ struct grub_erofs_xattr_ibody_header
>   struct grub_fshelp_node
>   {
>     struct grub_erofs_data *data;
> -  struct grub_erofs_inode_extended inode;
> +  union grub_erofs_inode inode;
>   
>     grub_uint64_t ino;
>     grub_uint8_t inode_type;
> @@ -236,26 +243,27 @@ erofs_iloc (grub_fshelp_node_t node)
>   {
>     struct grub_erofs_super *sb = &node->data->sb;
>   
> -  return (grub_le_to_cpu32 (sb->meta_blkaddr) << sb->log2_blksz) + (node->ino << EROFS_ISLOTBITS);
> +  return ((grub_uint64_t) grub_le_to_cpu32 (sb->meta_blkaddr) << 
> sb->log2_blksz) +
> +        (node->ino << EROFS_ISLOTBITS);
>   }
>   
>   static grub_err_t
>   erofs_read_inode (struct grub_erofs_data *data, grub_fshelp_node_t node)
>   {
> -  struct grub_erofs_inode_compact *dic;
> +  union grub_erofs_inode *di;
>     grub_err_t err;
>     grub_uint16_t i_format;
>     grub_uint64_t addr = erofs_iloc (node);
>   
> -  dic = (struct grub_erofs_inode_compact *) &node->inode;
> +  di = (union grub_erofs_inode *) &node->inode;
>   
>     err = grub_disk_read (data->disk, addr >> GRUB_DISK_SECTOR_BITS,
>                         addr & (GRUB_DISK_SECTOR_SIZE - 1),
> -                       sizeof (struct grub_erofs_inode_compact), dic);
> +                       sizeof (struct grub_erofs_inode_compact), &di->c);
>     if (err != GRUB_ERR_NONE)
>       return err;
>   
> -  i_format = grub_le_to_cpu16 (dic->i_format);
> +  i_format = grub_le_to_cpu16 (di->c.i_format);
>     node->inode_type = (i_format >> EROFS_I_VERSION_BIT) & 
> EROFS_I_VERSION_MASKS;
>     node->inode_datalayout = (i_format >> EROFS_I_DATALAYOUT_BIT) & 
> EROFS_I_DATALAYOUT_MASKS;
>   
> @@ -267,7 +275,7 @@ erofs_read_inode (struct grub_erofs_data *data, grub_fshelp_node_t node)
>           data->disk, addr >> GRUB_DISK_SECTOR_BITS,
>           addr & (GRUB_DISK_SECTOR_SIZE - 1),
>           sizeof (struct grub_erofs_inode_extended) - sizeof (struct 
> grub_erofs_inode_compact),
> -         (grub_uint8_t *) dic + sizeof (struct grub_erofs_inode_compact));
> +         (grub_uint8_t *) di + sizeof (struct grub_erofs_inode_compact));
>         if (err != GRUB_ERR_NONE)
>         return err;
>         break;
> @@ -294,17 +302,17 @@ erofs_inode_size (grub_fshelp_node_t node)
>   static grub_uint64_t
>   erofs_inode_file_size (grub_fshelp_node_t node)
>   {
> -  struct grub_erofs_inode_compact *dic = (struct grub_erofs_inode_compact *) 
> &node->inode;
> +  union grub_erofs_inode *di = (union grub_erofs_inode *) &node->inode;
>   
>     return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
> -            ? grub_le_to_cpu32 (dic->i_size)
> -            : grub_le_to_cpu64 (node->inode.i_size);
> +            ? grub_le_to_cpu32 (di->c.i_size)
> +            : grub_le_to_cpu64 (di->e.i_size);
>   }
>   
>   static grub_uint32_t
>   erofs_inode_xattr_ibody_size (grub_fshelp_node_t node)
>   {
> -  grub_uint16_t cnt = grub_le_to_cpu16 (node->inode.i_xattr_icount);
> +  grub_uint16_t cnt = grub_le_to_cpu16 (node->inode.e.i_xattr_icount);
>   
>     if (cnt == 0)
>       return 0;
> @@ -317,7 +325,7 @@ erofs_inode_mtime (grub_fshelp_node_t node)
>   {
>     return node->inode_type == EROFS_INODE_LAYOUT_COMPACT
>              ? grub_le_to_cpu64 (node->data->sb.build_time)
> -            : grub_le_to_cpu64 (node->inode.i_mtime);
> +            : grub_le_to_cpu64 (node->inode.e.i_mtime);
>   }
>   
>   static grub_err_t
> @@ -326,31 +334,37 @@ erofs_map_blocks_flatmode (grub_fshelp_node_t node,
>   {
>     grub_uint64_t nblocks, lastblk, file_size;
>     bool tailendpacking = (node->inode_datalayout == EROFS_INODE_FLAT_INLINE);
> -  grub_uint32_t blocksz = erofs_blocksz (node->data);
> +  grub_uint64_t blocksz = erofs_blocksz (node->data);
>   
> +  /* file_size is checked by caller and cannot be zero, hence nblocks > 0 */
>     file_size = erofs_inode_file_size (node);
> -  nblocks = (file_size + blocksz - 1) >> node->data->sb.log2_blksz;
> +  if (grub_add (file_size, blocksz - 1, &nblocks))
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +  nblocks >>= node->data->sb.log2_blksz;
>     lastblk = nblocks - tailendpacking;
>   
>     map->m_flags = EROFS_MAP_MAPPED;
>   
> +  /* no overflow as (lastblk <= nblocks) && (nblocks * blocksz <= UINT64_MAX - blocksz + 1) */
>     if (map->m_la < (lastblk * blocksz))
>       {
> -      if (grub_mul (grub_le_to_cpu32 (node->inode.i_u.raw_blkaddr), blocksz, 
> &map->m_pa) ||
> +      if (grub_mul ((grub_uint64_t) grub_le_to_cpu32 
> (node->inode.e.i_u.raw_blkaddr), blocksz,
> +                   &map->m_pa) ||
>           grub_add (map->m_pa, map->m_la, &map->m_pa))
> -       return GRUB_ERR_OUT_OF_RANGE;
> +       return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
>         if (grub_sub (lastblk * blocksz, map->m_la, &map->m_plen))
> -       return GRUB_ERR_OUT_OF_RANGE;
> +       return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
>       }
>     else if (tailendpacking)
>       {
>         if (grub_add (erofs_iloc (node), erofs_inode_size (node), &map->m_pa) ||
>           grub_add (map->m_pa, erofs_inode_xattr_ibody_size (node), &map->m_pa) 
> ||
>           grub_add (map->m_pa, map->m_la % blocksz, &map->m_pa))
> -       return GRUB_ERR_OUT_OF_RANGE;
> +       return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
>         if (grub_sub (file_size, map->m_la, &map->m_plen))
> -       return GRUB_ERR_OUT_OF_RANGE;
> +       return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
>   
> +      /* no overflow as map->m_plen <= UINT64_MAX - blocksz + 1 */
>         if (((map->m_pa % blocksz) + map->m_plen) > blocksz)
>         return grub_error (
>             GRUB_ERR_BAD_FS,
> @@ -371,8 +385,8 @@ static grub_err_t
>   erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
>                             struct grub_erofs_map_blocks *map)
>   {
> -  grub_uint16_t chunk_format = grub_le_to_cpu16 (node->inode.i_u.c.format);
> -  grub_uint64_t unit, pos, chunknr;
> +  grub_uint16_t chunk_format = grub_le_to_cpu16 (node->inode.e.i_u.c.format);
> +  grub_uint64_t unit, pos, chunknr, blkaddr;
>     grub_uint8_t chunkbits;
>     grub_err_t err;
>   
> @@ -390,22 +404,27 @@ erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
>   
>     if (grub_add (erofs_iloc (node), erofs_inode_size (node), &pos) ||
>         grub_add (pos, erofs_inode_xattr_ibody_size (node), &pos))
> -    return GRUB_ERR_OUT_OF_RANGE;
> -  pos = ALIGN_UP (pos, unit);
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +
> +  /* pos = ALIGN_UP(pos, unit) */
> +  if (grub_add (pos, unit - 1, &pos))
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +  pos &= ~(unit - 1);
> +
> +  /* no overflow for multiplication as chunkbits >= 9 and sizeof(unit) <= 8 */
>     if (grub_add (pos, chunknr * unit, &pos))
> -    return GRUB_ERR_OUT_OF_RANGE;
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
>   
>     map->m_la = chunknr << chunkbits;
>   
>     if (grub_sub (erofs_inode_file_size (node), map->m_la, &map->m_plen))
> -    return GRUB_ERR_OUT_OF_RANGE;
> +    return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
>     map->m_plen = grub_min (((grub_uint64_t) 1) << chunkbits,
>                           ALIGN_UP (map->m_plen, erofs_blocksz (node->data)));
>   
>     if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
>       {
>         struct grub_erofs_inode_chunk_index idx;
> -      grub_uint32_t blkaddr;
>   
>         err = grub_disk_read (node->data->disk, pos >> GRUB_DISK_SECTOR_BITS,
>                             pos & (GRUB_DISK_SECTOR_SIZE - 1), unit, &idx);
> @@ -413,21 +432,10 @@ erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
>         return err;
>   
>         blkaddr = grub_le_to_cpu32 (idx.blkaddr);
> -
> -      if (blkaddr == (grub_uint32_t) EROFS_NULL_ADDR)
> -       {
> -         map->m_pa = 0;
> -         map->m_flags = 0;
> -       }
> -      else
> -       {
> -         map->m_pa = blkaddr << node->data->sb.log2_blksz;
> -         map->m_flags = EROFS_MAP_MAPPED;
> -       }
>       }
>     else
>       {
> -      grub_uint32_t blkaddr_le, blkaddr;
> +      grub_uint32_t blkaddr_le;
>   
>         err = grub_disk_read (node->data->disk, pos >> GRUB_DISK_SECTOR_BITS,
>                             pos & (GRUB_DISK_SECTOR_SIZE - 1), unit, 
> &blkaddr_le);
> @@ -435,16 +443,17 @@ erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
>         return err;
>   
>         blkaddr = grub_le_to_cpu32 (blkaddr_le);
> -      if (blkaddr == (grub_uint32_t) EROFS_NULL_ADDR)
> -       {
> -         map->m_pa = 0;
> -         map->m_flags = 0;
> -       }
> -      else
> -       {
> -         map->m_pa = blkaddr << node->data->sb.log2_blksz;
> -         map->m_flags = EROFS_MAP_MAPPED;
> -       }
> +    }
> +
> +  if (blkaddr == EROFS_NULL_ADDR)
> +    {
> +      map->m_pa = 0;
> +      map->m_flags = 0;
> +    }
> +  else
> +    {
> +      map->m_pa = blkaddr << node->data->sb.log2_blksz;
> +      map->m_flags = EROFS_MAP_MAPPED;
>       }
>   
>     map->m_llen = map->m_plen;
> @@ -497,7 +506,10 @@ erofs_read_raw_data (grub_fshelp_node_t node, grub_uint8_t 
> *buf, grub_uint64_t s
>         if (err != GRUB_ERR_NONE)
>         return err;
>   
> -      eend = grub_min (offset + size, map.m_la + map.m_llen);
> +      if (grub_add(map.m_la, map.m_llen, &eend))
> +       return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +
> +      eend = grub_min (eend, offset + size);
>         if (!(map.m_flags & EROFS_MAP_MAPPED))
>         {
>           if (!map.m_llen)
> @@ -557,10 +569,7 @@ erofs_iterate_dir (grub_fshelp_node_t dir, 
> grub_fshelp_iterate_dir_hook_t hook,
>     file_size = erofs_inode_file_size (dir);
>     buf = grub_malloc (blocksz);
>     if (!buf)
> -    {
> -      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> -      return 0;
> -    }
> +    return 0;
>   
>     while (offset < file_size)
>       {
> @@ -592,10 +601,7 @@ erofs_iterate_dir (grub_fshelp_node_t dir, 
> grub_fshelp_iterate_dir_hook_t hook,
>   
>   	  fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
>           if (!fdiro)
> -           {
> -             grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> -             goto not_found;
> -           }
> +           goto not_found;
>   
>   	  fdiro->data = dir->data;
>           fdiro->ino = grub_le_to_cpu64 (de->nid);
> @@ -615,7 +621,14 @@ erofs_iterate_dir (grub_fshelp_node_t dir, 
> grub_fshelp_iterate_dir_hook_t hook,
>           if (de + 1 >= end)
>             de_namelen = grub_erofs_strnlen (de_name, maxsize - nameoff);
>           else
> -           de_namelen = grub_le_to_cpu16 (de[1].nameoff) - nameoff;
> +           {
> +             if (grub_sub (grub_le_to_cpu16 (de[1].nameoff), nameoff, 
> &de_namelen))
> +               {
> +                 grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
> +                 grub_free (fdiro);
> +                 goto not_found;
> +               }
> +           }
>   
>   	  if (nameoff + de_namelen > maxsize || de_namelen > EROFS_NAME_LEN)
>             {
> @@ -687,6 +700,13 @@ erofs_read_symlink (grub_fshelp_node_t node)
>         return NULL;
>       }
>   
> +  if (sz > EROFS_PATH_LEN)
> +    {
> +      grub_error (GRUB_ERR_BAD_FS,
> +                 "symlink too long @ inode %" PRIuGRUB_UINT64_T, node->ino);
> +      return NULL;
> +    }
> +
>     symlink = grub_malloc (sz);
>     if (!symlink)
>       return NULL;
> @@ -712,8 +732,6 @@ erofs_mount (grub_disk_t disk, bool read_root)
>   
>     err = grub_disk_read (disk, EROFS_SUPER_OFFSET >> GRUB_DISK_SECTOR_BITS, 0,
>                         sizeof (sb), &sb);
> -  if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
> -    grub_error (GRUB_ERR_BAD_FS, "not a valid erofs filesystem");
>     if (err != GRUB_ERR_NONE)
>       return NULL;
>     if (sb.magic != grub_cpu_to_le32_compile_time (EROFS_MAGIC) ||
> @@ -734,10 +752,7 @@ erofs_mount (grub_disk_t disk, bool read_root)
>   
>     data = grub_malloc (sizeof (*data));
>     if (!data)
> -    {
> -      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> -      return NULL;
> -    }
> +    return NULL;
>   
>     data->disk = disk;
>     data->sb = sb;
> @@ -875,10 +890,7 @@ grub_erofs_read (grub_file_t file, char *buf, grub_size_t 
> len)
>       {
>         err = erofs_read_inode (data, inode);
>         if (err != GRUB_ERR_NONE)
> -       {
> -         grub_error (GRUB_ERR_IO, "cannot read @ inode %" PRIuGRUB_UINT64_T, 
> inode->ino);
> -         return -1;
> -       }
> +       return -1;
>       }
>   
>     file_size = erofs_inode_file_size (inode);
> @@ -896,10 +908,7 @@ grub_erofs_read (grub_file_t file, char *buf, grub_size_t 
> len)
>   
>     err = erofs_read_raw_data (inode, (grub_uint8_t *) buf, len, off, &ret);
>     if (err != GRUB_ERR_NONE)
> -    {
> -      grub_error (GRUB_ERR_IO, "cannot read file @ inode %" PRIuGRUB_UINT64_T, 
> inode->ino);
> -      return -1;
> -    }
> +    return -1;
>   
>     return ret;
>   }