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 <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.
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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
