* [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.