[PATCH] ext4: Fix integer overflow in ext4fs_read_symlink()

[PATCH] ext4: Fix integer overflow in ext4fs_read_symlink()

[Richard Weinberger]

While zalloc() takes a size_t type, adding 1 to the le32 variable
will overflow.
A carefully crafted ext4 filesystem can exhibit an inode size of 0xffffffff
and as consequence zalloc() will do a zero allocation.

Later in the function the inode size is again used for copying data.
So an attacker can overwrite memory.

Avoid the overflow by using the __builtin_add_overflow() helper.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
I have found and verified this bug in u-boot.
But Barebox uses the same code, so it is most likely affected too.

Thanks,
//richard
---
 fs/ext4/ext4_common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 4bfb55ad0d..a38593105f 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -369,13 +369,18 @@ char *ext4fs_read_symlink(struct ext2fs_node *node)
 	char *symlink;
 	struct ext2fs_node *diro = node;
 	int status, ret;
+	size_t alloc_size;
 
 	if (!diro->inode_read) {
 		ret = ext4fs_read_inode(diro->data, diro->ino, &diro->inode);
 		if (ret)
 			return NULL;
 	}
-	symlink = zalloc(le32_to_cpu(diro->inode.size) + 1);
+
+	if (__builtin_add_overflow(le32_to_cpu(diro->inode.size), 1, &alloc_size))
+		return NULL;
+
+	symlink = zalloc(alloc_size);
 	if (!symlink)
 		return 0;
 
-- 
2.35.3

Re: [PATCH] ext4: Fix integer overflow in ext4fs_read_symlink()

[Ahmad Fatoum]

On 02.07.24 21:44, Richard Weinberger wrote:
> While zalloc() takes a size_t type, adding 1 to the le32 variable
> will overflow.
> A carefully crafted ext4 filesystem can exhibit an inode size of 0xffffffff
> and as consequence zalloc() will do a zero allocation.
> 
> Later in the function the inode size is again used for copying data.
> So an attacker can overwrite memory.
> 
> Avoid the overflow by using the __builtin_add_overflow() helper.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
> I have found and verified this bug in u-boot.
> But Barebox uses the same code, so it is most likely affected too.
> 
> Thanks,
> //richard
> ---
>  fs/ext4/ext4_common.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 4bfb55ad0d..a38593105f 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -369,13 +369,18 @@ char *ext4fs_read_symlink(struct ext2fs_node *node)
>  	char *symlink;
>  	struct ext2fs_node *diro = node;
>  	int status, ret;
> +	size_t alloc_size;
>  
>  	if (!diro->inode_read) {
>  		ret = ext4fs_read_inode(diro->data, diro->ino, &diro->inode);
>  		if (ret)
>  			return NULL;
>  	}
> -	symlink = zalloc(le32_to_cpu(diro->inode.size) + 1);
> +
> +	if (__builtin_add_overflow(le32_to_cpu(diro->inode.size), 1, &alloc_size))
> +		return NULL;
> +
> +	symlink = zalloc(alloc_size);
>  	if (!symlink)
>  		return 0;
>  

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] ext4: Fix integer overflow in ext4fs_read_symlink()

[Sascha Hauer]

On Tue, 02 Jul 2024 21:44:27 +0200, Richard Weinberger wrote:
> While zalloc() takes a size_t type, adding 1 to the le32 variable
> will overflow.
> A carefully crafted ext4 filesystem can exhibit an inode size of 0xffffffff
> and as consequence zalloc() will do a zero allocation.
> 
> Later in the function the inode size is again used for copying data.
> So an attacker can overwrite memory.
> 
> [...]

Applied, thanks!

[1/1] ext4: Fix integer overflow in ext4fs_read_symlink()
      https://git.pengutronix.de/cgit/barebox/commit/?id=a2b76550f7d8 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>