All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] minix: avoid mistakenly probing ext2 filesystems
@ 2020-05-27  3:09 Daniel Drake
  0 siblings, 0 replies; 3+ messages in thread
From: Daniel Drake @ 2020-05-27  3:09 UTC (permalink / raw)
  To: grub-devel

ext2 (and ext3, ext4) filesystems write the number of free inodes to
location 0x410.

On a minix filesystem, that same location is used for the minix superblock
magic number.

If the number of free inodes on an ext2 filesystem is equal to any
of the four minix superblock magic values plus any multiple of 65536,
grub's minix filesystem code will probe it as a minix filesystem.

In the case of an OS using ext2 as the root filesystem, since there will
ordinarily be some amount of file creation and deletion on every bootup,
it effectively means that this situation has a 1:16384 chance of being hit
on every reboot.

This will cause grub's filesystem probing code to mistakenly identify an
ext2 filesystem as minix. This can be seen by e.g. "search --label"
incorrectly indicating that no such ext2 partition with matching label
exists, whereas in fact it does.

After spotting the rough cause of the issue I was facing here, I borrowed
much of the diagnosis/explanation from meierfra who found and investigated
the same issue in util-linux in 2010:

https://bugs.launchpad.net/ubuntu/+source/util-linux/+bug/518582

This was fixed in util-linux by having the minix code check for the
ext2 magic. Do the same here.
---
 grub-core/fs/minix.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/minix.c b/grub-core/fs/minix.c
index d0d08363c..db0a83feb 100644
--- a/grub-core/fs/minix.c
+++ b/grub-core/fs/minix.c
@@ -38,6 +38,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
 #define GRUB_MINIX_MAGIC_30	0x138F
 #endif
 
+#define	EXT2_MAGIC		0xEF53
+
 #define GRUB_MINIX_INODE_DIR_BLOCKS	7
 #define GRUB_MINIX_LOG2_BSIZE	1
 #define GRUB_MINIX_ROOT_INODE	1
