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
