Re: [PATCH v8 1/2] fs/erofs: Add support for EROFS

[Yifan Zhao]

On 4/29/24 10:15 PM, Vladimir 'phcoder' Serbinenko wrote:

Generally looks good. Few comments. I have missed part of discussion, so point me to relevant parts of it if I miss something that's already been discussed

Le mer. 24 avr. 2024, 14:31, Yifan Zhao <zhaoyifan@sjtu.edu.cn> a écrit : De qq,

+  struct grub_erofs_super *sb = &node->data->sb;
+
+  return (grub_le_to_cpu32 (sb->meta_blkaddr) << sb->log2_blksz) + (node->ino << EROFS_ISLOTBITS);

Here you have an overflow. You shift 32-bit value and so it's shifted as 32-bit and only then it's implicitly cast to 64-bit. You need an explicit cast before shift but after bytesesp for meta_blkaddr.

    Fixed. Thanks for caching it.

+}
+
+static grub_err_t
+erofs_read_inode (struct grub_erofs_data *data, grub_fshelp_node_t node)
+{
+  struct grub_erofs_inode_compact *dic;
+  grub_err_t err;
+  grub_uint16_t i_format;
+  grub_uint64_t addr = erofs_iloc (node);
+
+  dic = (struct grub_erofs_inode_compact *) &node->inode;

Why not use union member here?

    Will introduce a union struct holding
    grub_erofs_inode_{compact,extended}. The same below.

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

Ditto

+  grub_uint32_t blocksz = erofs_blocksz (node->data);
+
+  file_size = erofs_inode_file_size (node);
+  nblocks = (file_size + blocksz - 1) >> node->data->sb.log2_blksz;
+  lastblk = nblocks - tailendpacking;
+
+  map->m_flags = EROFS_MAP_MAPPED;
+
+  if (map->m_la < (lastblk * blocksz))

Is this multiplication checked somewhere?

    Introduce safe arithmetic for `nblocks` and explain why checking is
    unnecessary here in the comment.

+    {
+      if (grub_mul (grub_le_to_cpu32 (node->inode.i_u.raw_blkaddr), blocksz, &map->m_pa) ||

Missing cast to 64-bit.

    Fixed by making `blocksz` 64-bit.

+         grub_add (map->m_pa, map->m_la, &map->m_pa))
+       return GRUB_ERR_OUT_OF_RANGE;

It looks like you miss a grub_error

    Fixed. The same below.

+      if (grub_sub (lastblk * blocksz, map->m_la, &map->m_plen))

Ditto

+       return GRUB_ERR_OUT_OF_RANGE;

Ditto. I stop reviewing this particular construct

+    }
+  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;
+      if (grub_sub (file_size, map->m_la, &map->m_plen))
+       return GRUB_ERR_OUT_OF_RANGE;
+
+      if (((map->m_pa % blocksz) + map->m_plen) > blocksz)

Addition not checked. If there is a reason it can't overflow even if FS is maliciously corrupted, please add it in the comment

    Add comment explaining it cannot overflow.

+  pos = ALIGN_UP (pos, unit);

Potential overflow if pos is only slightly under it's limit.

    Fixed by using safe arithmetic.

+  if (grub_add (pos, chunknr * unit, &pos))

Does this multiplication need verification?

    Here it cannot overflow. Add comment explaining it.

+
+  if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
+    {
+      struct grub_erofs_inode_chunk_index idx;
+      grub_uint32_t blkaddr;

I recommend making it 64-bit to avoid casts

    Fixed. The overflow below cannot happen now.

+       {
+         map->m_pa = blkaddr << node->data->sb.log2_blksz;

Overflow again.

+       {
+         map->m_pa = blkaddr << node->data->sb.log2_blksz;

Overflow 

+
+      eend = grub_min (offset + size, map.m_la + map.m_llen);

Where is it checked that m_la+m_llen don't overflow? It can be checked when you read in the  map it should be trimmed or error-out

    Fixed with safe arithmetic. It will now error-out if overflow
    happens.

+  file_size = erofs_inode_file_size (dir);
+  buf = grub_malloc (blocksz);
+  if (!buf)
+    {
+      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");

No need for grub_error: malloc already does it.

    Fix it. The same below.

+         else
+           de_namelen = grub_le_to_cpu16 (de[1].nameoff) - nameoff;

 This needs a check that it's not negative

    Fixed.

+

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

OUT_OF_RANGE is already treated the same asBAD_FS in context of opening a file

    Fixed by removing this error code conversion.

+  if (!data)
+    {
+      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");

Ditto

+static grub_ssize_t
+grub_erofs_read (grub_file_t file, char *buf, grub_size_t len)
+{
+  struct grub_erofs_data *data = ""> +  struct grub_fshelp_node *inode = &data->inode;
+  grub_off_t off = file->offset;
+  grub_uint64_t ret = 0, file_size;
+  grub_err_t err;
+
+  if (!inode->inode_loaded)
+    {
+      err = erofs_read_inode (data, inode);
+      if (err != GRUB_ERR_NONE)
+       {
+         grub_error (GRUB_ERR_IO, "cannot read @ inode %" PRIuGRUB_UINT64_T, inode->ino);

Any reason to transform error? This could be something else than I/O error. Why not just propagate it?

    Fixed by propagate underlying error.

 

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

Again, I propose to just propagate underlying error.

    Ditto.

 

+grub_size_t
+grub_strnlen (const char *s, grub_size_t n)

Already said in a different email

Handled in patch v9 already.

Thanks,

Yifan Zhao