Bug#1021846: [programmer11180 at programist.ru: Bug#1021846: grub-install is broken since 2.06-3: error: unknown filesystem]

[Steve McIntyre]

Hi Daniel!

On Sat, Dec 03, 2022 at 01:41:51AM +1100, Daniel Axtens wrote:
>Steve McIntyre <steve at einval.com> writes:
>>
>> программист некто (in CC) reported this bug a few weeks back in
>> Debian. Since I applied the bundle of filesystem bounds-checking fixes
>> a few months back, he can't run grub-install. He's done the work to
>> determine that the patch that breaks things for him is
>>
>> 2d014248d540c7e087934a94b6e7a2aa7fc2c704 fs/f2fs: Do not read past the end of nat journal entries
>>
>> The full thread of our discussion is at https://bugs.debian.org/1021846
>>
>> I don't have any knowledge of f2fs to go any further here. Help please! :-)
>
>Ergh, apologies for the regression.
>
>[somewhat off-topic: The fix came from a crash derived from fuzzing. I
>am not really knowledgeable about f2fs either - I was just trying to do
>my best based on what we could derive from the existing driver. In
>general, filesystems are a nightmare for fuzzing fixes because testing
>beyond the (quite decent!) tests that the grub test-suite runs is very
>challenging. There is usually no-one who is both involved in grub
>security and an expert on any given file system either. We do the best
>we can. Sadly our regression rate has been climbing, so we may need to
>come up with some other way to secure file systems or get access to
>sufficient expertise in the future.]

ACK. I used to develop amd maintain filesystems as a day job, I
understand the issue! Writing good and comprehensive tests is hard,
and therefore quite rare!

>I had a massive, massive work-in-progress spiel where I looked at this
>code and compared the linux code and counted sizes and so on and so
>forth. I was getting nowhere. But eventually I realised I had just made
>an off-by-one error in the test. You're allowed to have up to n =
>NAT_JOURNAL_ENTRIES entries _inclusive_, because the loop below uses i <
>n, not i <= n. D'oh.

Doh indeed! :-)

>Please try the following:
>
>diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
>index df6beb544cbd..855e24618c2b 100644
>--- a/grub-core/fs/f2fs.c
>+++ b/grub-core/fs/f2fs.c
>@@ -650,7 +650,7 @@ get_blkaddr_from_nat_journal (struct grub_f2fs_data *data, grub_uint32_t nid,
>   grub_uint16_t n = grub_le_to_cpu16 (data->nat_j.n_nats);
>   grub_uint16_t i;
> 
>-  if (n >= NAT_JOURNAL_ENTRIES)
>+  if (n > NAT_JOURNAL_ENTRIES)
>     return grub_error (GRUB_ERR_BAD_FS,
>                        "invalid number of nat journal entries");
>
>
>If for some reason that doesn't work, please add the following debug
>code and report the results:
>
>diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
>index 855e24618c2b..6e49a6d17b7a 100644
>--- a/grub-core/fs/f2fs.c
>+++ b/grub-core/fs/f2fs.c
>@@ -643,6 +643,10 @@ get_nat_journal (struct grub_f2fs_data *data)
>   return err;
> }
> 
>+#ifdef GRUB_UTIL
>+#include <stdio.h>
>+#endif
>+
> static grub_err_t
> get_blkaddr_from_nat_journal (struct grub_f2fs_data *data, grub_uint32_t nid,
>                               grub_uint32_t *blkaddr)
>@@ -650,6 +654,10 @@ get_blkaddr_from_nat_journal (struct grub_f2fs_data *data, grub_uint32_t nid,
>   grub_uint16_t n = grub_le_to_cpu16 (data->nat_j.n_nats);
>   grub_uint16_t i;
> 
>+#ifdef GRUB_UTIL
>+  fprintf(stderr, "%s: n = %hu\n", __func__, n);
>+#endif
>+
>   if (n > NAT_JOURNAL_ENTRIES)
>     return grub_error (GRUB_ERR_BAD_FS,
>                        "invalid number of nat journal entries");
>

программист некто: could you please try these changes and report back?

>Amusingly the debug code shows that the grub-fs-tester tests always have
>n = 0, which makes sense for a test that doesn't really stress the
>file-system, and also explains why we didn't catch the bug when it was
>introduced.

Right.

-- 
Steve McIntyre, Cambridge, UK.                                steve at einval.com
  Armed with "Valor": "Centurion" represents quality of Discipline,
  Honor, Integrity and Loyalty. Now you don't have to be a Caesar to
  concord the digital world while feeling safe and proud.