* GRUB coverity fixes for CIDs 314020 and 314023
@ 2022-06-02 15:18 Jagannathan Raman
2022-06-02 15:18 ` [PATCH 1/2] fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting Jagannathan Raman
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Jagannathan Raman @ 2022-06-02 15:18 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, darren.kenny, alec.r.brown
Hi,
This series addresses a couple of untrusted loop bounds flagged by
Coverity in "grub-core/fs/zfs". Both the bugs addressed in this series
are of the same type - caused by downcast of pointer from a strict type
to a less strict type.
Please share your thoughts on this.
Thank you!
--
Jag
Jagannathan Raman (2):
fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting
fs/zfs/zfs.c: zfs_mount() - avoid pointer downcasting
grub-core/fs/zfs/zfs.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting
2022-06-02 15:18 GRUB coverity fixes for CIDs 314020 and 314023 Jagannathan Raman
@ 2022-06-02 15:18 ` Jagannathan Raman
2022-06-02 15:18 ` [PATCH 2/2] fs/zfs/zfs.c: zfs_mount() " Jagannathan Raman
2022-06-03 13:12 ` GRUB coverity fixes for CIDs 314020 and 314023 Darren Kenny
2 siblings, 0 replies; 5+ messages in thread
From: Jagannathan Raman @ 2022-06-02 15:18 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, darren.kenny, alec.r.brown
Coverity reports that the while loop in the following function uses
tainted data as boundary:
fill_fs_info() -> dnode_get() -> zfs_log2()
The tainted originated from:
fill_fs_info() -> make_mdn()
The defect type is "Untrusted loop bound" caused as a result of
"tainted_data_downcast". Coverity does not like the pointer downcast
here and we need to address it.
We believe Coverity flags pointer downcast for the following two
reasons:
1. External data: The pointer downcast could indicate that the source is
external data, which we need to further sanitize - such as verifying its
limits. In this case, the data is read from an external source, which is
a disk. But, zio_read(), which reads the data from the disk, sanitizes it
using a checksum. checksum is the best facility that ZFS offers to verify
external data, and we don't believe a better way exists. Therefore, no
further action is possible for this.
2. Corruption due to alignment: downcasting a pointer from a strict type
to less strict type could result in data corruption. For example, the
following cast would corrupt because uint32_t is 4-byte aligned, and
won't be able to point to 0x1003 which is not 4-byte aligned.
uint8_t *ptr = 0x1003;
uint32_t *word = ptr; (incorrect, alignment issues)
This patch converts the "osp" pointer in make_mdn() from a "void" type
to "objset_phys_t" type to address this issue.
We are not sure if there are any other reasons why Coverity flags the
downcast. However, the fix for alignment issue masks/suppresses any
other issues from showing up.
Fixes: CID 314020
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
grub-core/fs/zfs/zfs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index 63b82abf3..d9c79663b 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -3156,7 +3156,7 @@ get_filesystem_dnode (dnode_end_t * mosmdn, char *fsname,
static grub_err_t
make_mdn (dnode_end_t * mdn, struct grub_zfs_data *data)
{
- void *osp;
+ objset_phys_t *osp;
blkptr_t *bp;
grub_size_t ospsize = 0;
grub_err_t err;
@@ -3164,7 +3164,7 @@ make_mdn (dnode_end_t * mdn, struct grub_zfs_data *data)
grub_dprintf ("zfs", "endian = %d\n", mdn->endian);
bp = &(((dsl_dataset_phys_t *) DN_BONUS (&mdn->dn))->ds_bp);
- err = zio_read (bp, mdn->endian, &osp, &ospsize, data);
+ err = zio_read (bp, mdn->endian, (void **) &osp, &ospsize, data);
if (err)
return err;
if (ospsize < OBJSET_PHYS_SIZE_V14)
@@ -3175,7 +3175,7 @@ make_mdn (dnode_end_t * mdn, struct grub_zfs_data *data)
mdn->endian = (grub_zfs_to_cpu64 (bp->blk_prop, mdn->endian)>>63) & 1;
grub_memmove ((char *) &(mdn->dn),
- (char *) &((objset_phys_t *) osp)->os_meta_dnode, DNODE_SIZE);
+ (char *) &(osp)->os_meta_dnode, DNODE_SIZE);
grub_free (osp);
return GRUB_ERR_NONE;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] fs/zfs/zfs.c: zfs_mount() - avoid pointer downcasting
2022-06-02 15:18 GRUB coverity fixes for CIDs 314020 and 314023 Jagannathan Raman
2022-06-02 15:18 ` [PATCH 1/2] fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting Jagannathan Raman
@ 2022-06-02 15:18 ` Jagannathan Raman
2022-06-03 13:12 ` GRUB coverity fixes for CIDs 314020 and 314023 Darren Kenny
2 siblings, 0 replies; 5+ messages in thread
From: Jagannathan Raman @ 2022-06-02 15:18 UTC (permalink / raw)
To: grub-devel; +Cc: daniel.kiper, darren.kenny, alec.r.brown
Coverity reports that while loopis in the following functions uses
tainted data as boundary:
zfs_mount() -> check_mos_features() -> dnode_get() -> zfs_log2()
zfs_mount() -> grub_memmove()
The defect type is "Untrusted loop bound" caused as a result of
"tainted_data_downcast". Coverity does not like the pointer downcast
here and we need to address it.
We believe Coverity flags pointer downcast for the following two
reasons:
1. External data: The pointer downcast could indicate that the source is
external data, which we need to further sanitize - such as verifying its
limits. In this case, the data is read from an external source, which is
a disk. But, zio_read(), which reads the data from the disk, sanitizes it
using a checksum. checksum is the best facility that ZFS offers to verify
external data, and we don't believe a better way exists. Therefore, no
further action is possible for this.
2. Corruption due to alignment: downcasting a pointer from a strict type
to less strict type could result in data corruption. For example, the
following cast would corrupt because uint32_t is 4-byte aligned, and
won't be able to point to 0x1003 which is not 4-byte aligned.
uint8_t *ptr = 0x1003;
uint32_t *word = ptr; (incorrect, alignment issues)
This patch converts the "osp" pointer in zfs_mount() from a "void" type
to "objset_phys_t" type to address this issue.
We are not sure if there are any other reasons why Coverity flags the
downcast. However, the fix for alignment issue masks/suppresses any
other issues from showing up.
Fixes: CID 314023
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
grub-core/fs/zfs/zfs.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index d9c79663b..ffa0e5863 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -3643,7 +3643,7 @@ zfs_mount (grub_device_t dev)
{
struct grub_zfs_data *data = 0;
grub_err_t err;
- void *osp = 0;
+ objset_phys_t *osp = 0;
grub_size_t ospsize;
grub_zfs_endian_t ub_endian = GRUB_ZFS_UNKNOWN_ENDIAN;
uberblock_t *ub;
@@ -3681,7 +3681,7 @@ zfs_mount (grub_device_t dev)
? GRUB_ZFS_LITTLE_ENDIAN : GRUB_ZFS_BIG_ENDIAN);
err = zio_read (&ub->ub_rootbp, ub_endian,
- &osp, &ospsize, data);
+ (void **) &osp, &ospsize, data);
if (err)
{
zfs_unmount (data);
@@ -3697,8 +3697,7 @@ zfs_mount (grub_device_t dev)
}
if (ub->ub_version >= SPA_VERSION_FEATURES &&
- check_mos_features(&((objset_phys_t *) osp)->os_meta_dnode,ub_endian,
- data) != 0)
+ check_mos_features(&osp->os_meta_dnode, ub_endian, data) != 0)
{
grub_error (GRUB_ERR_BAD_FS, "Unsupported features in pool");
grub_free (osp);
@@ -3707,8 +3706,7 @@ zfs_mount (grub_device_t dev)
}
/* Got the MOS. Save it at the memory addr MOS. */
- grub_memmove (&(data->mos.dn), &((objset_phys_t *) osp)->os_meta_dnode,
- DNODE_SIZE);
+ grub_memmove (&(data->mos.dn), &osp->os_meta_dnode, DNODE_SIZE);
data->mos.endian = (grub_zfs_to_cpu64 (ub->ub_rootbp.blk_prop,
ub_endian) >> 63) & 1;
grub_free (osp);
--
2.31.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: GRUB coverity fixes for CIDs 314020 and 314023
2022-06-02 15:18 GRUB coverity fixes for CIDs 314020 and 314023 Jagannathan Raman
2022-06-02 15:18 ` [PATCH 1/2] fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting Jagannathan Raman
2022-06-02 15:18 ` [PATCH 2/2] fs/zfs/zfs.c: zfs_mount() " Jagannathan Raman
@ 2022-06-03 13:12 ` Darren Kenny
2022-06-06 18:42 ` Daniel Kiper
2 siblings, 1 reply; 5+ messages in thread
From: Darren Kenny @ 2022-06-03 13:12 UTC (permalink / raw)
To: Jagannathan Raman, grub-devel; +Cc: daniel.kiper, alec.r.brown
On Thursday, 2022-06-02 at 15:18:25 UTC, Jagannathan Raman wrote:
> Hi,
>
> This series addresses a couple of untrusted loop bounds flagged by
> Coverity in "grub-core/fs/zfs". Both the bugs addressed in this series
> are of the same type - caused by downcast of pointer from a strict type
> to a less strict type.
>
> Please share your thoughts on this.
>
These changes look good to me, thanks for looking at them Jag.
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Thanks,
Darren.
> Thank you!
> --
> Jag
>
> Jagannathan Raman (2):
> fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting
> fs/zfs/zfs.c: zfs_mount() - avoid pointer downcasting
>
> grub-core/fs/zfs/zfs.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> --
> 2.31.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: GRUB coverity fixes for CIDs 314020 and 314023
2022-06-03 13:12 ` GRUB coverity fixes for CIDs 314020 and 314023 Darren Kenny
@ 2022-06-06 18:42 ` Daniel Kiper
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Kiper @ 2022-06-06 18:42 UTC (permalink / raw)
To: Darren Kenny; +Cc: Jagannathan Raman, grub-devel, alec.r.brown
On Fri, Jun 03, 2022 at 02:12:00PM +0100, Darren Kenny wrote:
> On Thursday, 2022-06-02 at 15:18:25 UTC, Jagannathan Raman wrote:
> > Hi,
> >
> > This series addresses a couple of untrusted loop bounds flagged by
> > Coverity in "grub-core/fs/zfs". Both the bugs addressed in this series
> > are of the same type - caused by downcast of pointer from a strict type
> > to a less strict type.
> >
> > Please share your thoughts on this.
> >
>
> These changes look good to me, thanks for looking at them Jag.
>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-06 18:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-02 15:18 GRUB coverity fixes for CIDs 314020 and 314023 Jagannathan Raman
2022-06-02 15:18 ` [PATCH 1/2] fs/zfs/zfs.c: make_mdn() - avoid pointer downcasting Jagannathan Raman
2022-06-02 15:18 ` [PATCH 2/2] fs/zfs/zfs.c: zfs_mount() " Jagannathan Raman
2022-06-03 13:12 ` GRUB coverity fixes for CIDs 314020 and 314023 Darren Kenny
2022-06-06 18:42 ` Daniel Kiper
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.