On Fri, Apr 18, 2025 at 04:59:15AM -0600, Simon Glass wrote: > From: Simon Glass <simon.gl...@canonical.com> > > Some memory allocations make use of data from the disk, so add some > overflow checks. > > Adjust LOG2_BLOCK_SIZE() so it is easier to read. > > Signed-off-by: Simon Glass <simon.gl...@canonical.com> > --- > > Changes in v2: > - Use Linux macros instead of gcc built-ins > > fs/ext4/ext4_write.c | 25 +++++++++++++++++++++---- > include/ext_common.h | 3 +-- > 2 files changed, 22 insertions(+), 6 deletions(-)
There's a lot going on here, which makes review harder.
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index d109ed6e90d..a9a53214dce 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -25,6 +25,7 @@
> #include <malloc.h>
> #include <memalign.h>
> #include <part.h>
> +#include <linux/overflow.h>
> #include <linux/stat.h>
> #include <div64.h>
> #include "ext4_common.h"
> @@ -108,8 +109,15 @@ int ext4fs_get_bgdtable(void)
> {
> int status;
> struct ext_filesystem *fs = get_fs();
> - int gdsize_total = ROUND(fs->no_blkgrp * fs->gdsize, fs->blksz);
> + size_t alloc_size;
> + int gdsize_total;
> +
> + if (check_mul_overflow(fs->no_blkgrp, fs->gdsize, &alloc_size))
> + return -1;
> + gdsize_total = ROUND(alloc_size, fs->blksz);
This is equivalent, yes.
> fs->no_blk_pergdt = gdsize_total / fs->blksz;
> + if (!fs->no_blk_pergdt)
> + return -1;
I don't know if that can ever happen but it's not unreasonable given
thee class of vulnerabilities that are "make up a specially corrupt
image", so this is good.
> /* allocate memory for gdtable */
> fs->gdtable = zalloc(gdsize_total);
> @@ -117,7 +125,7 @@ int ext4fs_get_bgdtable(void)
> return -ENOMEM;
> /* read the group descriptor table */
> status = ext4fs_devread((lbaint_t)fs->gdtable_blkno * fs->sect_perblk,
> - 0, fs->blksz * fs->no_blk_pergdt, fs->gdtable);
> + 0, gdsize_total, fs->gdtable);
Is that equivalent?
> if (status == 0)
> goto fail;
>
> @@ -599,10 +607,17 @@ int ext4fs_init(void)
> int i;
> uint32_t real_free_blocks = 0;
> struct ext_filesystem *fs = get_fs();
> + size_t alloc_size;
> +
> + /* check for a reasonable block size, no more than 64K */
> + if (LOG2_BLOCK_SIZE(ext4fs_root) > 16)
> + goto fail;
Reasonable for the same reason as above, but not related to overflow.
> /* populate fs */
> fs->blksz = EXT2_BLOCK_SIZE(ext4fs_root);
> fs->sect_perblk = fs->blksz >> fs->dev_desc->log2blksz;
> + if (!fs->sect_perblk)
> + goto fail;
Same.
>
> /* get the superblock */
> fs->sb = zalloc(SUPERBLOCK_SIZE);
> @@ -629,7 +644,9 @@ int ext4fs_init(void)
> }
>
> /* load all the available bitmap block of the partition */
> - fs->blk_bmaps = zalloc(fs->no_blkgrp * sizeof(char *));
> + if (check_mul_overflow(fs->no_blkgrp, sizeof(char *), &alloc_size))
> + goto fail;
> + fs->blk_bmaps = zalloc(alloc_size);
Equivalent.
> if (!fs->blk_bmaps)
> goto fail;
> for (i = 0; i < fs->no_blkgrp; i++) {
> @@ -649,7 +666,7 @@ int ext4fs_init(void)
> }
>
> /* load all the available inode bitmap of the partition */
> - fs->inode_bmaps = zalloc(fs->no_blkgrp * sizeof(unsigned char *));
> + fs->inode_bmaps = zalloc(alloc_size);
I'm going to assume that yes, really, there's no case where
sizeof(unsigned char *) != sizeof(char *) so this is equivalent.
> if (!fs->inode_bmaps)
> goto fail;
> for (i = 0; i < fs->no_blkgrp; i++) {
> diff --git a/include/ext_common.h b/include/ext_common.h
> index 6e17fbd2771..35d3a1844eb 100644
> --- a/include/ext_common.h
> +++ b/include/ext_common.h
> @@ -67,8 +67,7 @@ struct cmd_tbl;
> #define EXT2_BLOCK_SIZE(data) (1 << LOG2_BLOCK_SIZE(data))
>
> /* Log2 size of ext2 block in bytes. */
> -#define LOG2_BLOCK_SIZE(data) (le32_to_cpu \
> - (data->sblock.log2_block_size) \
> +#define LOG2_BLOCK_SIZE(data)
> (le32_to_cpu((data)->sblock.log2_block_size) \
> + EXT2_MIN_BLOCK_LOG_SIZE)
>
> #define EXT2_FT_DIR 2
That is indeed an odd place to enforce 80 character width over
readability.
--
Tom
signature.asc
Description: PGP signature
