I've sent out a v3 just now with fixes for these.
Thanks,
Andrew
On Tue, May 20, 2025 at 11:34 AM Daniel Kiper <daniel.ki...@oracle.com>
wrote:
> I think you should change the order of this patch and patch #3.
>
> On Mon, May 19, 2025 at 09:03:18PM -0500, Andrew Hamilton wrote:
> > Correct several memory access violations and hangs found during fuzzing.
> > The issues fixed here could occur if certain specific malformed NTFS
> > file systems were presented to GRUB. Currently, GRUB does not allow NTFS
> > file system access when lockdown mode is enforced, so these should be of
> > minimal impact.
> >
> > The changes made in this commit generally correct issues where pointers
> > into data buffers were being calculated using lengths read from the
> > NTFS file system without sufficient bounds / sanity checking; attempting
> > to iterate through a buffer using a length read from the NTFS file system
> > without confirming the length is larger than 0 (possible hang / infinite
> > loop); or attempting to access elements of a structure to free them, when
> > the structure pointer is null.
>
> s/null/NULL/
>
> Is it possible to group the changes logically and put them in
> separate patches? The leftovers can be put into one last patch.
>
> > Signed-off-by: Andrew Hamilton <adham...@gmail.com>
> > ---
> > grub-core/fs/ntfs.c | 95 ++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 80 insertions(+), 15 deletions(-)
> >
> > diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
> > index 0d087acd8..45fff2400 100644
> > --- a/grub-core/fs/ntfs.c
> > +++ b/grub-core/fs/ntfs.c
> > @@ -246,6 +246,7 @@ fixup (grub_uint8_t *buf, grub_size_t len, const
> grub_uint8_t *magic)
> > grub_uint16_t ss;
> > grub_uint8_t *pu;
> > grub_uint16_t us;
> > + grub_uint16_t pu_offset;
> >
> > COMPILE_TIME_ASSERT ((1 << GRUB_NTFS_BLK_SHR) ==
> GRUB_DISK_SECTOR_SIZE);
> >
> > @@ -255,7 +256,10 @@ fixup (grub_uint8_t *buf, grub_size_t len, const
> grub_uint8_t *magic)
> > ss = u16at (buf, 6) - 1;
> > if (ss != len)
> > return grub_error (GRUB_ERR_BAD_FS, "size not match");
> > - pu = buf + u16at (buf, 4);
> > + pu_offset = u16at (buf, 4);
> > + if (pu_offset >= (len * GRUB_DISK_SECTOR_SIZE - (2 * ss)))
> > + return grub_error (GRUB_ERR_BAD_FS, "pu offset size incorrect");
> > + pu = buf + pu_offset;
> > us = u16at (pu, 0);
> > buf -= 2;
> > while (ss > 0)
> > @@ -373,6 +377,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> >
> > new_pos = &at->emft_buf[first_attr_off (at->emft_buf)];
> > end = &at->emft_buf[emft_buf_size];
> > + at->end = end;
> >
> > while (new_pos && *new_pos != 0xFF)
> > {
> > @@ -395,7 +400,8 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > }
> > at->attr_cur = at->attr_nxt;
> > mft_end = at->mft->buf + (at->mft->data->mft_size <<
> GRUB_NTFS_BLK_SHR);
> > - while (at->attr_cur >= at->mft->buf && at->attr_cur < mft_end &&
> *at->attr_cur != 0xFF)
> > + while (at->attr_cur >= at->mft->buf && at->attr_cur < (mft_end - 4)
> > + && *at->attr_cur != 0xFF)
> > {
> > /* We can't use validate_attribute here because this logic
> > * seems to be used for both parsing through attributes
> > @@ -412,7 +418,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> >
> > if (*at->attr_cur == GRUB_NTFS_AT_ATTRIBUTE_LIST)
> > at->attr_end = at->attr_cur;
> > - if ((*at->attr_cur == attr) || (attr == 0))
> > + if ((*at->attr_cur == attr) || (attr == 0) || (nsize == 0))
> > return at->attr_cur;
> > at->attr_cur = at->attr_nxt;
> > }
> > @@ -442,13 +448,23 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > return NULL;
> > }
> > at->attr_nxt = at->edat_buf;
> > - at->attr_end = at->edat_buf + u32at (pa, 0x30);
> > + grub_uint32_t edat_offset = u32at (pa, 0x30);
> > + if (edat_offset >= n) {
> > + grub_error (GRUB_ERR_BAD_FS, "edat offset is out of bounds");
> > + return NULL;
> > + }
> > + at->attr_end = at->edat_buf + edat_offset;
> > pa_end = at->edat_buf + n;
> > }
> > else
> > {
> > at->attr_nxt = at->attr_end + res_attr_data_off (pa);
> > - at->attr_end = at->attr_end + u32at (pa, 4);
> > + grub_uint32_t edat_offset = u32at (pa, 4);
> > + if ((at->attr_end + edat_offset) >= (at->end)) {
> > + grub_error (GRUB_ERR_BAD_FS, "edat offset is out of bounds");
> > + return NULL;
> > + }
> > + at->attr_end = at->attr_end + edat_offset;
> > pa_end = at->mft->buf + (at->mft->data->mft_size <<
> GRUB_NTFS_BLK_SHR);
> > }
> > at->flags |= GRUB_NTFS_AF_ALST;
> > @@ -470,7 +486,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > at->attr_nxt = NULL;
> > }
> >
> > - if (at->attr_nxt >= at->attr_end || at->attr_nxt == NULL)
> > + if ((at->attr_nxt + GRUB_NTFS_ATTRIBUTE_HEADER_SIZE) >=
> at->attr_end || at->attr_nxt == NULL)
> > return NULL;
> >
> > if ((at->flags & GRUB_NTFS_AF_MMFT) && (attr ==
> GRUB_NTFS_AT_DATA))
> > @@ -537,13 +553,16 @@ locate_attr (struct grub_ntfs_attr *at, struct
> grub_ntfs_file *mft,
> > return NULL;
> > if ((at->flags & GRUB_NTFS_AF_ALST) == 0)
> > {
> > + /* Used to make sure we're not stuck in a loop. */
> > + grub_uint8_t *last_pa = NULL;
>
> Missing empty line...
>
> > while (1)
> > {
> > pa = find_attr (at, attr);
> > - if (pa == NULL)
> > + if (pa == NULL || pa == last_pa)
> > break;
> > if (at->flags & GRUB_NTFS_AF_ALST)
> > return pa;
> > + last_pa = pa;
> > }
> > grub_errno = GRUB_ERR_NONE;
> > free_attr (at);
> > @@ -639,6 +658,7 @@ read_data (struct grub_ntfs_attr *at, grub_uint8_t
> *pa, grub_uint8_t *dest,
> > grub_disk_read_hook_t read_hook, void *read_hook_data)
> > {
> > struct grub_ntfs_rlst cc, *ctx;
> > + grub_uint8_t *end_ptr = (pa + len);
> >
> > if (len == 0)
> > return 0;
> > @@ -671,7 +691,13 @@ read_data (struct grub_ntfs_attr *at, grub_uint8_t
> *pa, grub_uint8_t *dest,
> > return 0;
> > }
> >
> > - ctx->cur_run = pa + u16at (pa, 0x20);
> > + grub_uint16_t run_offset = u16at (pa, 0x20);
>
> Please move variable definition to proper place...
>
> Daniel
>
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
