| Message ID | 20240702194223.31998-2-richard@nod.at |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/2] ext4: Fix integer overflow in ext4fs_read_symlink() | expand |
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, 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. > 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; > } >
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
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 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(-)