Hi Daniel! On Sat, Dec 03, 2022 at 01:41:51AM +1100, Daniel Axtens wrote: >Steve McIntyre <st...@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. st...@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.
