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 <zhaoyi...@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.
+}
> +
> +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?
> +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?
+ {
> + if (grub_mul (grub_le_to_cpu32 (node->inode.i_u.raw_blkaddr),
> blocksz, &map->m_pa) ||
>
Missing cast to 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
+ 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
>
>
> + pos = ALIGN_UP (pos, unit);
>
Potential overflow if pos is only slightly under it's limit.
+ if (grub_add (pos, chunknr * unit, &pos))
>
Does this multiplication need verification?
> +
> + 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
> + {
> + 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
> + 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.
> + else
> + de_namelen = grub_le_to_cpu16 (de[1].nameoff) - nameoff;
>
This needs a check that it's not negative
> +
>
+ 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
>
> + 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 = file->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?
> +
> + 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.
>
> +grub_size_t
> +grub_strnlen (const char *s, grub_size_t n)
>
Already said in a different email
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
