[PATCH 1/2] ext4: Fix integer overflow in ext4fs_read_symlink()
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 --- 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 2ff0dca249..32364b72fb 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2183,13 +2183,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; + + symlink = zalloc(alloc_size); if (!symlink) return NULL;
The zalloc() function suffers from two problems. 1. If memalign() fails it will return NULL and memset() will use a NULL pointer. 2. memalign() itself seems to crash when more than 2^32 bytes are requested.
So, check the return value of memalign() and allocate only of size is less than CONFIG_SYS_MALLOC_LEN.
Signed-off-by: Richard Weinberger richard@nod.at --- FWIW, I didn't investigate further why memalign() fails for large sizes. Maybe this is an issue on it's own.
Thanks, //richard --- fs/ext4/ext4_common.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h index 84500e990a..0d1f72ae01 100644 --- a/fs/ext4/ext4_common.h +++ b/fs/ext4/ext4_common.h @@ -43,8 +43,14 @@
static inline void *zalloc(size_t size) { - void *p = memalign(ARCH_DMA_MINALIGN, size); - memset(p, 0, size); + void *p = NULL; + + if (size < CONFIG_SYS_MALLOC_LEN) + p = memalign(ARCH_DMA_MINALIGN, size); + + if (p) + memset(p, 0, size); + return p; }
On Tue, Jul 02, 2024 at 09:42:23PM +0200, Richard Weinberger wrote:
The zalloc() function suffers from two problems.
- If memalign() fails it will return NULL and memset() will use a NULL pointer.
- memalign() itself seems to crash when more than 2^32 bytes are requested.
So, check the return value of memalign() and allocate only of size is less than CONFIG_SYS_MALLOC_LEN.
Signed-off-by: Richard Weinberger richard@nod.at
FWIW, I didn't investigate further why memalign() fails for large sizes. Maybe this is an issue on it's own.
Thanks, //richard
fs/ext4/ext4_common.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h index 84500e990a..0d1f72ae01 100644 --- a/fs/ext4/ext4_common.h +++ b/fs/ext4/ext4_common.h @@ -43,8 +43,14 @@
static inline void *zalloc(size_t size) {
- void *p = memalign(ARCH_DMA_MINALIGN, size);
- memset(p, 0, size);
- void *p = NULL;
- if (size < CONFIG_SYS_MALLOC_LEN)
p = memalign(ARCH_DMA_MINALIGN, size);- if (p)
memset(p, 0, size);- return p;
}
The problem here is that "zalloc" is inline and so this change causes about 1KiB of growth on platforms which enable ext4 and so at least mx6sabresd now overflows it's maximum size. Looking harder, I think the best solution here would be for ext4 to stop using its own wrapper and instead call our kzalloc compatibility function.
Tom,
Am Donnerstag, 11. Juli 2024, 17:45:17 CEST schrieb Tom Rini:
The problem here is that "zalloc" is inline and so this change causes about 1KiB of growth on platforms which enable ext4 and so at least mx6sabresd now overflows it's maximum size. Looking harder, I think the best solution here would be for ext4 to stop using its own wrapper and instead call our kzalloc compatibility function.
As discussed on IRC yesterday, moving to kzalloc() is fine. But the crash around malloc() still needs a fix.
Last night I investigated further why u-boot's malloc() implementation crashes on my x86_64 test bed when ext4 tries to allocate a lot of memory.
It turned out that it's an integer overflow around malloc_extend_top() and sbrk(). malloc_extend_top() uses a size_t to calculate the amount of required memory and sbrk() takes an ptrdiff_t type.
On x86_64, u-boot seems to use unsigned long for size_t but just an int for ptrdiff_t. This is causing the trouble.
How about this?
diff --git a/arch/x86/include/asm/posix_types.h b/arch/x86/include/asm/posix_types.h index dbcea7f47f..e1ed9bcabc 100644 --- a/arch/x86/include/asm/posix_types.h +++ b/arch/x86/include/asm/posix_types.h @@ -20,11 +20,12 @@ typedef unsigned short __kernel_gid_t; #if defined(__x86_64__) typedef unsigned long __kernel_size_t; typedef long __kernel_ssize_t; +typedef long __kernel_ptrdiff_t; #else typedef unsigned int __kernel_size_t; typedef int __kernel_ssize_t; -#endif typedef int __kernel_ptrdiff_t; +#endif typedef long __kernel_time_t; typedef long __kernel_suseconds_t; typedef long __kernel_clock_t;
Thanks, //richard
On 02.07.24 21:42, Richard Weinberger wrote:
The zalloc() function suffers from two problems.
- If memalign() fails it will return NULL and memset() will use a NULL pointer.
- memalign() itself seems to crash when more than 2^32 bytes are requested.
So, check the return value of memalign() and allocate only of size is less than CONFIG_SYS_MALLOC_LEN.
Signed-off-by: Richard Weinberger richard@nod.at
FWIW, I didn't investigate further why memalign() fails for large sizes. Maybe this is an issue on it's own.
Thanks, //richard
fs/ext4/ext4_common.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h index 84500e990a..0d1f72ae01 100644 --- a/fs/ext4/ext4_common.h +++ b/fs/ext4/ext4_common.h @@ -43,8 +43,14 @@
static inline void *zalloc(size_t size) {
- void *p = memalign(ARCH_DMA_MINALIGN, size);
- memset(p, 0, size);
- void *p = NULL;
- if (size < CONFIG_SYS_MALLOC_LEN)
p = memalign(ARCH_DMA_MINALIGN, size);
Memalign() is called in many code locations.
If memalign() has a bug, it needs to be fixed in memalign. We should not try to work around it in all callers.
Best regards
Heinrich
- if (p)
memset(p, 0, size);- return p; }
Am Freitag, 12. Juli 2024, 13:15:56 CEST schrieb 'Heinrich Schuchardt' via upstream:
Memalign() is called in many code locations.
If memalign() has a bug, it needs to be fixed in memalign. We should not try to work around it in all callers.
Agreed, I did already: See https://lists.denx.de/pipermail/u-boot/2024-July/558477.html
Thanks, //richard
On 02.07.24 21:42, 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
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 2ff0dca249..32364b72fb 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2183,13 +2183,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))
U-Boot is freestanding code. You cannot use built-ins.
Best regards
Heinrich
return NULL;- symlink = zalloc(alloc_size); if (!symlink) return NULL;
Am Freitag, 12. Juli 2024, 13:10:12 CEST schrieb 'Heinrich Schuchardt' via upstream:
On 02.07.24 21:42, 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
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 2ff0dca249..32364b72fb 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2183,13 +2183,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))
U-Boot is freestanding code. You cannot use built-ins.
Hm, I see man built-ins in the U-Boot source. Why is this one special?
Thanks, //richard
On 12.07.24 13:14, Richard Weinberger wrote:
Am Freitag, 12. Juli 2024, 13:10:12 CEST schrieb 'Heinrich Schuchardt' via upstream:
On 02.07.24 21:42, 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
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 2ff0dca249..32364b72fb 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2183,13 +2183,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))
U-Boot is freestanding code. You cannot use built-ins.
Hm, I see man built-ins in the U-Boot source. Why is this one special?
See the definition of COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW in include/linux/compiler-clang.h.
Best regards
Heinrich
Am Freitag, 12. Juli 2024, 13:19:32 CEST schrieb Heinrich Schuchardt:
Hm, I see man built-ins in the U-Boot source. Why is this one special?
See the definition of COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW in include/linux/compiler-clang.h.
So I can't use __builtin_add_overflow() because u-boot is still supporting outdated compilers that don't offer __builtin_add_overflow()?
Maybe this is the ideal moment to raise the bar? __builtin_add_overflow() is supported by gcc >= 5.1 and clang >= 8.
Thanks, //richard
On Fri, Jul 12, 2024 at 01:26:35PM +0200, Richard Weinberger wrote:
Am Freitag, 12. Juli 2024, 13:19:32 CEST schrieb Heinrich Schuchardt:
Hm, I see man built-ins in the U-Boot source. Why is this one special?
See the definition of COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW in include/linux/compiler-clang.h.
So I can't use __builtin_add_overflow() because u-boot is still supporting outdated compilers that don't offer __builtin_add_overflow()?
Maybe this is the ideal moment to raise the bar? __builtin_add_overflow() is supported by gcc >= 5.1 and clang >= 8.
We already fail on gcc older than 6, for ARM. And we don't have a minimum clang specifically, but it's probably newer than 8. So this should be fine.
participants (4)
-
Heinrich Schuchardt -
Richard Weinberger -
Richard Weinberger -
Tom Rini