All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
@ 2025-02-28 13:07 B Horn
  2025-02-28 20:58 ` Andreas Klauer
  2025-02-28 22:17 ` Vladimir 'phcoder' Serbinenko
  0 siblings, 2 replies; 9+ messages in thread
From: B Horn @ 2025-02-28 13:07 UTC (permalink / raw)
  To: grub-devel; +Cc: B Horn, Andreas Klauer

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

Reported-by: Andreas Klauer <andreas.klauer@metamorpher.de>
Signed-off-by: B Horn <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
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-02-28 13:07 [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute() B Horn
@ 2025-02-28 20:58 ` Andreas Klauer
  2025-02-28 22:17 ` Vladimir 'phcoder' Serbinenko
  1 sibling, 0 replies; 9+ messages in thread
From: Andreas Klauer @ 2025-02-28 20:58 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: B Horn, Andreas Klauer

On Fri, Feb 28, 2025 at 01:07:06PM +0000, B Horn wrote:
> 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
> 
> Reported-by: Andreas Klauer <andreas.klauer@metamorpher.de>
> Signed-off-by: B Horn <b@horn.uk>

Thank you, it works fine here.

Regards
Andreas

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-02-28 13:07 [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute() B Horn
  2025-02-28 20:58 ` Andreas Klauer
@ 2025-02-28 22:17 ` Vladimir 'phcoder' Serbinenko
  2025-03-01  1:09   ` B Horn
  1 sibling, 1 reply; 9+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2025-02-28 22:17 UTC (permalink / raw)
  To: The development of GNU GRUB


[-- Attachment #1.1: Type: text/plain, Size: 2210 bytes --]

Why do you remove boundary check? Maybe it's better to add the second
boundary check from below

Le ven. 28 févr. 2025, 16:11, B Horn <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
>
> Reported-by: Andreas Klauer <andreas.klauer@metamorpher.de>
> Signed-off-by: B Horn <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
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 3159 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-02-28 22:17 ` Vladimir 'phcoder' Serbinenko
@ 2025-03-01  1:09   ` B Horn
  2025-03-10 12:19     ` Marta Lewandowska
  2025-03-17 12:47     ` Andrew Hamilton
  0 siblings, 2 replies; 9+ messages in thread
From: B Horn @ 2025-03-01  1:09 UTC (permalink / raw)
  To: grub-devel; +Cc: phcoder


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.klauer@metamorpher.de
>     <mailto:andreas.klauer@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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-03-01  1:09   ` B Horn
@ 2025-03-10 12:19     ` Marta Lewandowska
  2025-03-17 12:47     ` Andrew Hamilton
  1 sibling, 0 replies; 9+ messages in thread
From: Marta Lewandowska @ 2025-03-10 12:19 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: phcoder


[-- Attachment #1.1: Type: text/plain, Size: 4242 bytes --]

Hi,

This patch also fixes an issue introduced by the original patch involving
dual booting: grub-mount seg faults when trying to mount an ntfs volume,
e.g., when grub-mkconfig is run. There are bugs for this on fedora [1] and
arch [2], so further review of this patch is really appreciated!

thanks!
marta

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2350327
[2]
https://gitlab.archlinux.org/archlinux/packaging/packages/grub/-/issues/11

On Sat, Mar 1, 2025 at 2:10 AM 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.klauer@metamorpher.de
> >     <mailto:andreas.klauer@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
>

[-- Attachment #1.2: Type: text/html, Size: 6674 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-03-01  1:09   ` B Horn
  2025-03-10 12:19     ` Marta Lewandowska
@ 2025-03-17 12:47     ` Andrew Hamilton
  2025-03-18 14:22       ` Andrew Hamilton
  1 sibling, 1 reply; 9+ messages in thread
From: Andrew Hamilton @ 2025-03-17 12:47 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: phcoder


[-- Attachment #1.1: Type: text/plain, Size: 4066 bytes --]

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.klauer@metamorpher.de
> >     <mailto:andreas.klauer@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
>

[-- Attachment #1.2: Type: text/html, Size: 6282 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-03-17 12:47     ` Andrew Hamilton
@ 2025-03-18 14:22       ` Andrew Hamilton
  2025-03-20 16:45         ` Daniel Kiper
  0 siblings, 1 reply; 9+ messages in thread
From: Andrew Hamilton @ 2025-03-18 14:22 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: phcoder


[-- Attachment #1.1: Type: text/plain, Size: 4583 bytes --]

Unless someone else is already working on this, I started working on a
patch for this as well as fixes to correct the NTFS Grub test failures -
I’ll hopefully submit something for review in a few days.

Once it is available - any additional testing that can be done would be
appreciated.

Thanks,
Andrew

On Mon, Mar 17, 2025 at 7:47 AM Andrew Hamilton <adhamilt@gmail.com> wrote:

> 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.klauer@metamorpher.de
>> >     <mailto:andreas.klauer@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
>>
>

[-- Attachment #1.2: Type: text/html, Size: 7070 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-03-18 14:22       ` Andrew Hamilton
@ 2025-03-20 16:45         ` Daniel Kiper
  2025-03-20 16:54           ` Andrew Hamilton
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Kiper @ 2025-03-20 16:45 UTC (permalink / raw)
  To: Andrew Hamilton; +Cc: grub-devel, phcoder

On Tue, Mar 18, 2025 at 09:22:46AM -0500, Andrew Hamilton wrote:
> Unless someone else is already working on this, I started working on a patch
> for this as well as fixes to correct the NTFS Grub test failures - I’ll
> hopefully submit something for review in a few days.
>
> Once it is available - any additional testing that can be done would be
> appreciated.

Andrew, please post B Horn's updated patch taking into account Vladimir's
suggestion. It looks the issue is hitting more and more people.

It would be nice if you CC folks who are involved in the discussion here
and in another thread [1].

Daniel

[1] https://lore.kernel.org/grub-devel/be054d5caca7956d5b352d2526b947b6368cca67.camel@z-51.de/T/#mbc3b22d20d31d2765fbc4a2ac205194041dfd6b7

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute()
  2025-03-20 16:45         ` Daniel Kiper
@ 2025-03-20 16:54           ` Andrew Hamilton
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Hamilton @ 2025-03-20 16:54 UTC (permalink / raw)
  To: Daniel Kiper; +Cc: grub-devel, phcoder


[-- Attachment #1.1: Type: text/plain, Size: 1209 bytes --]

Will do! I’ll do this later today after work. I was attempting to chase
down some additional ntfs test failures (around hard links on an ntfs file
system) even with the fix mentioned but maybe to expedite the main issue
people are having I’ll put that on my queue for after this first fix.

Thanks!
Andrew

On Thu, Mar 20, 2025 at 11:45 AM Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Mar 18, 2025 at 09:22:46AM -0500, Andrew Hamilton wrote:
> > Unless someone else is already working on this, I started working on a
> patch
> > for this as well as fixes to correct the NTFS Grub test failures - I’ll
> > hopefully submit something for review in a few days.
> >
> > Once it is available - any additional testing that can be done would be
> > appreciated.
>
> Andrew, please post B Horn's updated patch taking into account Vladimir's
> suggestion. It looks the issue is hitting more and more people.
>
> It would be nice if you CC folks who are involved in the discussion here
> and in another thread [1].
>
> Daniel
>
> [1]
> https://lore.kernel.org/grub-devel/be054d5caca7956d5b352d2526b947b6368cca67.camel@z-51.de/T/#mbc3b22d20d31d2765fbc4a2ac205194041dfd6b7
>

[-- Attachment #1.2: Type: text/html, Size: 1801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-03-20 16:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-28 13:07 [PATCH] fs/ntfs: Check at->attr_cur after calling next_attribute() B Horn
2025-02-28 20:58 ` Andreas Klauer
2025-02-28 22:17 ` Vladimir 'phcoder' Serbinenko
2025-03-01  1:09   ` B Horn
2025-03-10 12:19     ` Marta Lewandowska
2025-03-17 12:47     ` Andrew Hamilton
2025-03-18 14:22       ` Andrew Hamilton
2025-03-20 16:45         ` Daniel Kiper
2025-03-20 16:54           ` Andrew Hamilton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.