[PATCH v2 3/3] ext4: Add a few overflow checks in the writing code

[Tom Rini]

On Fri, Apr 18, 2025 at 04:59:15AM -0600, Simon Glass wrote:

> From: Simon Glass <simon.glass at 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.glass at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250418/6d142395/attachment.sig>