Hi,
>
> Could you help have another look at this latest version?
> Is it ready for merging (Yifan has been working on for almost one year
> in his leisure time) so that kernel/initrd can be loaded then..
> Any comment would be much helpful.
>
I am doing this in my leisure time too - I appreciate your patience and sorry
it’s taking a while!
Thanks for the new version. I can confirm that I no longer see the ASAN issues
I identified on v9.
I had one other “maybe” finding. There’s a potentially very large allocation in
the symlink code:
+ if (grub_add (erofs_inode_file_size (node), 1, &sz))
+ {
+ grub_error (GRUB_ERR_OUT_OF_RANGE, N_ ("overflow is detected"));
+ return NULL;
+ }
+
+ symlink = grub_malloc (sz);
+ if (!symlink)
+ return NULL;
I don’t know if this is actually wrong or if the filesystem spec permits the
size here to be very large. At any rate if the memory allocation cannot be
satisfied the grub allocator will return NULL which you catch correctly. I’m
not sure if this needs any change, but I wanted to bring it to your attention
anyway just in case.
I found no crash issues in the fuzzers not running ASAN. In total, there were
over 14 billion executions of a basic fuzz harness over about 7 days. I’m
rerunning the fuzzers with the new build but I don’t expect anything will pop
up.
Congratulations, as far as I can tell, your filesystem implementation is one of
the most safe against arbitrary input in all of grub.
For this series:
Tested-by: Daniel Axtens <d...@axtens.net <mailto:d...@axtens.net>> # fuzz
testing only
Kind regards,
Daniel
> Thanks,
> Gao Xiang
>
>> Yifan Zhao (2):
>> fs/erofs: Add support for EROFS
>> fs/erofs: Add tests for EROFS in grub-fs-tester
>> .gitignore | 1 +
>> INSTALL | 8 +-
>> Makefile.util.def | 7 +
>> docs/grub.texi | 3 +-
>> grub-core/Makefile.core.def | 5 +
>> grub-core/fs/erofs.c | 984 +++++++++++++++++++++++++++++++++++
>> grub-core/kern/misc.c | 14 +
>> include/grub/misc.h | 1 +
>> tests/erofs_test.in | 20 +
>> tests/util/grub-fs-tester.in | 32 +-
>> 10 files changed, 1063 insertions(+), 12 deletions(-)
>> create mode 100644 grub-core/fs/erofs.c
>> create mode 100644 tests/erofs_test.in
>> Interdiff against v7:
>> diff --git a/grub-core/fs/erofs.c b/grub-core/fs/erofs.c
>> index 5b89b7924..13f92e71a 100644
>> --- a/grub-core/fs/erofs.c
>> +++ b/grub-core/fs/erofs.c
>> @@ -101,6 +101,7 @@ struct grub_erofs_inode_chunk_info
>> #define EROFS_NULL_ADDR 1
>> #define EROFS_NAME_LEN 255
>> +#define EROFS_MIN_LOG2_BLOCK_SIZE 9
>> #define EROFS_MAX_LOG2_BLOCK_SIZE 16
>> struct grub_erofs_inode_chunk_index
>> @@ -558,7 +559,7 @@ erofs_iterate_dir (grub_fshelp_node_t dir,
>> grub_fshelp_iterate_dir_hook_t hook,
>> goto not_found;
>> nameoff = grub_le_to_cpu16 (de->nameoff);
>> - if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff > maxsize)
>> + if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff >= maxsize)
>> {
>> grub_error (GRUB_ERR_BAD_FS,
>> "invalid nameoff %u @ inode %" PRIuGRUB_UINT64_T,
>> @@ -587,11 +588,12 @@ erofs_iterate_dir (grub_fshelp_node_t dir,
>> grub_fshelp_iterate_dir_hook_t hook,
>> fdiro->inode_loaded = false;
>> nameoff = grub_le_to_cpu16 (de->nameoff);
>> - if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff > maxsize)
>> + if (nameoff < sizeof (struct grub_erofs_dirent) || nameoff >= maxsize)
>> {
>> grub_error (GRUB_ERR_BAD_FS,
>> "invalid nameoff %u @ inode %" PRIuGRUB_UINT64_T,
>> nameoff, dir->ino);
>> + grub_free (fdiro);
>> goto not_found;
>> }
>> @@ -607,6 +609,7 @@ erofs_iterate_dir (grub_fshelp_node_t dir,
>> grub_fshelp_iterate_dir_hook_t hook,
>> "invalid de_namelen %" PRIuGRUB_SIZE
>> " @ inode %" PRIuGRUB_UINT64_T,
>> de_namelen, dir->ino);
>> + grub_free (fdiro);
>> goto not_found;
>> }
>> @@ -700,6 +703,7 @@ erofs_mount (grub_disk_t disk, bool read_root)
>> if (err != GRUB_ERR_NONE)
>> return NULL;
>> if (sb.magic != grub_cpu_to_le32_compile_time (EROFS_MAGIC) ||
>> + grub_le_to_cpu32 (sb.log2_blksz) < EROFS_MIN_LOG2_BLOCK_SIZE ||
>> grub_le_to_cpu32 (sb.log2_blksz) > EROFS_MAX_LOG2_BLOCK_SIZE)
>> {
>> grub_error (GRUB_ERR_BAD_FS, "not a valid erofs filesystem");
>> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
>> index ecb27fa5a..67240d64d 100644
>> --- a/grub-core/kern/misc.c
>> +++ b/grub-core/kern/misc.c
>> @@ -602,7 +602,7 @@ grub_strnlen (const char *s, grub_size_t n)
>> if (n == 0)
>> return 0;
>> - while (*p && n--)
>> + while (n-- && *p)
>> p++;
>> return p - s;
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
