Hi Daniel,
On 2024/5/10 01:57, Daniel Kiper wrote:
Hey,
On Thu, May 02, 2024 at 03:01:37PM +0800, 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
Sadly Coverity complains about the EROFS code. Please take a look below...
Let me try to fix these Coverity warnings directly..
Daniel
________________________________________________________________________________________________________
*** CID 460612: (BAD_SHIFT)
/grub-core/fs/erofs.c: 418 in erofs_map_blocks_chunkmode()
412 pos &= ~(unit - 1);
413
414 /* no overflow for multiplication as chunkbits >= 9 and sizeof(unit)
<= 8 */
415 if (grub_add (pos, chunknr * unit, &pos))
416 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
417
CID 460612: (BAD_SHIFT)
In expression "chunknr << chunkbits", left shifting by more than 63 bits has
undefined behavior. The shift amount, "chunkbits", is as much as 64.
These BAD_SHIFT warnings are in the same function:
erofs_map_blocks_chunkmode()
and we already have
398 chunkbits = node->data->sb.log2_blksz + (chunk_format &
EROFS_CHUNK_FORMAT_BLKBITS_MASK);
399 if (chunkbits > 64)
400 return grub_error (GRUB_ERR_BAD_FS, "invalid chunkbits %u @ inode %"
PRIuGRUB_UINT64_T,
401 chunkbits, node->ino);
I guess here should be "if (chunkbits > 63)"?
418 map->m_la = chunknr << chunkbits;
419
420 if (grub_sub (erofs_inode_file_size (node), map->m_la, &map->m_plen))
421 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
422 map->m_plen = grub_min (((grub_uint64_t) 1) << chunkbits,
423 ALIGN_UP (map->m_plen, erofs_blocksz
(node->data)));
/grub-core/fs/erofs.c: 403 in erofs_map_blocks_chunkmode()
397
398 chunkbits = node->data->sb.log2_blksz + (chunk_format &
EROFS_CHUNK_FORMAT_BLKBITS_MASK);
399 if (chunkbits > 64)
400 return grub_error (GRUB_ERR_BAD_FS, "invalid chunkbits %u @ inode
%" PRIuGRUB_UINT64_T,
401 chunkbits, node->ino);
402
CID 460612: (BAD_SHIFT)
In expression "map->m_la >> chunkbits", right shifting by more than 63 bits has
undefined behavior. The shift amount, "chunkbits", is as much as 64.
403 chunknr = map->m_la >> chunkbits;
404
405 if (grub_add (erofs_iloc (node), erofs_inode_size (node), &pos) ||
406 grub_add (pos, erofs_inode_xattr_ibody_size (node), &pos))
407 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
408
/grub-core/fs/erofs.c: 422 in erofs_map_blocks_chunkmode()
416 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
417
418 map->m_la = chunknr << chunkbits;
419
420 if (grub_sub (erofs_inode_file_size (node), map->m_la, &map->m_plen))
421 return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
CID 460612: (BAD_SHIFT)
In expression "1UL << chunkbits", left shifting by more than 63 bits has undefined
behavior. The shift amount, "chunkbits", is as much as 64.
422 map->m_plen = grub_min (((grub_uint64_t) 1) << chunkbits,
423 ALIGN_UP (map->m_plen, erofs_blocksz
(node->data)));
424
425 if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
426 {
427 struct grub_erofs_inode_chunk_index idx;
________________________________________________________________________________________________________
*** CID 460611: Error handling issues (CHECKED_RETURN)
/grub-core/fs/erofs.c: 793 in erofs_dir_iter()
787 {
788 struct grub_erofs_dir_ctx *ctx = data;
789 struct grub_dirhook_info info = {0};
790
791 if (!node->inode_loaded)
792 {
CID 460611: Error handling issues (CHECKED_RETURN)
Calling "erofs_read_inode" without checking return value (as is done
elsewhere 6 out of 7 times).
Will fix as below:
@@ -787,11 +787,13 @@ erofs_dir_iter (const char *filename, enum
grub_fshelp_filetype filetype,
{
struct grub_erofs_dir_ctx *ctx = data;
struct grub_dirhook_info info = {0};
+ grub_err_t err;
if (!node->inode_loaded)
{
- erofs_read_inode (ctx->data, node);
- grub_errno = GRUB_ERR_NONE;
+ err = erofs_read_inode (ctx->data, node);
+ if (err != GRUB_ERR_NONE)
+ return 0;
}
793 erofs_read_inode (ctx->data, node);
794 grub_errno = GRUB_ERR_NONE;
795 }
796
797 if (node->inode_loaded)
798 {
________________________________________________________________________________________________________
*** CID 460610: Resource leaks (RESOURCE_LEAK)
/grub-core/fs/erofs.c: 957 in grub_erofs_label()
951 *label = NULL;
952 return grub_errno;
953 }
954
955 *label = grub_strndup ((char *) data->sb.volume_name, sizeof
(data->sb.volume_name));
956 if (!*label)
CID 460610: Resource leaks (RESOURCE_LEAK)
Variable "data" going out of scope leaks the storage it points to.
957 return grub_errno;
958
959 grub_free (data);
Will fix as below:
@@ -953,11 +955,10 @@ grub_erofs_label (grub_device_t device, char **label)
}
*label = grub_strndup ((char *) data->sb.volume_name, sizeof
(data->sb.volume_name));
- if (!*label)
- return grub_errno;
-
grub_free (data);
+ if (!*label)
+ return grub_errno;
return GRUB_ERR_NONE;
}
Do these look okay?
Thanks,
Gao Xiang
960
961 return GRUB_ERR_NONE;
962 }
________________________________________________________________________________________________________
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
