Re: [PATCH v2 1/3] ext4: Fix integer overflow in ext4fs_read_symlink()

[Heinrich Schuchardt]

On 09.08.24 11:54, 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 <rich...@nod.at>
---
  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 52152a2295..36999b608f 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -2181,13 +2181,18 @@ static char *ext4fs_read_symlink(struct ext2fs_node 
*node)
        struct ext2fs_node *diro = node;
        int status;
        loff_t actread;
+       size_t alloc_size;

        if (!diro->inode_read) {
                status = ext4fs_read_inode(diro->data, diro->ino, &diro->inode);
                if (status == 0)
                        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;

Thank you for pointing at the problematic code.

You are calling __builtin_add_overflow(int, int, size_t *).

__builtin_add_overflow() is not defined in the C-standard.

Is there any well defined behavior for this on systems where size_t has
more bits than int?

I could imagine implementations just copying an int to the first 32 bits
of alloc_size and leaving the other bits untouched.

Hopefully your C-compiler simply refuses to compile this code.

I would prefer to stick to standard C:

alloc_size = le32_to_cpu(diro->inode.size) + 1UL;
if (!alloc_size)
        return NULL;

Here an overflow can only occur on 32bit systems.

Best regards

Heinrich

+
+       symlink = zalloc(alloc_size);
        if (!symlink)
                return NULL;