@@ -466,7 +468,22 @@ grub_minix_find_file (struct grub_minix_data *data, const char *path)
 static struct grub_minix_data *
 grub_minix_mount (grub_disk_t disk)
 {
-  struct grub_minix_data *data;
+  struct grub_minix_data *data = NULL;
+  grub_uint16_t ext2_marker;
+
+  grub_disk_read (disk, 1 * 2, 56, sizeof (ext2_marker),
+                  &ext2_marker);
+  if (grub_errno)
+    goto fail;
+
+  /* ext2 filesystems can sometimes be mistakenly identified
+   * as minix, e.g. due to the number of free ext2 inodes being
+   * written to the same location where the minix superblock
+   * magic is found.
+   * Avoid such situations by skipping any filesystems that
+   * have the ext2 superblock magic. */
+  if (ext2_marker == grub_cpu_to_le16_compile_time (EXT2_MAGIC))
+    goto fail;
 
   data = grub_malloc (sizeof (struct grub_minix_data));
   if (!data)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* [PATCH] minix: avoid mistakenly probing ext2 filesystems
@ 2021-03-12 18:05 Derek Foreman
  2021-03-22 16:18 ` Daniel Kiper
  0 siblings, 1 reply; 3+ messages in thread
From: Derek Foreman @ 2021-03-12 18:05 UTC (permalink / raw)
  To: grub-devel; +Cc: linux, Daniel Drake, Derek Foreman

From: Daniel Drake <drake@endlessm.com>

ext2 (and ext3, ext4) filesystems write the number of free inodes to
location 0x410.

On a minix filesystem, that same location is used for the minix superblock
magic number.

If the number of free inodes on an ext2 filesystem is equal to any
of the four minix superblock magic values plus any multiple of 65536,
grub's minix filesystem code will probe it as a minix filesystem.

In the case of an OS using ext2 as the root filesystem, since there will
ordinarily be some amount of file creation and deletion on every bootup,
it effectively means that this situation has a 1:16384 chance of being hit
on every reboot.

This will cause grub's filesystem probing code to mistakenly identify an
ext2 filesystem as minix. This can be seen by e.g. "search --label"
incorrectly indicating that no such ext2 partition with matching label
exists, whereas in fact it does.

After spotting the rough cause of the issue I was facing here, I borrowed
much of the diagnosis/explanation from meierfra who found and investigated
the same issue in util-linux in 2010:

https://bugs.launchpad.net/ubuntu/+source/util-linux/+bug/518582

This was fixed in util-linux by having the minix code check for the
ext2 magic. Do the same here.

Signed-off-by: Daniel Drake <drake@endlessm.com>
Reviewed-by: Derek Foreman <derek@endlessos.org>
---

This bug fix was previously sent to the grub-devel list once before:
 https://lists.gnu.org/archive/html/grub-devel/2020-05/msg00205.html
but received no response, so I'm bring it up again.

If my understanding is correct, the bytes in question overlap with the
"maximum file system size" field in the minix superblock, which will
never contain the ext2 magic byte pattern, so there shouldn't be any
unintended side effects.

 grub-core/fs/minix.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/minix.c b/grub-core/fs/minix.c
index d0d08363c..db0a83feb 100644
--- a/grub-core/fs/minix.c
+++ b/grub-core/fs/minix.c
@@ -38,6 +38,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
 #define GRUB_MINIX_MAGIC_30	0x138F
 #endif
 
+#define	EXT2_MAGIC		0xEF53
+
 #define GRUB_MINIX_INODE_DIR_BLOCKS	7
 #define GRUB_MINIX_LOG2_BSIZE	1
 #define GRUB_MINIX_ROOT_INODE	1
@@ -466,7 +468,22 @@ grub_minix_find_file (struct grub_minix_data *data, const char *path)
 static struct grub_minix_data *
 grub_minix_mount (grub_disk_t disk)
 {
-  struct grub_minix_data *data;
+  struct grub_minix_data *data = NULL;
+  grub_uint16_t ext2_marker;
+
+  grub_disk_read (disk, 1 * 2, 56, sizeof (ext2_marker),
+                  &ext2_marker);
+  if (grub_errno)
+    goto fail;
+
+  /* ext2 filesystems can sometimes be mistakenly identified
+   * as minix, e.g. due to the number of free ext2 inodes being
+   * written to the same location where the minix superblock
+   * magic is found.
+   * Avoid such situations by skipping any filesystems that
+   * have the ext2 superblock magic. */
+  if (ext2_marker == grub_cpu_to_le16_compile_time (EXT2_MAGIC))
+    goto fail;
 
   data = grub_malloc (sizeof (struct grub_minix_data));
   if (!data)
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] minix: avoid mistakenly probing ext2 filesystems
  2021-03-12 18:05 [PATCH] minix: avoid mistakenly probing ext2 filesystems Derek Foreman
@ 2021-03-22 16:18 ` Daniel Kiper
  0 siblings, 0 replies; 3+ messages in thread
From: Daniel Kiper @ 2021-03-22 16:18 UTC (permalink / raw)
  To: Derek Foreman; +Cc: grub-devel, linux, Daniel Drake

On Fri, Mar 12, 2021 at 12:05:08PM -0600, Derek Foreman wrote:
> From: Daniel Drake <drake@endlessm.com>
>
> ext2 (and ext3, ext4) filesystems write the number of free inodes to
> location 0x410.
>
> On a minix filesystem, that same location is used for the minix superblock
> magic number.
>
> If the number of free inodes on an ext2 filesystem is equal to any
> of the four minix superblock magic values plus any multiple of 65536,
> grub's minix filesystem code will probe it as a minix filesystem.
>
> In the case of an OS using ext2 as the root filesystem, since there will
> ordinarily be some amount of file creation and deletion on every bootup,
> it effectively means that this situation has a 1:16384 chance of being hit
> on every reboot.
>
> This will cause grub's filesystem probing code to mistakenly identify an
> ext2 filesystem as minix. This can be seen by e.g. "search --label"
> incorrectly indicating that no such ext2 partition with matching label
> exists, whereas in fact it does.
>
> After spotting the rough cause of the issue I was facing here, I borrowed
> much of the diagnosis/explanation from meierfra who found and investigated
> the same issue in util-linux in 2010:
>
> https://bugs.launchpad.net/ubuntu/+source/util-linux/+bug/518582
>
> This was fixed in util-linux by having the minix code check for the
> ext2 magic. Do the same here.
>
> Signed-off-by: Daniel Drake <drake@endlessm.com>
> Reviewed-by: Derek Foreman <derek@endlessos.org>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

> ---
>
> This bug fix was previously sent to the grub-devel list once before:
>  https://lists.gnu.org/archive/html/grub-devel/2020-05/msg00205.html
> but received no response, so I'm bring it up again.
>
> If my understanding is correct, the bytes in question overlap with the
> "maximum file system size" field in the minix superblock, which will
> never contain the ext2 magic byte pattern, so there shouldn't be any
> unintended side effects.
>
>  grub-core/fs/minix.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/fs/minix.c b/grub-core/fs/minix.c
> index d0d08363c..db0a83feb 100644
> --- a/grub-core/fs/minix.c
> +++ b/grub-core/fs/minix.c
> @@ -38,6 +38,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define GRUB_MINIX_MAGIC_30	0x138F
>  #endif
>
> +#define	EXT2_MAGIC		0xEF53
> +
>  #define GRUB_MINIX_INODE_DIR_BLOCKS	7
>  #define GRUB_MINIX_LOG2_BSIZE	1
>  #define GRUB_MINIX_ROOT_INODE	1
> @@ -466,7 +468,22 @@ grub_minix_find_file (struct grub_minix_data *data, const char *path)
>  static struct grub_minix_data *
>  grub_minix_mount (grub_disk_t disk)
>  {
> -  struct grub_minix_data *data;
> +  struct grub_minix_data *data = NULL;
> +  grub_uint16_t ext2_marker;
> +
> +  grub_disk_read (disk, 1 * 2, 56, sizeof (ext2_marker),
> +                  &ext2_marker);
> +  if (grub_errno)
> +    goto fail;
> +
> +  /* ext2 filesystems can sometimes be mistakenly identified
> +   * as minix, e.g. due to the number of free ext2 inodes being
> +   * written to the same location where the minix superblock
> +   * magic is found.
> +   * Avoid such situations by skipping any filesystems that
> +   * have the ext2 superblock magic. */
> +  if (ext2_marker == grub_cpu_to_le16_compile_time (EXT2_MAGIC))
> +    goto fail;
>
>    data = grub_malloc (sizeof (struct grub_minix_data));
>    if (!data)
> --
> 2.26.2
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-22 16:19 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-12 18:05 [PATCH] minix: avoid mistakenly probing ext2 filesystems Derek Foreman
2021-03-22 16:18 ` Daniel Kiper
  -- strict thread matches above, loose matches on Subject: below --
2020-05-27  3:09 Daniel Drake

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.