* [PATCH 1/1] fs/udf: Fix out of bounds access
@ 2023-06-01 18:50 Lidong Chen
2023-06-02 9:43 ` Darren Kenny
0 siblings, 1 reply; 3+ messages in thread
From: Lidong Chen @ 2023-06-01 18:50 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, lidong.chen
Implemented a boundary check before advancing the allocation
descriptors pointer.
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/udf.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
index 12e88ab62..2359222eb 100644
--- a/grub-core/fs/udf.c
+++ b/grub-core/fs/udf.c
@@ -458,6 +458,7 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
char *ptr;
grub_ssize_t len;
grub_disk_addr_t filebytes;
+ char *end_ptr;
switch (U16 (node->block.fe.tag.tag_ident))
{
@@ -476,9 +477,17 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
return 0;
}
+ end_ptr = (char *) node + get_fshelp_size (node->data);
+
if ((U16 (node->block.fe.icbtag.flags) & GRUB_UDF_ICBTAG_FLAG_AD_MASK)
== GRUB_UDF_ICBTAG_FLAG_AD_SHORT)
{
+ if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
+
struct grub_udf_short_ad *ad = (struct grub_udf_short_ad *) ptr;
filebytes = fileblock * U32 (node->data->lvd.bsize);
@@ -528,10 +537,23 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
filebytes -= adlen;
ad++;
len -= sizeof (struct grub_udf_short_ad);
+
+ if ((char *) ad >= end_ptr ||
+ (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
}
}
else
{
+ if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
+
struct grub_udf_long_ad *ad = (struct grub_udf_long_ad *) ptr;
filebytes = fileblock * U32 (node->data->lvd.bsize);
@@ -583,6 +605,13 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
filebytes -= adlen;
ad++;
len -= sizeof (struct grub_udf_long_ad);
+
+ if ((char *) ad >= end_ptr ||
+ (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
}
}
@@ -602,6 +631,7 @@ grub_udf_read_file (grub_fshelp_node_t node,
case GRUB_UDF_ICBTAG_FLAG_AD_IN_ICB:
{
char *ptr;
+ char *end_ptr = (char *) node + get_fshelp_size (node->data);
ptr = ((U16 (node->block.fe.tag.tag_ident) == GRUB_UDF_TAG_IDENT_FE) ?
((char *) &node->block.fe.ext_attr[0]
@@ -609,6 +639,12 @@ grub_udf_read_file (grub_fshelp_node_t node,
((char *) &node->block.efe.ext_attr[0]
+ U32 (node->block.efe.ext_attr_length)));
+ if (ptr > end_ptr || (ptr + pos) > end_ptr || (ptr + pos + len) > end_ptr)
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
+
grub_memcpy (buf, ptr + pos, len);
return len;
--
2.39.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] fs/udf: Fix out of bounds access
2023-06-01 18:50 [PATCH 1/1] fs/udf: Fix out of bounds access Lidong Chen
@ 2023-06-02 9:43 ` Darren Kenny
2023-06-07 1:09 ` Lidong Chen
0 siblings, 1 reply; 3+ messages in thread
From: Darren Kenny @ 2023-06-02 9:43 UTC (permalink / raw)
To: Lidong Chen, grub-devel; +Cc: daniel.kiper, lidong.chen
Hi Li,
In general looks good...
On Thursday, 2023-06-01 at 18:50:19 UTC, Lidong Chen wrote:
> Implemented a boundary check before advancing the allocation
> descriptors pointer.
>
> Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
> ---
> grub-core/fs/udf.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
> index 12e88ab62..2359222eb 100644
> --- a/grub-core/fs/udf.c
> +++ b/grub-core/fs/udf.c
> @@ -458,6 +458,7 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> char *ptr;
> grub_ssize_t len;
> grub_disk_addr_t filebytes;
> + char *end_ptr;
>
> switch (U16 (node->block.fe.tag.tag_ident))
> {
> @@ -476,9 +477,17 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> return 0;
> }
>
> + end_ptr = (char *) node + get_fshelp_size (node->data);
> +
> if ((U16 (node->block.fe.icbtag.flags) & GRUB_UDF_ICBTAG_FLAG_AD_MASK)
> == GRUB_UDF_ICBTAG_FLAG_AD_SHORT)
> {
> + if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
>
Should this probably also be testing ptr < end_ptr?
I wonder if a local macro like this would be useful:
#define GRUB_UDF_INVALID_STRUCT_PTR(_ptr, _struct) \
((char *) (_ptr) >= end_ptr ||
((grub_ssize_t)(end_ptr - (char*)(_ptr)) < (grub_ssize_t)sizeof(_struct))
or the more positive and succinct version, and subsequent negated (!) test:
#define GRUB_UDF_VALID_STRUCT_PTR(_ptr, _struct) \
((char *)(_ptr) <= (end_ptr - sizeof(_struct)))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
> struct grub_udf_short_ad *ad = (struct grub_udf_short_ad *) ptr;
>
> filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -528,10 +537,23 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> filebytes -= adlen;
> ad++;
> len -= sizeof (struct grub_udf_short_ad);
> +
> + if ((char *) ad >= end_ptr ||
> + (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> }
> }
> else
> {
> + if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
> struct grub_udf_long_ad *ad = (struct grub_udf_long_ad *) ptr;
>
> filebytes = fileblock * U32 (node->data->lvd.bsize);
> @@ -583,6 +605,13 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
> filebytes -= adlen;
> ad++;
> len -= sizeof (struct grub_udf_long_ad);
> +
> + if ((char *) ad >= end_ptr ||
> + (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> }
> }
>
> @@ -602,6 +631,7 @@ grub_udf_read_file (grub_fshelp_node_t node,
> case GRUB_UDF_ICBTAG_FLAG_AD_IN_ICB:
> {
> char *ptr;
> + char *end_ptr = (char *) node + get_fshelp_size (node->data);
>
> ptr = ((U16 (node->block.fe.tag.tag_ident) == GRUB_UDF_TAG_IDENT_FE) ?
> ((char *) &node->block.fe.ext_attr[0]
> @@ -609,6 +639,12 @@ grub_udf_read_file (grub_fshelp_node_t node,
> ((char *) &node->block.efe.ext_attr[0]
> + U32 (node->block.efe.ext_attr_length)));
>
> + if (ptr > end_ptr || (ptr + pos) > end_ptr || (ptr + pos + len) > end_ptr)
>
Not sure there is a need for all of these, would the last one not
suffice? Might be worth testing that pos and len are > 0 if only using
that one.
Thanks,
Darren.
> + {
> + grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
> + return 0;
> + }
> +
> grub_memcpy (buf, ptr + pos, len);
>
> return len;
> --
> 2.39.1
>
>
> _______________________________________________
> 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
* Re: [PATCH 1/1] fs/udf: Fix out of bounds access
2023-06-02 9:43 ` Darren Kenny
@ 2023-06-07 1:09 ` Lidong Chen
0 siblings, 0 replies; 3+ messages in thread
From: Lidong Chen @ 2023-06-07 1:09 UTC (permalink / raw)
To: Darren Kenny; +Cc: The development of GNU GRUB, Daniel Kiper
[-- Attachment #1: Type: text/plain, Size: 4318 bytes --]
On Jun 2, 2023, at 2:43 AM, Darren Kenny <darren.kenny@oracle.com> wrote:
Hi Li,
In general looks good...
On Thursday, 2023-06-01 at 18:50:19 UTC, Lidong Chen wrote:
Implemented a boundary check before advancing the allocation
descriptors pointer.
Signed-off-by: Lidong Chen <lidong.chen@oracle.com>
---
grub-core/fs/udf.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
index 12e88ab62..2359222eb 100644
--- a/grub-core/fs/udf.c
+++ b/grub-core/fs/udf.c
@@ -458,6 +458,7 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
char *ptr;
grub_ssize_t len;
grub_disk_addr_t filebytes;
+ char *end_ptr;
switch (U16 (node->block.fe.tag.tag_ident))
{
@@ -476,9 +477,17 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
return 0;
}
+ end_ptr = (char *) node + get_fshelp_size (node->data);
+
if ((U16 (node->block.fe.icbtag.flags) & GRUB_UDF_ICBTAG_FLAG_AD_MASK)
== GRUB_UDF_ICBTAG_FLAG_AD_SHORT)
{
+ if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
Should this probably also be testing ptr < end_ptr?
I wonder if a local macro like this would be useful:
#define GRUB_UDF_INVALID_STRUCT_PTR(_ptr, _struct) \
((char *) (_ptr) >= end_ptr ||
((grub_ssize_t)(end_ptr - (char*)(_ptr)) < (grub_ssize_t)sizeof(_struct))
Hi Darren,
Thank you for the review! I updated the patch with the suggestion.
or the more positive and succinct version, and subsequent negated (!) test:
#define GRUB_UDF_VALID_STRUCT_PTR(_ptr, _struct) \
((char *)(_ptr) <= (end_ptr - sizeof(_struct)))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
+
struct grub_udf_short_ad *ad = (struct grub_udf_short_ad *) ptr;
filebytes = fileblock * U32 (node->data->lvd.bsize);
@@ -528,10 +537,23 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
filebytes -= adlen;
ad++;
len -= sizeof (struct grub_udf_short_ad);
+
+ if ((char *) ad >= end_ptr ||
+ (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_short_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
}
}
else
{
+ if ((end_ptr - ptr) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
+
struct grub_udf_long_ad *ad = (struct grub_udf_long_ad *) ptr;
filebytes = fileblock * U32 (node->data->lvd.bsize);
@@ -583,6 +605,13 @@ grub_udf_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
filebytes -= adlen;
ad++;
len -= sizeof (struct grub_udf_long_ad);
+
+ if ((char *) ad >= end_ptr ||
+ (end_ptr - (char *) ad) < (grub_ssize_t) sizeof (struct grub_udf_long_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
}
}
@@ -602,6 +631,7 @@ grub_udf_read_file (grub_fshelp_node_t node,
case GRUB_UDF_ICBTAG_FLAG_AD_IN_ICB:
{
char *ptr;
+ char *end_ptr = (char *) node + get_fshelp_size (node->data);
ptr = ((U16 (node->block.fe.tag.tag_ident) == GRUB_UDF_TAG_IDENT_FE) ?
((char *) &node->block.fe.ext_attr[0]
@@ -609,6 +639,12 @@ grub_udf_read_file (grub_fshelp_node_t node,
((char *) &node->block.efe.ext_attr[0]
+ U32 (node->block.efe.ext_attr_length)));
+ if (ptr > end_ptr || (ptr + pos) > end_ptr || (ptr + pos + len) > end_ptr)
Not sure there is a need for all of these, would the last one not
suffice? Might be worth testing that pos and len are > 0 if only using
that one.
Both pos and len are unsigned, so, I updated the patch with only the last condition check.
Thanks again,
Lidong
Thanks,
Darren.
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
+
grub_memcpy (buf, ptr + pos, len);
return len;
--
2.39.1
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org<mailto:Grub-devel@gnu.org>
https://lists.gnu.org/mailman/listinfo/grub-devel
[-- Attachment #2: Type: text/html, Size: 28075 bytes --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-07 1:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-01 18:50 [PATCH 1/1] fs/udf: Fix out of bounds access Lidong Chen
2023-06-02 9:43 ` Darren Kenny
2023-06-07 1:09 ` 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.