For some fuzzers I’m working on, l happened to run into the same issue this patch is addressing and used a fix similar to what Vladimir suggested (keeping mft_end check and adding attr_cur != NULL check). This fixes the issue for me in my fuzzer so far.
Thanks, Andrew On Fri, Feb 28, 2025 at 7:12 PM B Horn <b...@horn.uk> wrote: > > On 28/02/2025 22:17, Vladimir 'phcoder' Serbinenko wrote: > > Why do you remove boundary check? Maybe it's better to add the second > > boundary check from below > > The issue stems from my original patches being rebased over `fs/ntfs: > Fix out-of-bounds read`, with both my patches and that commit fixing the > same fuzzed NTFS issue in the while-loop. I felt it was better to get it > to known good state as I verified my original patches against quite a > few NTFS volumes including several that caused the original OOB reads. > > Keeping the mft_end checks doesn't hurt after adding the attr_cur check > back in, just not much point as at->end should always be the same value. > > > > > Le ven. 28 févr. 2025, 16:11, B Horn <b...@horn.uk <mailto:b...@horn.uk>> a > > écrit : > > > > On some NTFS volumes GRUB would enter an infinite loop when > > next_attribute() returned NULL, which can happen in normal cases when > > the end of the attribute list is reached. > > This would trigger a NULL deref, but as the null page is mapped on > the > > majority of firmware, an infinite loop would occur as the while loop > > didn't make any progress. > > > > Fixing this by verifying the value of at->attr_cur on the next > iteration > > of the loop, after it has been set to the result of next_attribute(). > > Also removing the redundant check against mft_end as the > > next_attribute() should handle that now. > > A pointer to the end of the buffer is stored in at->end, which is > > initialized the same way as mft_end was. > > > > Fixes: https://savannah.gnu.org/bugs/index.php?66855 <https:// > > savannah.gnu.org/bugs/index.php?66855> > > > > Reported-by: Andreas Klauer <andreas.kla...@metamorpher.de > > <mailto:andreas.kla...@metamorpher.de>> > > Signed-off-by: B Horn <b...@horn.uk <mailto:b...@horn.uk>> > > --- > > grub-core/fs/ntfs.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c > > index 960833a34..77531e627 100644 > > --- a/grub-core/fs/ntfs.c > > +++ b/grub-core/fs/ntfs.c > > @@ -311,8 +311,6 @@ free_attr (struct grub_ntfs_attr *at) > > static grub_uint8_t * > > find_attr (struct grub_ntfs_attr *at, grub_uint8_t attr) > > { > > - grub_uint8_t *mft_end; > > - > > if (at->flags & GRUB_NTFS_AF_ALST) > > { > > retry: > > @@ -386,8 +384,7 @@ find_attr (struct grub_ntfs_attr *at, > > grub_uint8_t attr) > > return NULL; > > } > > at->attr_cur = at->attr_nxt; > > - mft_end = at->mft->buf + (at->mft->data->mft_size << > > GRUB_NTFS_BLK_SHR); > > - while (at->attr_cur < mft_end && *at->attr_cur != 0xFF) > > + while (at->attr_cur && *at->attr_cur != 0xFF) > > { > > at->attr_nxt = next_attribute (at->attr_cur, at->end); > > if (*at->attr_cur == GRUB_NTFS_AT_ATTRIBUTE_LIST) > > -- > > 2.48.1 > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org <mailto:Grub-devel@gnu.org> > > https://lists.gnu.org/mailman/listinfo/grub-devel <https:// > > lists.gnu.org/mailman/listinfo/grub-devel> > > > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
