Re: [PATCH 0/6] NVMeoFC support on Grub
On 2023-05-10 17:57, Daniel Kiper wrote: On Mon, May 08, 2023 at 07:28:34PM +0530, Avnish Chouhan wrote: This patch series adds support of NVMeoFC on grub. It consists of six patches. Patch 1/6 (0001-ieee1275-powerpc-implements-fibre-channel-discovery-.patch): grub-ofpathname doesn't work with fibre channel because there is no function currently implemented for it. This patch enables it by prividing a function that looks for the port name, building the entire path for OF devices. Patch 2/6(0002-ieee1275-powerpc-enables-device-mapper-discovery.patch): This patch enables the device mapper discovery on ofpath.c. Currently, when we are dealing with a device like /dev/dm-* the ofpath returns null since there is no function implemented to handle this case. This patch implements a function that will look into /sys/block/dm-* devices and search recursively inside slaves directory to find the root disk. Patch 3/6(0003-ieee1275-implement-FCP-methods-for-WWPN-and-LUNs.patch): This patch enables the fcp-targets and fcp-luns methods which are responsible to get WWPNs and LUNs for fibre channel devices. Those methods are specially necessary if the boot directory and grub installation are in different FCP disks, allowing the dev_iterate() to find the WWPNs and LUNs when called by searchfs.uuid tool. Patch 4/6(0004-change-partition-parser.patch): Usually grub will parse the PFW arguments by searching for the first occurence of the character ':'. However, we can have this char more than once on NQN. This patch changes the logic to find the last occurence of this char so we can get the proper values for NVMeoFC Patch 5/6(0005-ieee1275-add-support-for-NVMeoFC.patch): This patch implements the functions to scan and discovery of NVMeoFC. Patch 6/6(0006-ieee1275-ofpath-enable-NVMeoF-logical-device-transla.patch): This patch add code to enable the translation of logical devices to the of NVMeoFC paths. We are nearing to the code freeze and I am stopping getting new features which were not approved/reviewed by the GRUB maintainers earlier. So, please do not except any reviews from me in the following weeks. Though I will certainly will take a look at the series after the GRUB release... Daniel Hi Daniel, How are you! Did you get a chance to look NVMeoFC patch series? Thank you! Regards, Avnish Chouhan ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v2 1/1] xfs: Fix issues found while fuzzing the XFS filesystem
On Fri, Jun 02, 2023 at 06:08:44PM +, Lidong Chen wrote: > From: Darren Kenny > > While performing fuzz testing with XFS filesystem images with ASAN > enabled, several issues were found where the memory accesses are made > beyond the data that is allocated into the struct grub_xfs_data > structure's data field. > > The existing stucture didn't store the size of the memory allocated into > the buffer in the data field and had no way to check it. To resolve > these issues, the data size is stored to enable checks into the data > buffer. > > With these checks in place, the fuzzing corpus no longer cause any > crashes. > > Signed-off-by: Darren Kenny > Signed-off-by: Robbie Harwood > Signed-off-by: Marta Lewandowska > Signed-off-by: Lidong Chen Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] osdep/linux: Fix md array device enumeration
From: Kees Cook
GET_ARRAY_INFO's info.nr_disks does not map to GET_DISK_INFO's
disk.number, which is an internal kernel index. If an array has had drives
added, removed, etc, there may be gaps in GET_DISK_INFO's results. But
since the consumer of devicelist cannot tolerate gaps (it expects to walk
a NULL-terminated list of device name strings), the devicelist index (j)
must be tracked separately from the disk.number index (i). But grub
wants to only examine active (i.e. present and non-failed) disks, so how
many disks are left to be queried must be also separately tracked
(remaining).
Fixes: 49de079bbe1c ("... (grub_util_raid_getmembers): Handle "removed" disks")
Fixes: 2b00217369ac ("... Added support for RAID and LVM")
Fixes: https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1912043
Fixes: https://savannah.gnu.org/bugs/index.php?59887
Signed-off-by: Kees Cook
---
grub-core/osdep/linux/getroot.c | 27 +++
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/grub-core/osdep/linux/getroot.c b/grub-core/osdep/linux/getroot.c
index 001b818fe..d7b62e702 100644
--- a/grub-core/osdep/linux/getroot.c
+++ b/grub-core/osdep/linux/getroot.c
@@ -135,10 +135,18 @@ struct mountinfo_entry
char fstype[ESCAPED_PATH_MAX + 1], device[ESCAPED_PATH_MAX + 1];
};
+/* GET_DISK_INFO nr_disks (total count) does not map to disk.number,
+ which is an internal kernel index. Instead, do what mdadm does
+ and keep scanning until we find enough valid disks. The limit is
+ copied from there, which notes that it is sufficiently high given
+ that the on-disk metadata for v1.x can only support 1920. */
+#define MD_MAX_DISKS 4096
+
static char **
grub_util_raid_getmembers (const char *name, int bootable)
{
int fd, ret, i, j;
+ int remaining;
char **devicelist;
mdu_version_t version;
mdu_array_info_t info;
@@ -170,22 +178,25 @@ grub_util_raid_getmembers (const char *name, int bootable)
devicelist = xcalloc (info.nr_disks + 1, sizeof (char *));
- for (i = 0, j = 0; j < info.nr_disks; i++)
+ remaining = info.nr_disks;
+ for (i = 0, j = 0; i < MD_MAX_DISKS && remaining > 0; i++)
{
disk.number = i;
ret = ioctl (fd, GET_DISK_INFO, &disk);
if (ret != 0)
grub_util_error (_("ioctl GET_DISK_INFO error: %s"), strerror (errno));
-
+
+ /* Skip empty slot: MD_DISK_REMOVED slots don't count toward nr_disks. */
if (disk.state & (1 << MD_DISK_REMOVED))
continue;
+ remaining--;
- if (disk.state & (1 << MD_DISK_ACTIVE))
- devicelist[j] = grub_find_device (NULL,
- makedev (disk.major, disk.minor));
- else
- devicelist[j] = NULL;
- j++;
+ /* Only examine disks that are actively participating in the array. */
+ if (!(disk.state & (1 << MD_DISK_ACTIVE)))
+ continue;
+
+ devicelist[j++] = grub_find_device (NULL, makedev (disk.major,
+disk.minor));
}
devicelist[j] = NULL;
--
2.39.2
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] osdep/linux: Fix md array device enumeration
On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> From: Kees Cook
>
> GET_ARRAY_INFO's info.nr_disks does not map to GET_DISK_INFO's
> disk.number, which is an internal kernel index. If an array has had drives
> added, removed, etc, there may be gaps in GET_DISK_INFO's results. But
> since the consumer of devicelist cannot tolerate gaps (it expects to walk
> a NULL-terminated list of device name strings), the devicelist index (j)
> must be tracked separately from the disk.number index (i). But grub
> wants to only examine active (i.e. present and non-failed) disks, so how
> many disks are left to be queried must be also separately tracked
> (remaining).
>
> Fixes: 49de079bbe1c ("... (grub_util_raid_getmembers): Handle "removed"
> disks")
> Fixes: 2b00217369ac ("... Added support for RAID and LVM")
> Fixes: https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1912043
> Fixes: https://savannah.gnu.org/bugs/index.php?59887
> Signed-off-by: Kees Cook
I did not add a cover letter, I just found this patch in a merge request
on Debian's GitLab, and it still applies, but I haven't tested it
further than Kees did, and don't know what the test case is quite
honestly.
--
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer i speak de, en
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v1 1/1] loongarch: add relaxation support
On Tue, Jun 06, 2023 at 10:34:06AM +0800, Xiaotian Wu wrote:
> Add R_LARCH_B16, R_LARCH_B21 and R_LARCH_RELAX relocation types to support
> LoongArch relaxation.
Please explain why this relaxation support is needed. I think you can
copy explanation from the cover letter.
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=56576f4a722b7398d35802ecf7d4185c27d6d69b
> https://github.com/loongson/la-abi-specs/blob/release/laelf.adoc#relocations
>
> Signed-off-by: Xiaotian Wu
> ---
> grub-core/kern/loongarch64/dl.c| 21 +++-
> grub-core/kern/loongarch64/dl_helper.c | 72 --
> include/grub/elf.h | 3 ++
> include/grub/loongarch64/reloc.h | 6 ++-
> util/grub-mkimagexx.c | 28 --
> util/grub-module-verifier.c| 3 ++
> 6 files changed, 122 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/kern/loongarch64/dl.c b/grub-core/kern/loongarch64/dl.c
> index 43080e72e..c22d2bd52 100644
> --- a/grub-core/kern/loongarch64/dl.c
> +++ b/grub-core/kern/loongarch64/dl.c
> @@ -87,11 +87,28 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void *ehdr,
> }
> break;
> case R_LARCH_MARK_LA:
> + case R_LARCH_RELAX:
> break;
> case R_LARCH_SOP_PUSH_PCREL:
> case R_LARCH_SOP_PUSH_PLT_PCREL:
> grub_loongarch64_sop_push (&stack, sym_addr - (grub_uint64_t)place);
> break;
> + case R_LARCH_B16:
> + {
> + grub_uint32_t *abs_place = place;
> + grub_ssize_t off = sym_addr - (grub_addr_t) place;
> +
> + grub_loongarch64_b16 (abs_place, off);
> + }
> + break;
> + case R_LARCH_B21:
> + {
> + grub_uint32_t *abs_place = place;
> + grub_ssize_t off = sym_addr - (grub_addr_t) place;
> +
> + grub_loongarch64_b21 (abs_place, off);
> + }
> + break;
> case R_LARCH_B26:
> {
> grub_uint32_t *abs_place = place;
> @@ -109,13 +126,13 @@ grub_arch_dl_relocate_symbols (grub_dl_t mod, void
> *ehdr,
> case R_LARCH_ABS64_LO20:
> {
> grub_uint32_t *abs_place = place;
> - grub_loongarch64_xxx64_lo20 (abs_place, sym_addr);
> + grub_loongarch64_abs64_lo20 (abs_place, sym_addr);
This change...
> }
> break;
> case R_LARCH_ABS64_HI12:
> {
> grub_uint32_t *abs_place = place;
> - grub_loongarch64_xxx64_hi12 (abs_place, sym_addr);
> + grub_loongarch64_abs64_hi12 (abs_place, sym_addr);
... and this one does not seem to belong to this patch. Please move
these changes to separate patch.
> }
> break;
> case R_LARCH_PCALA_HI20:
> diff --git a/grub-core/kern/loongarch64/dl_helper.c
> b/grub-core/kern/loongarch64/dl_helper.c
> index cda1a53c8..2809eeb7f 100644
> --- a/grub-core/kern/loongarch64/dl_helper.c
> +++ b/grub-core/kern/loongarch64/dl_helper.c
> @@ -24,6 +24,10 @@
> #include
> #include
>
> +/*
> + * LoongArch relocations documentation:
> + *
> https://github.com/loongson/la-abi-specs/blob/release/laelf.adoc#relocations
> + */
Probably not a part of this patch too...
> static void grub_loongarch64_stack_push (grub_loongarch64_stack_t stack,
> grub_uint64_t x);
> static grub_uint64_t grub_loongarch64_stack_pop (grub_loongarch64_stack_t
> stack);
>
> @@ -200,14 +204,58 @@ grub_loongarch64_sop_32_s_0_10_10_16_s2
> (grub_loongarch64_stack_t stack,
>*place =(*place) | ((a >> 18) & 0x3ff);
> }
>
> +/*
> + * B16 relocation for the 18-bit PC-relative jump
> + * (*(uint32_t *) PC) [25 ... 10] = (S+A-PC) [17 ... 2]
> + */
> +void grub_loongarch64_b16 (grub_uint32_t *place, grub_int64_t offset)
> +{
> + grub_uint32_t val;
> + const grub_uint32_t insmask = grub_cpu_to_le32_compile_time (0xfc0003ff);
> +
> + grub_dprintf ("dl", " reloc_b16 %p %c= 0x%"PRIxGRUB_INT64_T"\n",
s/"PRIxGRUB_INT64_T"/" PRIxGRUB_INT64_T "/
Simply add spaces before and after PRIxGRUB_INT64_T...
> + place, offset > 0 ? '+' : '-',
> + offset < 0 ? (grub_int64_t) -(grub_uint64_t) offset : offset);
These two casts does not make a lot of sense for me. You remove minus
sign from the offset. So, I do not expect an overflow here. Then you
can safely drop casts. Right?
> + val = ((offset >> 2) & 0x) << 10;
> +
> + *place &= insmask;
> + *place |= grub_cpu_to_le32 (val) & ~insmask;
> +}
> +
> +/*
> + * B21 relocation for the 23-bit PC-relative jump
> + * (*(uint32_t *) PC) [4 ... 0] = (S+A-PC) [22 ... 18]
> + * (*(uint32_t *) PC) [25 ... 10] = (S+A-PC) [17 ... 2]
> + */
> +void grub_loongarch64_b21 (grub_uint32_t *place, grub_int64_t offset)
> +{
> + grub_uint32_t val;
> + const grub_uint32_t insmask = grub_cpu_to_le32_compile_time (0xfc0003e0);
> +
> + grub_dprintf ("dl", " reloc_b21 %p %c= 0x%"PRIxGRUB_INT64_T"\n",
> + place, offset > 0 ? '+' : '-',
> + offset < 0 ? (grub_int64_t) -(grub_uint64_t) offset : offset);
Dit
Re: [PATCH] osdep/linux: Fix md array device enumeration
On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > From: Kees Cook
> >
> > GET_ARRAY_INFO's info.nr_disks does not map to GET_DISK_INFO's
> > disk.number, which is an internal kernel index. If an array has had drives
> > added, removed, etc, there may be gaps in GET_DISK_INFO's results. But
> > since the consumer of devicelist cannot tolerate gaps (it expects to walk
> > a NULL-terminated list of device name strings), the devicelist index (j)
> > must be tracked separately from the disk.number index (i). But grub
> > wants to only examine active (i.e. present and non-failed) disks, so how
> > many disks are left to be queried must be also separately tracked
> > (remaining).
> >
> > Fixes: 49de079bbe1c ("... (grub_util_raid_getmembers): Handle "removed"
> > disks")
> > Fixes: 2b00217369ac ("... Added support for RAID and LVM")
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1912043
> > Fixes: https://savannah.gnu.org/bugs/index.php?59887
> > Signed-off-by: Kees Cook
>
> I did not add a cover letter, I just found this patch in a merge request
> on Debian's GitLab, and it still applies, but I haven't tested it
> further than Kees did, and don't know what the test case is quite
> honestly.
This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
device enumeration).
I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
(osdep/linux: Fix md array device enumeration) is not in sync with
commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
than 1024 disks). I think we should sync both numbers down to 1024...
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] osdep/linux: Fix md array device enumeration
On Tue, Jun 06, 2023 at 07:09:26PM +0200, Daniel Kiper wrote:
> On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote:
> > On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote:
> > > From: Kees Cook
> > >
> > > GET_ARRAY_INFO's info.nr_disks does not map to GET_DISK_INFO's
> > > disk.number, which is an internal kernel index. If an array has had drives
> > > added, removed, etc, there may be gaps in GET_DISK_INFO's results. But
> > > since the consumer of devicelist cannot tolerate gaps (it expects to walk
> > > a NULL-terminated list of device name strings), the devicelist index (j)
> > > must be tracked separately from the disk.number index (i). But grub
> > > wants to only examine active (i.e. present and non-failed) disks, so how
> > > many disks are left to be queried must be also separately tracked
> > > (remaining).
> > >
> > > Fixes: 49de079bbe1c ("... (grub_util_raid_getmembers): Handle "removed"
> > > disks")
> > > Fixes: 2b00217369ac ("... Added support for RAID and LVM")
> > > Fixes: https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1912043
> > > Fixes: https://savannah.gnu.org/bugs/index.php?59887
> > > Signed-off-by: Kees Cook
> >
> > I did not add a cover letter, I just found this patch in a merge request
> > on Debian's GitLab, and it still applies, but I haven't tested it
> > further than Kees did, and don't know what the test case is quite
> > honestly.
>
> This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array
> device enumeration).
Huh sorry,
I must have been looking at a wrong branch and applied to a wrong
branch before forwarding.
>
> I realized right now that MD_MAX_DISKS defined in commit c39f27cd6
> (osdep/linux: Fix md array device enumeration) is not in sync with
> commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more
> than 1024 disks). I think we should sync both numbers down to 1024...
+1
--
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer i speak de, en
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] osdep/linux: Fix md array device enumeration
On Tue, Jun 6, 2023 at 10:27 AM Julian Andres Klode wrote: > > On Tue, Jun 06, 2023 at 07:09:26PM +0200, Daniel Kiper wrote: > > On Tue, Jun 06, 2023 at 06:15:27PM +0200, Julian Andres Klode wrote: > > > On Tue, Jun 06, 2023 at 06:10:21PM +0200, Julian Andres Klode wrote: > > [...] > > This patch is in upstream as commit c39f27cd6 (osdep/linux: Fix md array > > device enumeration). Oh good. I really thought it had landed already, so thanks for checking. I got worried this morning when I saw the email to grub-devel. :P "Wasn't that fixed already?" :) But thank you for making sure it hadn't gotten lost! Is there a way to close the tracker item for it? > [...] > > I realized right now that MD_MAX_DISKS defined in commit c39f27cd6 > > (osdep/linux: Fix md array device enumeration) is not in sync with > > commit 2a5e3c1f2 (disk/diskfilter: Don't make a RAID array with more > > than 1024 disks). I think we should sync both numbers down to 1024... > > +1 Yeah, seems reasonable, though as I hinted in the original patch, this number appeared to have been arbitrarily chosen by mdadm at the time. -Kees -- Kees Cook ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/1] fs/udf: Fix out of bounds access
On Jun 2, 2023, at 2:43 AM, Darren Kenny 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
---
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
https://lists.gnu.org/mailman/listinfo/grub-devel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 0/1] fs/udf: Fix out of bounds access
Thanks Daniel and Darren for reviewing the changes. This v2 addressed Darren's comments. Lidong Chen (1): fs/udf: Fix out of bounds access grub-core/fs/udf.c | 38 ++ 1 file changed, 38 insertions(+) -- 2.39.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 1/1] fs/udf: Fix out of bounds access
Implemented a boundary check before advancing the allocation
descriptors pointer.
Signed-off-by: Lidong Chen
Reviewed-by: Darren Kenny
Reviewed-by: Daniel Kiper
---
grub-core/fs/udf.c | 38 ++
1 file changed, 38 insertions(+)
diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c
index 7679ea309..58884d2ba 100644
--- a/grub-core/fs/udf.c
+++ b/grub-core/fs/udf.c
@@ -114,6 +114,10 @@ GRUB_MOD_LICENSE ("GPLv3+");
#define GRUB_UDF_PARTMAP_TYPE_11
#define GRUB_UDF_PARTMAP_TYPE_22
+#define GRUB_UDF_INVALID_STRUCT_PTR(_ptr, _struct) \
+ ((char *) (_ptr) >= end_ptr || \
+ ((grub_ssize_t)(end_ptr - (char*)(_ptr)) < (grub_ssize_t)sizeof(_struct)))
+
struct grub_udf_lb_addr
{
grub_uint32_t block_num;
@@ -458,6 +462,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 +481,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 (GRUB_UDF_INVALID_STRUCT_PTR(ptr, 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);
@@ -542,10 +555,22 @@ 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 (GRUB_UDF_INVALID_STRUCT_PTR(ad, struct grub_udf_short_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
}
}
else
{
+ if (GRUB_UDF_INVALID_STRUCT_PTR(ptr, 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);
@@ -611,6 +636,12 @@ 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 (GRUB_UDF_INVALID_STRUCT_PTR(ad, struct grub_udf_long_ad))
+ {
+ grub_error (GRUB_ERR_BAD_FS, "corrupted UDF file system");
+ return 0;
+ }
}
}
@@ -630,6 +661,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]
@@ -637,6 +669,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 + 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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
