Re: [PATCH v8 0/2] Introduce EROFS support

[Yifan Zhao]

On 5/1/24 9:22 PM, Daniel Axtens wrote:

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.

    Your testing effort is very important for this patchset. Thank you
    for your contribution!

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 believe your suggestion is reasonable. Currently, we are relying on the failure of malloc to catch overly large (potentially malicious) symlink sizes, which is not an elegant solution.

In the next version, we will introduce a maximum PATH_LEN (such as 4096), and will produce an error when the symlink is too long.

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.

Thank you once again for your time and contribution!

Kind regards,

Yifan Zhao

For this series:

Tested-by: Daniel Axtens <dja@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;