| Message ID | 20240702194223.31998-1-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 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.
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;
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(-)