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