> May I ask you to stick to the GRUB coding style? [1]
I have corrected the comment style in v3.
>
> > + at->attr_nxt = next_attribute (at->attr_cur, at->attr_end,
false);
> > if ((*at->attr_cur == attr) || (attr == 0))
> > {
> > grub_uint8_t *new_pos, *end;
> > @@ -378,7 +381,9 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
attr)
> > {
> > return new_pos;
> > }
> > - new_pos = next_attribute (new_pos, end);
> > + /* Go to the next attribute in the list but do not
validate */
> > + /* because this is the attribute list type. */
>
> Ditto.
I have corrected the comment style in v3.
>
> > + new_pos = next_attribute (new_pos, end, false);
> > }
> > grub_error (GRUB_ERR_BAD_FS,
> > "can\'t find 0x%X in attribute list",
> > @@ -392,7 +397,19 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
attr)
> > 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)
> > {
> > - at->attr_nxt = next_attribute (at->attr_cur, at->end);
> > + /* We can't use validate_attribute here because this logic
> > + * seems to be used for both parsing through attributes
> > + * and attribute lists. */
>
> Ditto.
I have corrected the comment style in v3.
>
> > + grub_uint16_t nsize = u16at (at->attr_cur, 4);
>
> Please do not mix code with variable definitions. Define variables (and
> initialize if needed) at the beginning of a function or a given block.
> Separate definitions with code with one empty line.
I have made this change in v3.
>
> > + if (at->attr_cur + nsize >= at->end ||
> > + at->attr_cur + GRUB_NTFS_ATTRIBUTE_HEADER_SIZE >= at->end)
>
> if (at->attr_cur + grub_max (GRUB_NTFS_ATTRIBUTE_HEADER_SIZE, nsize) >=
at->end))
I have made this change in v3.
>
> > + {
> > + at->attr_nxt = at->attr_cur;
> > + break;
> > + }
> > + else
> > + at->attr_nxt = at->attr_cur + nsize;
>
> Is there any chance for overflow here? And in the "if" above?
I don't see a way for an overflow to realistically happen here since this is
doing pointer math to calculate a new address and the value being added is
16
bits max. I assume we're at least on a 32-bit address system so we should
be
okay assuming the memory isn't allocated less than 65536 bytes from the end
(0xFFFFFFFF).
>
> > if (*at->attr_cur == GRUB_NTFS_AT_ATTRIBUTE_LIST)
> > at->attr_end = at->attr_cur;
> > if ((*at->attr_cur == attr) || (attr == 0))
> > @@ -439,14 +456,18 @@ find_attr (struct grub_ntfs_attr *at,
grub_uint8_t attr)
> > /* From this point on pa_end is the end of the buffer */
> > at->end = pa_end;
> >
> > - if (validate_attribute (at->attr_nxt, pa_end) == false)
> > - return NULL;
> > + if (at->attr_end >= pa_end || at->attr_nxt >= pa_end)
> > + return NULL;
> >
> > while (at->attr_nxt)
> > {
> > if ((*at->attr_nxt == attr) || (attr == 0))
> > break;
> > - at->attr_nxt = next_attribute (at->attr_nxt, pa_end);
> > +
> > + grub_uint16_t nxt_offset = u16at (at->attr_nxt, 4);
>
> Again, please move this to the beginning of this code block.
I have made this change in v3.
>
> > + at->attr_nxt += nxt_offset;
> > + if (nxt_offset == 0 || at->attr_nxt >= (pa_end - 4))
>
> I think at least "(pa_end - 4)" begs for explanation in the comment.
Added commentary for this in v3.
>
> > + at->attr_nxt = NULL;
> > }
> >
> > if (at->attr_nxt >= at->attr_end || at->attr_nxt == NULL)
> > @@ -471,7 +492,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
attr)
> > + 1));
> > pa = at->attr_nxt + u16at (pa, 4);
> >
> > - if (validate_attribute (pa, pa_end) == true)
> > + if (pa >= pa_end)
> > pa = NULL;
> >
> > while (pa)
> > @@ -490,7 +511,9 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
attr)
> > u32at (pa, 0x10) * (at->mft->data->mft_size <<
GRUB_NTFS_BLK_SHR),
> > at->mft->data->mft_size << GRUB_NTFS_BLK_SHR, 0, 0, 0))
> > return NULL;
> > - pa = next_attribute (pa, pa_end);
> > + pa += u16at(pa, 4);
>
> When you introduce math are you sure there is no chance for overflow?
I think we should be safe from overflow here since a bit above this there is
a check that includes "if ((pa >= pa_end) ||" that will trigger an error. So
for a minimum 32-bit address system we should be safe since this is pointer
arithmetic adding a 16-bit value.
>
> And a nit, missing space before "("...
Corrected this in v3.
Thanks,
Andrew
On Tue, May 20, 2025 at 10:52 AM Daniel Kiper <[email protected]>
wrote:
> On Mon, May 19, 2025 at 09:03:16PM -0500, Andrew Hamilton wrote:
> > Correct ntfs_test test failures around attempting to validate attribute
> > list entries as attributes. The NTFS code uses common logic in some
> > places to parse both attributes and attribute_lists which complicates
> > validation. Attribute lists contain different headers including a
> > different size of the length field (2 bytes) at offset 4 instead of the
> > 4 byte length field used in attributes at offset 4. There are other
> > differences as well, but attempting to validate attribute list types
> > using attribute header validation was causing failure of the NTFS test
> > suite. This change restores some of the validation logic which may be
> > shared between attributes and attribute lists to be closer to the
> > original logic prior to fixes for previous CVEs. A following commit will
> > address some of the implications of removing this validation logic by
> > correcting some fuzzer failures (some which are exposed by removing the
> > validation in some of the cases).
> >
> > Fixes: 067b6d225 (fs/ntfs: Implement attribute verification)
> >
> > Signed-off-by: Andrew Hamilton <[email protected]>
> > ---
> > grub-core/fs/ntfs.c | 45 ++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
> > index 3eb70111b..0d087acd8 100644
> > --- a/grub-core/fs/ntfs.c
> > +++ b/grub-core/fs/ntfs.c
> > @@ -219,7 +219,7 @@ validate_attribute (grub_uint8_t *attr, void *end)
> >
> > /* Return the next attribute if it exists, otherwise return NULL. */
> > static grub_uint8_t *
> > -next_attribute (grub_uint8_t *curr_attribute, void *end)
> > +next_attribute (grub_uint8_t *curr_attribute, void *end, bool validate)
> > {
> > grub_uint8_t *next = curr_attribute;
> >
> > @@ -231,7 +231,7 @@ next_attribute (grub_uint8_t *curr_attribute, void
> *end)
> > return NULL;
> >
> > next += u16at (curr_attribute, 4);
> > - if (validate_attribute (next, end) == false)
> > + if (validate && validate_attribute (next, end) == false)
> > return NULL;
> >
> > return next;
> > @@ -316,13 +316,16 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > {
> > grub_uint8_t *mft_end;
> >
> > + /* GRUB_NTFS_AF_ALST indicates the attribute list type */
> > if (at->flags & GRUB_NTFS_AF_ALST)
> > {
> > retry:
> > while (at->attr_nxt)
> > {
> > at->attr_cur = at->attr_nxt;
> > - at->attr_nxt = next_attribute (at->attr_cur, at->attr_end);
> > + /* Go to the next attribute in the list but do not validate */
> > + /* because this is the attribute list type. */
>
> May I ask you to stick to the GRUB coding style? [1]
>
> > + at->attr_nxt = next_attribute (at->attr_cur, at->attr_end,
> false);
> > if ((*at->attr_cur == attr) || (attr == 0))
> > {
> > grub_uint8_t *new_pos, *end;
> > @@ -378,7 +381,9 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > {
> > return new_pos;
> > }
> > - new_pos = next_attribute (new_pos, end);
> > + /* Go to the next attribute in the list but do not
> validate */
> > + /* because this is the attribute list type. */
>
> Ditto.
>
> > + new_pos = next_attribute (new_pos, end, false);
> > }
> > grub_error (GRUB_ERR_BAD_FS,
> > "can\'t find 0x%X in attribute list",
> > @@ -392,7 +397,19 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > 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)
> > {
> > - at->attr_nxt = next_attribute (at->attr_cur, at->end);
> > + /* We can't use validate_attribute here because this logic
> > + * seems to be used for both parsing through attributes
> > + * and attribute lists. */
>
> Ditto.
>
> > + grub_uint16_t nsize = u16at (at->attr_cur, 4);
>
> Please do not mix code with variable definitions. Define variables (and
> initialize if needed) at the beginning of a function or a given block.
> Separate definitions with code with one empty line.
>
> > + if (at->attr_cur + nsize >= at->end ||
> > + at->attr_cur + GRUB_NTFS_ATTRIBUTE_HEADER_SIZE >= at->end)
>
> if (at->attr_cur + grub_max (GRUB_NTFS_ATTRIBUTE_HEADER_SIZE, nsize) >=
> at->end))
>
> > + {
> > + at->attr_nxt = at->attr_cur;
> > + break;
> > + }
> > + else
> > + at->attr_nxt = at->attr_cur + nsize;
>
> Is there any chance for overflow here? And in the "if" above?
>
> > if (*at->attr_cur == GRUB_NTFS_AT_ATTRIBUTE_LIST)
> > at->attr_end = at->attr_cur;
> > if ((*at->attr_cur == attr) || (attr == 0))
> > @@ -439,14 +456,18 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > /* From this point on pa_end is the end of the buffer */
> > at->end = pa_end;
> >
> > - if (validate_attribute (at->attr_nxt, pa_end) == false)
> > - return NULL;
> > + if (at->attr_end >= pa_end || at->attr_nxt >= pa_end)
> > + return NULL;
> >
> > while (at->attr_nxt)
> > {
> > if ((*at->attr_nxt == attr) || (attr == 0))
> > break;
> > - at->attr_nxt = next_attribute (at->attr_nxt, pa_end);
> > +
> > + grub_uint16_t nxt_offset = u16at (at->attr_nxt, 4);
>
> Again, please move this to the beginning of this code block.
>
> > + at->attr_nxt += nxt_offset;
> > + if (nxt_offset == 0 || at->attr_nxt >= (pa_end - 4))
>
> I think at least "(pa_end - 4)" begs for explanation in the comment.
>
> > + at->attr_nxt = NULL;
> > }
> >
> > if (at->attr_nxt >= at->attr_end || at->attr_nxt == NULL)
> > @@ -471,7 +492,7 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > + 1));
> > pa = at->attr_nxt + u16at (pa, 4);
> >
> > - if (validate_attribute (pa, pa_end) == true)
> > + if (pa >= pa_end)
> > pa = NULL;
> >
> > while (pa)
> > @@ -490,7 +511,9 @@ find_attr (struct grub_ntfs_attr *at, grub_uint8_t
> attr)
> > u32at (pa, 0x10) * (at->mft->data->mft_size <<
> GRUB_NTFS_BLK_SHR),
> > at->mft->data->mft_size << GRUB_NTFS_BLK_SHR, 0, 0, 0))
> > return NULL;
> > - pa = next_attribute (pa, pa_end);
> > + pa += u16at(pa, 4);
>
> When you introduce math are you sure there is no chance for overflow?
>
> And a nit, missing space before "("...
>
> Daniel
>
> [1]
> https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments
>
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
