* [PATCH 0/4] Prevent out-of-bound reads
@ 2023-04-20 17:59 Lidong Chen
2023-04-20 17:59 ` [PATCH 1/4] fs/hfsplus: Validate btree node size Lidong Chen
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Lidong Chen @ 2023-04-20 17:59 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, lidong.chen
This set of patches adds checks to ensure the node
size is valid before accessing it. In addition, error
messages are marked for translation.
Lidong Chen (4):
fs/hfsplus: Validate btree node size
fs/hfsplus: Prevent out of bound access in catalog file
fs/hfsplus: Set grub errno to prevent NULL pointer access
fs/hfsplus: Mark error strings for translation
grub-core/fs/hfsplus.c | 49 +++++++++++++++++++++++++++++++-----------
1 file changed, 36 insertions(+), 13 deletions(-)
--
2.39.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] fs/hfsplus: Validate btree node size
2023-04-20 17:59 [PATCH 0/4] Prevent out-of-bound reads Lidong Chen
@ 2023-04-20 17:59 ` Lidong Chen
2023-04-20 22:08 ` Vladimir 'phcoder' Serbinenko
2023-04-20 17:59 ` [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file Lidong Chen
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Lidong Chen @ 2023-04-20 17:59 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, lidong.chen
The invalid btree node size can cause crashes when parsing
the btree. The fix is to ensure the btree node size is within
the valid range defined in the HFS Plus techical note, TN1150.
https://developer.apple.com/library/archive/technotes/tn/tn1150.html
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/hfsplus.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 6337cbfcb..1ffebc8be 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -84,6 +84,9 @@ struct grub_hfsplus_catfile
#define GRUB_HFSPLUS_FILEMODE_DIRECTORY 0040000
#define GRUB_HFSPLUS_FILEMODE_SYMLINK 0120000
+#define HFSPLUS_BTNODE_MINSZ (1 << 9)
+#define HFSPLUS_BTNODE_MAXSZ (1 << 15)
+
/* Some pre-defined file IDs. */
enum
{
@@ -584,6 +587,10 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
return 0;
}
+ if (btree->nodesize < HFSPLUS_BTNODE_MINSZ ||
+ btree->nodesize > HFSPLUS_BTNODE_MAXSZ)
+ return grub_error (GRUB_ERR_BAD_FS, "invalid HFS+ btree node size");
+
node = grub_malloc (btree->nodesize);
if (! node)
return grub_errno;
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file
2023-04-20 17:59 [PATCH 0/4] Prevent out-of-bound reads Lidong Chen
2023-04-20 17:59 ` [PATCH 1/4] fs/hfsplus: Validate btree node size Lidong Chen
@ 2023-04-20 17:59 ` Lidong Chen
2023-04-20 22:09 ` Vladimir 'phcoder' Serbinenko
2023-04-24 20:48 ` Lidong Chen
2023-04-20 17:59 ` [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access Lidong Chen
2023-04-20 17:59 ` [PATCH 4/4] fs/hfsplus: Mark error strings for translation Lidong Chen
3 siblings, 2 replies; 13+ messages in thread
From: Lidong Chen @ 2023-04-20 17:59 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, lidong.chen
A corrupted hfsplus can have a catalog key that is out of range.
This can lead to out of bound access when advancing the pointer to
access catalog file info.
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/hfsplus.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 1ffebc8be..9c1f12574 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -87,6 +87,9 @@ struct grub_hfsplus_catfile
#define HFSPLUS_BTNODE_MINSZ (1 << 9)
#define HFSPLUS_BTNODE_MAXSZ (1 << 15)
+#define HFSPLUS_CATKEY_MIN_LEN 6
+#define HFSPLUS_CATKEY_MAX_LEN 516
+
/* Some pre-defined file IDs. */
enum
{
@@ -699,6 +702,13 @@ list_nodes (void *record, void *hook_arg)
catkey = (struct grub_hfsplus_catkey *) record;
+ if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
+ grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
+ {
+ grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
+ return 1;
+ }
+
fileinfo =
(struct grub_hfsplus_catfile *) ((char *) record
+ grub_be_to_cpu16 (catkey->keylen)
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access
2023-04-20 17:59 [PATCH 0/4] Prevent out-of-bound reads Lidong Chen
2023-04-20 17:59 ` [PATCH 1/4] fs/hfsplus: Validate btree node size Lidong Chen
2023-04-20 17:59 ` [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file Lidong Chen
@ 2023-04-20 17:59 ` Lidong Chen
2023-04-20 22:07 ` Vladimir 'phcoder' Serbinenko
2023-04-20 17:59 ` [PATCH 4/4] fs/hfsplus: Mark error strings for translation Lidong Chen
3 siblings, 1 reply; 13+ messages in thread
From: Lidong Chen @ 2023-04-20 17:59 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, lidong.chen
When an invalid node size is detected in grub_hfsplus_mount(), data pinter
is freed. Thus, file->data is not set. The code should also set the
grub error when that happens to indicate an error and to avoid accessing
the unintialized file->data in grub_file_close().
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/hfsplus.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 9c1f12574..cf13e8a63 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -357,7 +357,10 @@ grub_hfsplus_mount (grub_disk_t disk)
(header.key_compare == GRUB_HFSPLUSX_BINARYCOMPARE));
if (data->catalog_tree.nodesize < 2)
- goto fail;
+ {
+ grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
+ goto fail;
+ }
if (grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, 0,
sizeof (struct grub_hfsplus_btnode),
@@ -374,7 +377,10 @@ grub_hfsplus_mount (grub_disk_t disk)
data->extoverflow_tree.nodesize = grub_be_to_cpu16 (header.nodesize);
if (data->extoverflow_tree.nodesize < 2)
- goto fail;
+ {
+ grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
+ goto fail;
+ }
data->extoverflow_tree_ready = 1;
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] fs/hfsplus: Mark error strings for translation
2023-04-20 17:59 [PATCH 0/4] Prevent out-of-bound reads Lidong Chen
` (2 preceding siblings ...)
2023-04-20 17:59 ` [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access Lidong Chen
@ 2023-04-20 17:59 ` Lidong Chen
2023-04-20 22:03 ` Vladimir 'phcoder' Serbinenko
3 siblings, 1 reply; 13+ messages in thread
From: Lidong Chen @ 2023-04-20 17:59 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, lidong.chen
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/hfsplus.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index cf13e8a63..f91af1477 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -179,7 +179,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
if (node->fileid == GRUB_HFSPLUS_FILEID_OVERFLOW)
{
grub_error (GRUB_ERR_READ_ERROR,
- "extra extents found in an extend overflow file");
+ N_("extra extents found in an extend overflow file"));
break;
}
@@ -190,7 +190,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
if (!node->data->extoverflow_tree_ready)
{
grub_error (GRUB_ERR_BAD_FS,
- "attempted to read extent overflow tree before loading");
+ N_("attempted to read extent overflow tree before loading"));
break;
}
@@ -205,8 +205,8 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
|| !nnode)
{
grub_error (GRUB_ERR_READ_ERROR,
- "no block found for the file id 0x%x and the block"
- " offset 0x%" PRIuGRUB_UINT64_T,
+ N_("no block found for the file id 0x%x and the block"
+ " offset 0x%" PRIuGRUB_UINT64_T),
node->fileid, fileblock);
break;
}
@@ -277,7 +277,7 @@ grub_hfsplus_mount (grub_disk_t disk)
/* See if there's an embedded HFS+ filesystem. */
if (grub_be_to_cpu16 (volheader.hfs.embed_sig) != GRUB_HFSPLUS_MAGIC)
{
- grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
+ grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
goto fail;
}
@@ -303,7 +303,7 @@ grub_hfsplus_mount (grub_disk_t disk)
|| ((volheader.hfsplus.blksize & (volheader.hfsplus.blksize - 1)) != 0)
|| grub_be_to_cpu32 (volheader.hfsplus.blksize) < GRUB_DISK_SECTOR_SIZE)
{
- grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
+ grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
goto fail;
}
@@ -358,7 +358,7 @@ grub_hfsplus_mount (grub_disk_t disk)
if (data->catalog_tree.nodesize < 2)
{
- grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
+ grub_error (GRUB_ERR_BAD_FS, N_("invalid catalog node size"));
goto fail;
}
@@ -378,7 +378,7 @@ grub_hfsplus_mount (grub_disk_t disk)
if (data->extoverflow_tree.nodesize < 2)
{
- grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
+ grub_error (GRUB_ERR_BAD_FS, N_("invalid extents overflow node size"));
goto fail;
}
@@ -406,7 +406,7 @@ grub_hfsplus_mount (grub_disk_t disk)
fail:
if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
- grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
+ grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
grub_free (data);
return 0;
@@ -550,7 +550,7 @@ grub_hfsplus_btree_iterate_node (struct grub_hfsplus_btree *btree,
if (node_count && first_node->next == saved_node)
{
- grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
+ grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
return 0;
}
if (!(node_count & (node_count - 1)))
@@ -598,7 +598,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
if (btree->nodesize < HFSPLUS_BTNODE_MINSZ ||
btree->nodesize > HFSPLUS_BTNODE_MAXSZ)
- return grub_error (GRUB_ERR_BAD_FS, "invalid HFS+ btree node size");
+ return grub_error (GRUB_ERR_BAD_FS, N_("invalid HFS+ btree node size"));
node = grub_malloc (btree->nodesize);
if (! node)
@@ -613,7 +613,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
if (save_node == currnode)
{
grub_free (node);
- return grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
+ return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
}
if (!(node_count & (node_count - 1)))
save_node = currnode;
@@ -626,7 +626,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
btree->nodesize, (char *) node) <= 0)
{
grub_free (node);
- return grub_error (GRUB_ERR_BAD_FS, "couldn't read i-node");
+ return grub_error (GRUB_ERR_BAD_FS, N_("couldn't read i-node"));
}
nodedesc = (struct grub_hfsplus_btnode *) node;
@@ -668,7 +668,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
+ 2);
if ((char *) pointer > node + btree->nodesize - 2)
- return grub_error (GRUB_ERR_BAD_FS, "HFS+ key beyond end of node");
+ return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ key beyond end of node"));
currnode = grub_be_to_cpu32 (grub_get_unaligned32 (pointer));
match = 1;
@@ -711,7 +711,7 @@ list_nodes (void *record, void *hook_arg)
if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
{
- grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
+ grub_error (GRUB_ERR_BAD_FS, N_("catalog key length is out of range"));
return 1;
}
--
2.39.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] fs/hfsplus: Mark error strings for translation
2023-04-20 17:59 ` [PATCH 4/4] fs/hfsplus: Mark error strings for translation Lidong Chen
@ 2023-04-20 22:03 ` Vladimir 'phcoder' Serbinenko
2023-04-21 13:10 ` Daniel Kiper
0 siblings, 1 reply; 13+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2023-04-20 22:03 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: daniel.kiper, lidong.chen
Is there any reason to translate those strings? They refer to deep
HFS+ structures and problems and to common people they are meaningless
even in their language. And someone in IT is likely to understand
those concepts in English. I wouldn't know a word for "extent" in my
native language. And those errors are very rare as they indicate a
corrupted FS in the files GRUB cares about. OTOH translating them
requires a lot of effort on translator's side for little benefit. They
were not forgotten. They were not marked for translation on purpose.
If we mark all errors for translation we will make our already large
list of strings 10x larger and 90% of it are hyper-obscure error
conditions. Error conditions are translated only if they are likely to
occur and indicate a resolution direction like "linux command needs to
be run before initrd command"
On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com> wrote:
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
> grub-core/fs/hfsplus.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
> index cf13e8a63..f91af1477 100644
> --- a/grub-core/fs/hfsplus.c
> +++ b/grub-core/fs/hfsplus.c
> @@ -179,7 +179,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> if (node->fileid == GRUB_HFSPLUS_FILEID_OVERFLOW)
> {
> grub_error (GRUB_ERR_READ_ERROR,
> - "extra extents found in an extend overflow file");
> + N_("extra extents found in an extend overflow file"));
> break;
> }
>
> @@ -190,7 +190,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> if (!node->data->extoverflow_tree_ready)
> {
> grub_error (GRUB_ERR_BAD_FS,
> - "attempted to read extent overflow tree before loading");
> + N_("attempted to read extent overflow tree before loading"));
> break;
> }
>
> @@ -205,8 +205,8 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> || !nnode)
> {
> grub_error (GRUB_ERR_READ_ERROR,
> - "no block found for the file id 0x%x and the block"
> - " offset 0x%" PRIuGRUB_UINT64_T,
> + N_("no block found for the file id 0x%x and the block"
> + " offset 0x%" PRIuGRUB_UINT64_T),
> node->fileid, fileblock);
> break;
> }
> @@ -277,7 +277,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> /* See if there's an embedded HFS+ filesystem. */
> if (grub_be_to_cpu16 (volheader.hfs.embed_sig) != GRUB_HFSPLUS_MAGIC)
> {
> - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
> + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
> goto fail;
> }
>
> @@ -303,7 +303,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> || ((volheader.hfsplus.blksize & (volheader.hfsplus.blksize - 1)) != 0)
> || grub_be_to_cpu32 (volheader.hfsplus.blksize) < GRUB_DISK_SECTOR_SIZE)
> {
> - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
> + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
> goto fail;
> }
>
> @@ -358,7 +358,7 @@ grub_hfsplus_mount (grub_disk_t disk)
>
> if (data->catalog_tree.nodesize < 2)
> {
> - grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
> + grub_error (GRUB_ERR_BAD_FS, N_("invalid catalog node size"));
> goto fail;
> }
>
> @@ -378,7 +378,7 @@ grub_hfsplus_mount (grub_disk_t disk)
>
> if (data->extoverflow_tree.nodesize < 2)
> {
> - grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
> + grub_error (GRUB_ERR_BAD_FS, N_("invalid extents overflow node size"));
> goto fail;
> }
>
> @@ -406,7 +406,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> fail:
>
> if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
> - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
> + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
>
> grub_free (data);
> return 0;
> @@ -550,7 +550,7 @@ grub_hfsplus_btree_iterate_node (struct grub_hfsplus_btree *btree,
>
> if (node_count && first_node->next == saved_node)
> {
> - grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
> + grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
> return 0;
> }
> if (!(node_count & (node_count - 1)))
> @@ -598,7 +598,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
>
> if (btree->nodesize < HFSPLUS_BTNODE_MINSZ ||
> btree->nodesize > HFSPLUS_BTNODE_MAXSZ)
> - return grub_error (GRUB_ERR_BAD_FS, "invalid HFS+ btree node size");
> + return grub_error (GRUB_ERR_BAD_FS, N_("invalid HFS+ btree node size"));
>
> node = grub_malloc (btree->nodesize);
> if (! node)
> @@ -613,7 +613,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> if (save_node == currnode)
> {
> grub_free (node);
> - return grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
> + return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
> }
> if (!(node_count & (node_count - 1)))
> save_node = currnode;
> @@ -626,7 +626,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> btree->nodesize, (char *) node) <= 0)
> {
> grub_free (node);
> - return grub_error (GRUB_ERR_BAD_FS, "couldn't read i-node");
> + return grub_error (GRUB_ERR_BAD_FS, N_("couldn't read i-node"));
> }
>
> nodedesc = (struct grub_hfsplus_btnode *) node;
> @@ -668,7 +668,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> + 2);
>
> if ((char *) pointer > node + btree->nodesize - 2)
> - return grub_error (GRUB_ERR_BAD_FS, "HFS+ key beyond end of node");
> + return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ key beyond end of node"));
>
> currnode = grub_be_to_cpu32 (grub_get_unaligned32 (pointer));
> match = 1;
> @@ -711,7 +711,7 @@ list_nodes (void *record, void *hook_arg)
> if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
> grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
> {
> - grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
> + grub_error (GRUB_ERR_BAD_FS, N_("catalog key length is out of range"));
> return 1;
> }
>
> --
> 2.39.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards
Vladimir 'phcoder' Serbinenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access
2023-04-20 17:59 ` [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access Lidong Chen
@ 2023-04-20 22:07 ` Vladimir 'phcoder' Serbinenko
2023-04-25 5:06 ` Lidong Chen
0 siblings, 1 reply; 13+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2023-04-20 22:07 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: daniel.kiper, lidong.chen
On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com> wrote:
>
> When an invalid node size is detected in grub_hfsplus_mount(), data pinter
> is freed. Thus, file->data is not set. The code should also set the
> grub error when that happens to indicate an error and to avoid accessing
> the unintialized file->data in grub_file_close().
file->data is set to NULL in grub_file_open as part of zalloc. What
sets it to something else?
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
> grub-core/fs/hfsplus.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
> index 9c1f12574..cf13e8a63 100644
> --- a/grub-core/fs/hfsplus.c
> +++ b/grub-core/fs/hfsplus.c
> @@ -357,7 +357,10 @@ grub_hfsplus_mount (grub_disk_t disk)
> (header.key_compare == GRUB_HFSPLUSX_BINARYCOMPARE));
>
> if (data->catalog_tree.nodesize < 2)
> - goto fail;
> + {
> + grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
> + goto fail;
> + }
>
> if (grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, 0,
> sizeof (struct grub_hfsplus_btnode),
> @@ -374,7 +377,10 @@ grub_hfsplus_mount (grub_disk_t disk)
> data->extoverflow_tree.nodesize = grub_be_to_cpu16 (header.nodesize);
>
> if (data->extoverflow_tree.nodesize < 2)
> - goto fail;
> + {
> + grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
> + goto fail;
> + }
>
> data->extoverflow_tree_ready = 1;
>
> --
> 2.39.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards
Vladimir 'phcoder' Serbinenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] fs/hfsplus: Validate btree node size
2023-04-20 17:59 ` [PATCH 1/4] fs/hfsplus: Validate btree node size Lidong Chen
@ 2023-04-20 22:08 ` Vladimir 'phcoder' Serbinenko
0 siblings, 0 replies; 13+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2023-04-20 22:08 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: daniel.kiper, lidong.chen
LGTM
On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com> wrote:
>
> The invalid btree node size can cause crashes when parsing
> the btree. The fix is to ensure the btree node size is within
> the valid range defined in the HFS Plus techical note, TN1150.
>
> https://developer.apple.com/library/archive/technotes/tn/tn1150.html
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
> grub-core/fs/hfsplus.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
> index 6337cbfcb..1ffebc8be 100644
> --- a/grub-core/fs/hfsplus.c
> +++ b/grub-core/fs/hfsplus.c
> @@ -84,6 +84,9 @@ struct grub_hfsplus_catfile
> #define GRUB_HFSPLUS_FILEMODE_DIRECTORY 0040000
> #define GRUB_HFSPLUS_FILEMODE_SYMLINK 0120000
>
> +#define HFSPLUS_BTNODE_MINSZ (1 << 9)
> +#define HFSPLUS_BTNODE_MAXSZ (1 << 15)
> +
> /* Some pre-defined file IDs. */
> enum
> {
> @@ -584,6 +587,10 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> return 0;
> }
>
> + if (btree->nodesize < HFSPLUS_BTNODE_MINSZ ||
> + btree->nodesize > HFSPLUS_BTNODE_MAXSZ)
> + return grub_error (GRUB_ERR_BAD_FS, "invalid HFS+ btree node size");
> +
> node = grub_malloc (btree->nodesize);
> if (! node)
> return grub_errno;
> --
> 2.39.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards
Vladimir 'phcoder' Serbinenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file
2023-04-20 17:59 ` [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file Lidong Chen
@ 2023-04-20 22:09 ` Vladimir 'phcoder' Serbinenko
2023-04-24 20:48 ` Lidong Chen
1 sibling, 0 replies; 13+ messages in thread
From: Vladimir 'phcoder' Serbinenko @ 2023-04-20 22:09 UTC (permalink / raw)
To: The development of GNU GRUB
On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com> wrote:
>
> A corrupted hfsplus can have a catalog key that is out of range.
> This can lead to out of bound access when advancing the pointer to
> access catalog file info.
Can you explain where 6 and 516 come from?
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
> grub-core/fs/hfsplus.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
> index 1ffebc8be..9c1f12574 100644
> --- a/grub-core/fs/hfsplus.c
> +++ b/grub-core/fs/hfsplus.c
> @@ -87,6 +87,9 @@ struct grub_hfsplus_catfile
> #define HFSPLUS_BTNODE_MINSZ (1 << 9)
> #define HFSPLUS_BTNODE_MAXSZ (1 << 15)
>
> +#define HFSPLUS_CATKEY_MIN_LEN 6
> +#define HFSPLUS_CATKEY_MAX_LEN 516
> +
> /* Some pre-defined file IDs. */
> enum
> {
> @@ -699,6 +702,13 @@ list_nodes (void *record, void *hook_arg)
>
> catkey = (struct grub_hfsplus_catkey *) record;
>
> + if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
> + grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
> + {
> + grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
> + return 1;
> + }
> +
> fileinfo =
> (struct grub_hfsplus_catfile *) ((char *) record
> + grub_be_to_cpu16 (catkey->keylen)
> --
> 2.39.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards
Vladimir 'phcoder' Serbinenko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] fs/hfsplus: Mark error strings for translation
2023-04-20 22:03 ` Vladimir 'phcoder' Serbinenko
@ 2023-04-21 13:10 ` Daniel Kiper
2023-04-21 18:14 ` Lidong Chen
0 siblings, 1 reply; 13+ messages in thread
From: Daniel Kiper @ 2023-04-21 13:10 UTC (permalink / raw)
To: Vladimir 'phcoder' Serbinenko
Cc: The development of GNU GRUB, lidong.chen
On Fri, Apr 21, 2023 at 12:03:52AM +0200, Vladimir 'phcoder' Serbinenko wrote:
> Is there any reason to translate those strings? They refer to deep
> HFS+ structures and problems and to common people they are meaningless
> even in their language. And someone in IT is likely to understand
> those concepts in English. I wouldn't know a word for "extent" in my
> native language. And those errors are very rare as they indicate a
Yeah, I know what you mean... :-)
> corrupted FS in the files GRUB cares about. OTOH translating them
> requires a lot of effort on translator's side for little benefit. They
> were not forgotten. They were not marked for translation on purpose.
> If we mark all errors for translation we will make our already large
> list of strings 10x larger and 90% of it are hyper-obscure error
> conditions. Error conditions are translated only if they are likely to
> occur and indicate a resolution direction like "linux command needs to
> be run before initrd command"
This patch has been suggested by me because I thought the lack of N_()
was a mistake. Your comments shed some light to this. Though I still
think some messages, e.g. "not a HFS+ filesystem", could be translated.
Daniel
> On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com> wrote:
> >
> > Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> > ---
> > grub-core/fs/hfsplus.c | 30 +++++++++++++++---------------
> > 1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
> > index cf13e8a63..f91af1477 100644
> > --- a/grub-core/fs/hfsplus.c
> > +++ b/grub-core/fs/hfsplus.c
> > @@ -179,7 +179,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> > if (node->fileid == GRUB_HFSPLUS_FILEID_OVERFLOW)
> > {
> > grub_error (GRUB_ERR_READ_ERROR,
> > - "extra extents found in an extend overflow file");
> > + N_("extra extents found in an extend overflow file"));
> > break;
> > }
> >
> > @@ -190,7 +190,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> > if (!node->data->extoverflow_tree_ready)
> > {
> > grub_error (GRUB_ERR_BAD_FS,
> > - "attempted to read extent overflow tree before loading");
> > + N_("attempted to read extent overflow tree before loading"));
> > break;
> > }
> >
> > @@ -205,8 +205,8 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> > || !nnode)
> > {
> > grub_error (GRUB_ERR_READ_ERROR,
> > - "no block found for the file id 0x%x and the block"
> > - " offset 0x%" PRIuGRUB_UINT64_T,
> > + N_("no block found for the file id 0x%x and the block"
> > + " offset 0x%" PRIuGRUB_UINT64_T),
> > node->fileid, fileblock);
> > break;
> > }
> > @@ -277,7 +277,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> > /* See if there's an embedded HFS+ filesystem. */
> > if (grub_be_to_cpu16 (volheader.hfs.embed_sig) != GRUB_HFSPLUS_MAGIC)
> > {
> > - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
> > + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
> > goto fail;
> > }
> >
> > @@ -303,7 +303,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> > || ((volheader.hfsplus.blksize & (volheader.hfsplus.blksize - 1)) != 0)
> > || grub_be_to_cpu32 (volheader.hfsplus.blksize) < GRUB_DISK_SECTOR_SIZE)
> > {
> > - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
> > + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
> > goto fail;
> > }
> >
> > @@ -358,7 +358,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> >
> > if (data->catalog_tree.nodesize < 2)
> > {
> > - grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
> > + grub_error (GRUB_ERR_BAD_FS, N_("invalid catalog node size"));
> > goto fail;
> > }
> >
> > @@ -378,7 +378,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> >
> > if (data->extoverflow_tree.nodesize < 2)
> > {
> > - grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
> > + grub_error (GRUB_ERR_BAD_FS, N_("invalid extents overflow node size"));
> > goto fail;
> > }
> >
> > @@ -406,7 +406,7 @@ grub_hfsplus_mount (grub_disk_t disk)
> > fail:
> >
> > if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
> > - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
> > + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
> >
> > grub_free (data);
> > return 0;
> > @@ -550,7 +550,7 @@ grub_hfsplus_btree_iterate_node (struct grub_hfsplus_btree *btree,
> >
> > if (node_count && first_node->next == saved_node)
> > {
> > - grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
> > + grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
> > return 0;
> > }
> > if (!(node_count & (node_count - 1)))
> > @@ -598,7 +598,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> >
> > if (btree->nodesize < HFSPLUS_BTNODE_MINSZ ||
> > btree->nodesize > HFSPLUS_BTNODE_MAXSZ)
> > - return grub_error (GRUB_ERR_BAD_FS, "invalid HFS+ btree node size");
> > + return grub_error (GRUB_ERR_BAD_FS, N_("invalid HFS+ btree node size"));
> >
> > node = grub_malloc (btree->nodesize);
> > if (! node)
> > @@ -613,7 +613,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> > if (save_node == currnode)
> > {
> > grub_free (node);
> > - return grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
> > + return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
> > }
> > if (!(node_count & (node_count - 1)))
> > save_node = currnode;
> > @@ -626,7 +626,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> > btree->nodesize, (char *) node) <= 0)
> > {
> > grub_free (node);
> > - return grub_error (GRUB_ERR_BAD_FS, "couldn't read i-node");
> > + return grub_error (GRUB_ERR_BAD_FS, N_("couldn't read i-node"));
> > }
> >
> > nodedesc = (struct grub_hfsplus_btnode *) node;
> > @@ -668,7 +668,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
> > + 2);
> >
> > if ((char *) pointer > node + btree->nodesize - 2)
> > - return grub_error (GRUB_ERR_BAD_FS, "HFS+ key beyond end of node");
> > + return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ key beyond end of node"));
> >
> > currnode = grub_be_to_cpu32 (grub_get_unaligned32 (pointer));
> > match = 1;
> > @@ -711,7 +711,7 @@ list_nodes (void *record, void *hook_arg)
> > if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
> > grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
> > {
> > - grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
> > + grub_error (GRUB_ERR_BAD_FS, N_("catalog key length is out of range"));
> > return 1;
> > }
> >
> > --
> > 2.39.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] fs/hfsplus: Mark error strings for translation
2023-04-21 13:10 ` Daniel Kiper
@ 2023-04-21 18:14 ` Lidong Chen
0 siblings, 0 replies; 13+ messages in thread
From: Lidong Chen @ 2023-04-21 18:14 UTC (permalink / raw)
To: Daniel Kiper
Cc: Vladimir 'phcoder' Serbinenko,
The development of GNU GRUB
> On Apr 21, 2023, at 6:10 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Fri, Apr 21, 2023 at 12:03:52AM +0200, Vladimir 'phcoder' Serbinenko wrote:
>> Is there any reason to translate those strings? They refer to deep
>> HFS+ structures and problems and to common people they are meaningless
>> even in their language. And someone in IT is likely to understand
>> those concepts in English. I wouldn't know a word for "extent" in my
>> native language. And those errors are very rare as they indicate a
>
> Yeah, I know what you mean... :-)
>
>> corrupted FS in the files GRUB cares about. OTOH translating them
>> requires a lot of effort on translator's side for little benefit. They
>> were not forgotten. They were not marked for translation on purpose.
>> If we mark all errors for translation we will make our already large
>> list of strings 10x larger and 90% of it are hyper-obscure error
>> conditions. Error conditions are translated only if they are likely to
>> occur and indicate a resolution direction like "linux command needs to
>> be run before initrd command"
>
> This patch has been suggested by me because I thought the lack of N_()
> was a mistake. Your comments shed some light to this. Though I still
> think some messages, e.g. "not a HFS+ filesystem", could be translated.
I can revert the changes, except the general messages as mentioned by Daniel.
Thanks,
Lidong
>
> Daniel
>
>> On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com> wrote:
>>>
>>> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
>>> ---
>>> grub-core/fs/hfsplus.c | 30 +++++++++++++++---------------
>>> 1 file changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
>>> index cf13e8a63..f91af1477 100644
>>> --- a/grub-core/fs/hfsplus.c
>>> +++ b/grub-core/fs/hfsplus.c
>>> @@ -179,7 +179,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>>> if (node->fileid == GRUB_HFSPLUS_FILEID_OVERFLOW)
>>> {
>>> grub_error (GRUB_ERR_READ_ERROR,
>>> - "extra extents found in an extend overflow file");
>>> + N_("extra extents found in an extend overflow file"));
>>> break;
>>> }
>>>
>>> @@ -190,7 +190,7 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>>> if (!node->data->extoverflow_tree_ready)
>>> {
>>> grub_error (GRUB_ERR_BAD_FS,
>>> - "attempted to read extent overflow tree before loading");
>>> + N_("attempted to read extent overflow tree before loading"));
>>> break;
>>> }
>>>
>>> @@ -205,8 +205,8 @@ grub_hfsplus_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>>> || !nnode)
>>> {
>>> grub_error (GRUB_ERR_READ_ERROR,
>>> - "no block found for the file id 0x%x and the block"
>>> - " offset 0x%" PRIuGRUB_UINT64_T,
>>> + N_("no block found for the file id 0x%x and the block"
>>> + " offset 0x%" PRIuGRUB_UINT64_T),
>>> node->fileid, fileblock);
>>> break;
>>> }
>>> @@ -277,7 +277,7 @@ grub_hfsplus_mount (grub_disk_t disk)
>>> /* See if there's an embedded HFS+ filesystem. */
>>> if (grub_be_to_cpu16 (volheader.hfs.embed_sig) != GRUB_HFSPLUS_MAGIC)
>>> {
>>> - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
>>> + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
>>> goto fail;
>>> }
>>>
>>> @@ -303,7 +303,7 @@ grub_hfsplus_mount (grub_disk_t disk)
>>> || ((volheader.hfsplus.blksize & (volheader.hfsplus.blksize - 1)) != 0)
>>> || grub_be_to_cpu32 (volheader.hfsplus.blksize) < GRUB_DISK_SECTOR_SIZE)
>>> {
>>> - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
>>> + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
>>> goto fail;
>>> }
>>>
>>> @@ -358,7 +358,7 @@ grub_hfsplus_mount (grub_disk_t disk)
>>>
>>> if (data->catalog_tree.nodesize < 2)
>>> {
>>> - grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
>>> + grub_error (GRUB_ERR_BAD_FS, N_("invalid catalog node size"));
>>> goto fail;
>>> }
>>>
>>> @@ -378,7 +378,7 @@ grub_hfsplus_mount (grub_disk_t disk)
>>>
>>> if (data->extoverflow_tree.nodesize < 2)
>>> {
>>> - grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
>>> + grub_error (GRUB_ERR_BAD_FS, N_("invalid extents overflow node size"));
>>> goto fail;
>>> }
>>>
>>> @@ -406,7 +406,7 @@ grub_hfsplus_mount (grub_disk_t disk)
>>> fail:
>>>
>>> if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
>>> - grub_error (GRUB_ERR_BAD_FS, "not a HFS+ filesystem");
>>> + grub_error (GRUB_ERR_BAD_FS, N_("not a HFS+ filesystem"));
>>>
>>> grub_free (data);
>>> return 0;
>>> @@ -550,7 +550,7 @@ grub_hfsplus_btree_iterate_node (struct grub_hfsplus_btree *btree,
>>>
>>> if (node_count && first_node->next == saved_node)
>>> {
>>> - grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
>>> + grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
>>> return 0;
>>> }
>>> if (!(node_count & (node_count - 1)))
>>> @@ -598,7 +598,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
>>>
>>> if (btree->nodesize < HFSPLUS_BTNODE_MINSZ ||
>>> btree->nodesize > HFSPLUS_BTNODE_MAXSZ)
>>> - return grub_error (GRUB_ERR_BAD_FS, "invalid HFS+ btree node size");
>>> + return grub_error (GRUB_ERR_BAD_FS, N_("invalid HFS+ btree node size"));
>>>
>>> node = grub_malloc (btree->nodesize);
>>> if (! node)
>>> @@ -613,7 +613,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
>>> if (save_node == currnode)
>>> {
>>> grub_free (node);
>>> - return grub_error (GRUB_ERR_BAD_FS, "HFS+ btree loop");
>>> + return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ btree loop"));
>>> }
>>> if (!(node_count & (node_count - 1)))
>>> save_node = currnode;
>>> @@ -626,7 +626,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
>>> btree->nodesize, (char *) node) <= 0)
>>> {
>>> grub_free (node);
>>> - return grub_error (GRUB_ERR_BAD_FS, "couldn't read i-node");
>>> + return grub_error (GRUB_ERR_BAD_FS, N_("couldn't read i-node"));
>>> }
>>>
>>> nodedesc = (struct grub_hfsplus_btnode *) node;
>>> @@ -668,7 +668,7 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
>>> + 2);
>>>
>>> if ((char *) pointer > node + btree->nodesize - 2)
>>> - return grub_error (GRUB_ERR_BAD_FS, "HFS+ key beyond end of node");
>>> + return grub_error (GRUB_ERR_BAD_FS, N_("HFS+ key beyond end of node"));
>>>
>>> currnode = grub_be_to_cpu32 (grub_get_unaligned32 (pointer));
>>> match = 1;
>>> @@ -711,7 +711,7 @@ list_nodes (void *record, void *hook_arg)
>>> if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
>>> grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
>>> {
>>> - grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
>>> + grub_error (GRUB_ERR_BAD_FS, N_("catalog key length is out of range"));
>>> return 1;
>>> }
>>>
>>> --
>>> 2.39.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file
2023-04-20 17:59 ` [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file Lidong Chen
2023-04-20 22:09 ` Vladimir 'phcoder' Serbinenko
@ 2023-04-24 20:48 ` Lidong Chen
1 sibling, 0 replies; 13+ messages in thread
From: Lidong Chen @ 2023-04-24 20:48 UTC (permalink / raw)
To: grub-devel@gnu.org; +Cc: Daniel Kiper, Vladimir 'phcoder' Serbinenko
[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]
BTW,
The HFSPLUS_CATKEY_* macros defined in this patch are based on the Technical Note TN1150: https://developer.apple.com/library/archive/technotes/tn/tn1150.html
"IMPORTANT:
The length of the key varies with the length of the string stored in the nodeName field; it
occupies only the number of bytes required to hold the name. The keyLength field
determines the actual length of the key; it varies between
kHFSPlusCatalogKeyMinimumLength (6) to kHFSPlusCatalogKeyMaximumLength (516).”
Regards,
Lidong
On Apr 20, 2023, at 10:59 AM, Lidong Chen <lidong.chen@oracle.com> wrote:
A corrupted hfsplus can have a catalog key that is out of range.
This can lead to out of bound access when advancing the pointer to
access catalog file info.
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/hfsplus.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 1ffebc8be..9c1f12574 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -87,6 +87,9 @@ struct grub_hfsplus_catfile
#define HFSPLUS_BTNODE_MINSZ (1 << 9)
#define HFSPLUS_BTNODE_MAXSZ (1 << 15)
+#define HFSPLUS_CATKEY_MIN_LEN 6
+#define HFSPLUS_CATKEY_MAX_LEN 516
+
/* Some pre-defined file IDs. */
enum
{
@@ -699,6 +702,13 @@ list_nodes (void *record, void *hook_arg)
catkey = (struct grub_hfsplus_catkey *) record;
+ if (grub_be_to_cpu16 (catkey->keylen) < HFSPLUS_CATKEY_MIN_LEN ||
+ grub_be_to_cpu16 (catkey->keylen) > HFSPLUS_CATKEY_MAX_LEN)
+ {
+ grub_error (GRUB_ERR_BAD_FS, "catalog key length is out of range");
+ return 1;
+ }
+
fileinfo =
(struct grub_hfsplus_catfile *) ((char *) record
+ grub_be_to_cpu16 (catkey->keylen)
--
2.39.1
[-- Attachment #2: Type: text/html, Size: 9149 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access
2023-04-20 22:07 ` Vladimir 'phcoder' Serbinenko
@ 2023-04-25 5:06 ` Lidong Chen
0 siblings, 0 replies; 13+ messages in thread
From: Lidong Chen @ 2023-04-25 5:06 UTC (permalink / raw)
To: Vladimir 'phcoder' Serbinenko
Cc: The development of GNU GRUB, Daniel Kiper
[-- Attachment #1: Type: text/plain, Size: 3181 bytes --]
On Apr 20, 2023, at 3:07 PM, Vladimir 'phcoder' Serbinenko <phcoder@gmail.com> wrote:
On Thu, Apr 20, 2023 at 8:00 PM Lidong Chen <lidong.chen@oracle.com<mailto:lidong.chen@oracle.com>> wrote:
When an invalid node size is detected in grub_hfsplus_mount(), data pinter
is freed. Thus, file->data is not set. The code should also set the
grub error when that happens to indicate an error and to avoid accessing
the unintialized file->data in grub_file_close().
file->data is set to NULL in grub_file_open as part of zalloc. What
sets it to something else?
Thanks for the review. Here is the flow that causes invalid access to file->data in grub_file_close():
static struct grub_hfsplus_data *
grub_hfsplus_mount (grub_disk_t disk)
{
data->catalog_tree.nodesize = grub_be_to_cpu16 (header.nodesize);
if (data->catalog_tree.nodesize < 2)
goto fail; // grub_errno is not set to indicate the failure here.
fail:
grub_free (data);
return 0;
}
static grub_err_t
grub_hfsplus_open (struct grub_file *file, const char *name)
{
data = grub_hfsplus_mount (file->device->disk);
if (!data)
goto fail;
fail:
grub_free (data);
return grub_errno; // return GRUB_ERR_NONE
}
grub_file_t
grub_file_open (const char *name, enum grub_file_type type)
{
if ((file->fs->fs_open) (file, file_name) != GRUB_ERR_NONE) // grub_hfsplus_open()
goto fail;
file->name = grub_strdup (name);
grub_errno = GRUB_ERR_NONE;
return file;
}
static grub_err_t
grub_hfsplus_close (grub_file_t file)
{
struct grub_hfsplus_data *data =
(struct grub_hfsplus_data *) file->data;
grub_free (data->opened_file.cbuf); // SIGSEGV here
return GRUB_ERR_NONE;
}
Regards,
Lidong
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/hfsplus.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 9c1f12574..cf13e8a63 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -357,7 +357,10 @@ grub_hfsplus_mount (grub_disk_t disk)
(header.key_compare == GRUB_HFSPLUSX_BINARYCOMPARE));
if (data->catalog_tree.nodesize < 2)
- goto fail;
+ {
+ grub_error (GRUB_ERR_BAD_FS, "invalid catalog node size");
+ goto fail;
+ }
if (grub_hfsplus_read_file (&data->extoverflow_tree.file, 0, 0,
sizeof (struct grub_hfsplus_btnode),
@@ -374,7 +377,10 @@ grub_hfsplus_mount (grub_disk_t disk)
data->extoverflow_tree.nodesize = grub_be_to_cpu16 (header.nodesize);
if (data->extoverflow_tree.nodesize < 2)
- goto fail;
+ {
+ grub_error (GRUB_ERR_BAD_FS, "invalid extents overflow node size");
+ goto fail;
+ }
data->extoverflow_tree_ready = 1;
--
2.39.1
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org<mailto:Grub-devel@gnu.org>
https://lists.gnu.org/mailman/listinfo/grub-devel
--
Regards
Vladimir 'phcoder' Serbinenko
[-- Attachment #2: Type: text/html, Size: 36818 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-04-25 5:06 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-20 17:59 [PATCH 0/4] Prevent out-of-bound reads Lidong Chen
2023-04-20 17:59 ` [PATCH 1/4] fs/hfsplus: Validate btree node size Lidong Chen
2023-04-20 22:08 ` Vladimir 'phcoder' Serbinenko
2023-04-20 17:59 ` [PATCH 2/4] fs/hfsplus: Prevent out of bound access in catalog file Lidong Chen
2023-04-20 22:09 ` Vladimir 'phcoder' Serbinenko
2023-04-24 20:48 ` Lidong Chen
2023-04-20 17:59 ` [PATCH 3/4] fs/hfsplus: Set grub errno to prevent NULL pointer access Lidong Chen
2023-04-20 22:07 ` Vladimir 'phcoder' Serbinenko
2023-04-25 5:06 ` Lidong Chen
2023-04-20 17:59 ` [PATCH 4/4] fs/hfsplus: Mark error strings for translation Lidong Chen
2023-04-20 22:03 ` Vladimir 'phcoder' Serbinenko
2023-04-21 13:10 ` Daniel Kiper
2023-04-21 18:14 ` Lidong Chen
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.