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

[PATCH 1/2] 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 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;
 
-- 
2.35.3

[PATCH 2/2] ext4: Fix zalloc()

[Richard Weinberger]

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

Re: [PATCH 2/2] ext4: Fix zalloc()

[Tom Rini]

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

On Tue, Jul 02, 2024 at 09:42:23PM +0200, Richard Weinberger wrote:

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

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

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

Re: [PATCH 2/2] ext4: Fix zalloc()

[Richard Weinberger]

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

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

Re: [PATCH 2/2] ext4: Fix zalloc()

[Heinrich Schuchardt]

On 02.07.24 21:42, Richard Weinberger wrote:
> 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);

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

Re: [PATCH 2/2] ext4: Fix zalloc()

[Richard Weinberger]

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

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

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

[Heinrich Schuchardt]

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

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

[Richard Weinberger]

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

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

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

[Heinrich Schuchardt]

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

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

[Richard Weinberger]

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

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

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

[Tom Rini]

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

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.

-- 
Tom

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