* [PATCH] ext2fs: fix to avoid invalid memory access
@ 2020-06-18 15:16 Wang Shilong
2020-06-18 19:58 ` Eric Biggers
0 siblings, 1 reply; 2+ messages in thread
From: Wang Shilong @ 2020-06-18 15:16 UTC (permalink / raw)
To: linux-ext4; +Cc: Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
Valgrind reported error messages like following:
==129205== Address 0x1b804b04 is 4 bytes after a block of size 4,096 alloc'd
==129205== at 0x483980B: malloc (vg_replace_malloc.c:307)
==129205== by 0x44F973: ext2fs_get_mem (ext2fs.h:1846)
==129205== by 0x44F973: ext2fs_get_pathname (get_pathname.c:162)
==129205== by 0x430917: print_pathname (message.c:212)
==129205== by 0x430FB1: expand_percent_expression (message.c:462)
==129205== by 0x430FB1: print_e2fsck_message (message.c:544)
==129205== by 0x430BED: expand_at_expression (message.c:262)
==129205== by 0x430BED: print_e2fsck_message (message.c:528)
==129205== by 0x430450: fix_problem (problem.c:2494)
==129205== by 0x423F8B: e2fsck_process_bad_inode (pass2.c:1929)
==129205== by 0x425AE8: check_dir_block (pass2.c:1407)
==129205== by 0x426942: check_dir_block2 (pass2.c:961)
==129205== by 0x445736: ext2fs_dblist_iterate3.part.0 (dblist.c:254)
==129205== by 0x423835: e2fsck_pass2 (pass2.c:187)
==129205== by 0x414B19: e2fsck_run (e2fsck.c:257)
Dir block might be corrupted and cause the next dirent is out
of block size boundary, even though we have the check to avoid
problem, memory check tools like valgrind still complains it.
Patch try to fix the problem by checking if offset exceed max
offset firstly before getting the pointer.
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
lib/ext2fs/csum.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
index c2550365..643777fd 100644
--- a/lib/ext2fs/csum.c
+++ b/lib/ext2fs/csum.c
@@ -260,22 +260,24 @@ static errcode_t __get_dirent_tail(ext2_filsys fs,
void *top;
struct ext2_dir_entry_tail *t;
unsigned int rec_len;
+ unsigned int max_len;
errcode_t retval = 0;
__u16 (*translate)(__u16) = (need_swab ? disk_to_host16 : do_nothing16);
d = dirent;
top = EXT2_DIRENT_TAIL(dirent, fs->blocksize);
+ max_len = (char *)top - (char *)dirent;
rec_len = translate(d->rec_len);
while ((void *) d < top) {
if ((rec_len < 8) || (rec_len & 0x03))
return EXT2_ET_DIR_CORRUPTED;
+ if ((char *)d - (char *)dirent + rec_len > max_len)
+ return EXT2_ET_DIR_CORRUPTED;
d = (struct ext2_dir_entry *)(((char *)d) + rec_len);
rec_len = translate(d->rec_len);
}
- if ((char *)d > ((char *)dirent + fs->blocksize))
- return EXT2_ET_DIR_CORRUPTED;
if (d != top)
return EXT2_ET_DIR_NO_SPACE_FOR_CSUM;
--
2.25.4
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] ext2fs: fix to avoid invalid memory access
2020-06-18 15:16 [PATCH] ext2fs: fix to avoid invalid memory access Wang Shilong
@ 2020-06-18 19:58 ` Eric Biggers
0 siblings, 0 replies; 2+ messages in thread
From: Eric Biggers @ 2020-06-18 19:58 UTC (permalink / raw)
To: Wang Shilong; +Cc: linux-ext4, Wang Shilong
On Fri, Jun 19, 2020 at 12:16:03AM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> Valgrind reported error messages like following:
>
> ==129205== Address 0x1b804b04 is 4 bytes after a block of size 4,096 alloc'd
> ==129205== at 0x483980B: malloc (vg_replace_malloc.c:307)
> ==129205== by 0x44F973: ext2fs_get_mem (ext2fs.h:1846)
> ==129205== by 0x44F973: ext2fs_get_pathname (get_pathname.c:162)
> ==129205== by 0x430917: print_pathname (message.c:212)
> ==129205== by 0x430FB1: expand_percent_expression (message.c:462)
> ==129205== by 0x430FB1: print_e2fsck_message (message.c:544)
> ==129205== by 0x430BED: expand_at_expression (message.c:262)
> ==129205== by 0x430BED: print_e2fsck_message (message.c:528)
> ==129205== by 0x430450: fix_problem (problem.c:2494)
> ==129205== by 0x423F8B: e2fsck_process_bad_inode (pass2.c:1929)
> ==129205== by 0x425AE8: check_dir_block (pass2.c:1407)
> ==129205== by 0x426942: check_dir_block2 (pass2.c:961)
> ==129205== by 0x445736: ext2fs_dblist_iterate3.part.0 (dblist.c:254)
> ==129205== by 0x423835: e2fsck_pass2 (pass2.c:187)
> ==129205== by 0x414B19: e2fsck_run (e2fsck.c:257)
>
> Dir block might be corrupted and cause the next dirent is out
> of block size boundary, even though we have the check to avoid
> problem, memory check tools like valgrind still complains it.
>
> Patch try to fix the problem by checking if offset exceed max
> offset firstly before getting the pointer.
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> lib/ext2fs/csum.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
> index c2550365..643777fd 100644
> --- a/lib/ext2fs/csum.c
> +++ b/lib/ext2fs/csum.c
> @@ -260,22 +260,24 @@ static errcode_t __get_dirent_tail(ext2_filsys fs,
> void *top;
> struct ext2_dir_entry_tail *t;
> unsigned int rec_len;
> + unsigned int max_len;
> errcode_t retval = 0;
> __u16 (*translate)(__u16) = (need_swab ? disk_to_host16 : do_nothing16);
>
> d = dirent;
> top = EXT2_DIRENT_TAIL(dirent, fs->blocksize);
>
> + max_len = (char *)top - (char *)dirent;
> rec_len = translate(d->rec_len);
> while ((void *) d < top) {
> if ((rec_len < 8) || (rec_len & 0x03))
> return EXT2_ET_DIR_CORRUPTED;
> + if ((char *)d - (char *)dirent + rec_len > max_len)
> + return EXT2_ET_DIR_CORRUPTED;
> d = (struct ext2_dir_entry *)(((char *)d) + rec_len);
> rec_len = translate(d->rec_len);
> }
>
> - if ((char *)d > ((char *)dirent + fs->blocksize))
> - return EXT2_ET_DIR_CORRUPTED;
> if (d != top)
> return EXT2_ET_DIR_NO_SPACE_FOR_CSUM;
This looks buggy. Previously this returned EXT2_ET_DIR_NO_SPACE_FOR_CSUM if the
last dirent extends beyond where the metadata checksum entry is supposed to
begin, but doesn't exceed fs->blocksize. But your change makes it return
EXT2_ET_DIR_CORRUPTED in that case, which is potentially a regression.
How about:
diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
index 54b53a3c..0dbb4963 100644
--- a/lib/ext2fs/csum.c
+++ b/lib/ext2fs/csum.c
@@ -266,16 +266,14 @@ static errcode_t __get_dirent_tail(ext2_filsys fs,
d = dirent;
top = EXT2_DIRENT_TAIL(dirent, fs->blocksize);
- rec_len = translate(d->rec_len);
while ((void *) d < top) {
- if ((rec_len < 8) || (rec_len & 0x03))
+ rec_len = translate(d->rec_len);
+ if ((rec_len < 8) || (rec_len & 0x03) ||
+ (rec_len > (char *)dirent + fs->blocksize - (char *)d))
return EXT2_ET_DIR_CORRUPTED;
d = (struct ext2_dir_entry *)(((char *)d) + rec_len);
- rec_len = translate(d->rec_len);
}
- if ((char *)d > ((char *)dirent + fs->blocksize))
- return EXT2_ET_DIR_CORRUPTED;
if (d != top)
return EXT2_ET_DIR_NO_SPACE_FOR_CSUM;
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-06-18 19:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-18 15:16 [PATCH] ext2fs: fix to avoid invalid memory access Wang Shilong
2020-06-18 19:58 ` Eric Biggers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox