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

[PATCH v2 1/3] 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>
---
 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;
+
+	symlink = zalloc(alloc_size);
 	if (!symlink)
 		return NULL;
 
-- 
2.35.3

[PATCH v2 2/3] compiler: Ensure __builtin_*_overflow() support

[Richard Weinberger]

Both gcc and clang support this for a long time.
Make sure the feature is present.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 include/linux/compiler_types.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 1a3060117f..8b6ce9c11c 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -70,6 +70,13 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 #error "Unknown compiler"
 #endif
 
+/*
+ * At least gcc 5.1 or clang 8 are needed.
+ */
+#ifndef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
+#error Unsupported compiler
+#endif
+
 /*
  * Some architectures need to provide custom definitions of macros provided
  * by linux/compiler-*.h, and can do so using asm/compiler.h. We include that
-- 
2.35.3

[PATCH v2 3/3] ext4: Fix zalloc()

[Richard Weinberger]

Currently, zalloc() calls uncondtionally memset(),
if the allocation failes, memset() will write to a null pointer.

Fix by using kzalloc().

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ext4/ext4_common.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
index 84500e990a..346752092b 100644
--- a/fs/ext4/ext4_common.h
+++ b/fs/ext4/ext4_common.h
@@ -24,6 +24,7 @@
 #include <ext4fs.h>
 #include <malloc.h>
 #include <asm/cache.h>
+#include <linux/compat.h>
 #include <linux/errno.h>
 #if defined(CONFIG_EXT4_WRITE)
 #include "ext4_journal.h"
@@ -43,9 +44,7 @@
 
 static inline void *zalloc(size_t size)
 {
-	void *p = memalign(ARCH_DMA_MINALIGN, size);
-	memset(p, 0, size);
-	return p;
+	return kzalloc(size, 0);
 }
 
 int ext4fs_read_inode(struct ext2_data *data, int ino,
-- 
2.35.3

Re: [PATCH v2 3/3] ext4: Fix zalloc()

[Heinrich Schuchardt]

On 8/9/24 11:54, Richard Weinberger wrote:
> Currently, zalloc() calls uncondtionally memset(),
> if the allocation failes, memset() will write to a null pointer.
>
> Fix by using kzalloc().
>
> Signed-off-by: Richard Weinberger <richard@nod.at>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   fs/ext4/ext4_common.h | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
> index 84500e990a..346752092b 100644
> --- a/fs/ext4/ext4_common.h
> +++ b/fs/ext4/ext4_common.h
> @@ -24,6 +24,7 @@
>   #include <ext4fs.h>
>   #include <malloc.h>
>   #include <asm/cache.h>
> +#include <linux/compat.h>
>   #include <linux/errno.h>
>   #if defined(CONFIG_EXT4_WRITE)
>   #include "ext4_journal.h"
> @@ -43,9 +44,7 @@
>
>   static inline void *zalloc(size_t size)
>   {
> -	void *p = memalign(ARCH_DMA_MINALIGN, size);
> -	memset(p, 0, size);
> -	return p;
> +	return kzalloc(size, 0);
>   }
>
>   int ext4fs_read_inode(struct ext2_data *data, int ino,

Re: [PATCH v2 3/3] ext4: Fix zalloc()

[Ilias Apalodimas]

On Fri, 9 Aug 2024 at 12:55, Richard Weinberger <richard@nod.at> wrote:
>
> Currently, zalloc() calls uncondtionally memset(),
> if the allocation failes, memset() will write to a null pointer.
>
> Fix by using kzalloc().
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ext4/ext4_common.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/ext4_common.h b/fs/ext4/ext4_common.h
> index 84500e990a..346752092b 100644
> --- a/fs/ext4/ext4_common.h
> +++ b/fs/ext4/ext4_common.h
> @@ -24,6 +24,7 @@
>  #include <ext4fs.h>
>  #include <malloc.h>
>  #include <asm/cache.h>
> +#include <linux/compat.h>
>  #include <linux/errno.h>
>  #if defined(CONFIG_EXT4_WRITE)
>  #include "ext4_journal.h"
> @@ -43,9 +44,7 @@
>
>  static inline void *zalloc(size_t size)
>  {
> -       void *p = memalign(ARCH_DMA_MINALIGN, size);
> -       memset(p, 0, size);
> -       return p;
> +       return kzalloc(size, 0);
>  }
>
>  int ext4fs_read_inode(struct ext2_data *data, int ino,
> --
> 2.35.3
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

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 <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 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;
>

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

[Richard Weinberger]

Heinrich,

Am Freitag, 9. August 2024, 18:13:27 CEST schrieb 'Heinrich Schuchardt' via upstream:
> 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?

https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html says:

Built-in Function: bool __builtin_add_overflow (type1 a, type2 b, type3 *res)

These built-in functions promote the first two operands into infinite precision signed type and perform addition on those promoted operands.
The result is then cast to the type the third pointer argument points to and stored there.
If the stored result is equal to the infinite precision result, the built-in functions return false, otherwise they return true.
As the addition is performed in infinite signed precision, these built-in functions have fully defined behavior for all argument values. 

So, I don't really see a problem here.

> 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.

In this specific case we could avoid the builtis, yes.
But I prefer using __builtin_add_overflow() as it offers a generic
and bullet prove mechanism to avoid these kind of bugs.

It's not that they are something new and funky.
Other projects use them since ages.

Thanks,
//richard

-- 
​​​​​sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT
UID/VAT Nr: ATU 66964118 | FN: 374287y

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

On Fri, Aug 09, 2024 at 06:29:34PM +0200, Richard Weinberger wrote:
> Heinrich,
> 
> Am Freitag, 9. August 2024, 18:13:27 CEST schrieb 'Heinrich Schuchardt' via upstream:
> > 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?
> 
> https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html says:
> 
> Built-in Function: bool __builtin_add_overflow (type1 a, type2 b, type3 *res)
> 
> These built-in functions promote the first two operands into infinite precision signed type and perform addition on those promoted operands.
> The result is then cast to the type the third pointer argument points to and stored there.
> If the stored result is equal to the infinite precision result, the built-in functions return false, otherwise they return true.
> As the addition is performed in infinite signed precision, these built-in functions have fully defined behavior for all argument values. 
> 
> So, I don't really see a problem here.
> 
> > 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.
> 
> In this specific case we could avoid the builtis, yes.
> But I prefer using __builtin_add_overflow() as it offers a generic
> and bullet prove mechanism to avoid these kind of bugs.
> 
> It's not that they are something new and funky.
> Other projects use them since ages.

I'm fine with using this buildin, it's widely enough implemented, for
good reason.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Tom Rini]

On Fri, 09 Aug 2024 11:54:28 +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 to u-boot/next, thanks!

-- 
Tom