[U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries
The following command triggers a segfault in search_dir: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ; ext4write host 0 0 /./foo 0x10'
The following command triggers a segfault in check_filename: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ; ext4write host 0 0 /. 0x10'
"." is the first entry in the directory, thus previous_dir is NULL. The whole previous_dir block in search_dir seems to be a bad copy from check_filename(...). As the changed data is not written to disk, the statement is mostly harmless, save the possible NULL-ptr reference.
Typically a file is unlinked by extending the direntlen of the previous entry. If the entry is the first entry in the directory block, it is invalidated by setting inode=0.
The inode==0 case is hard to trigger without crafted filesystems. It only hits if the first entry in a directory block is deleted and later a lookup for the entry (by name) is done.
Signed-off-by: Stefan Brüns stefan.bruens@rwth-aachen.de --- fs/ext4/ext4_common.c | 57 ++++++++++++++++++--------------------------------- fs/ext4/ext4_write.c | 2 +- include/ext4fs.h | 2 +- 3 files changed, 22 insertions(+), 39 deletions(-)
diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index b00b84f..16c5f53 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -511,16 +511,14 @@ fail: static int search_dir(struct ext2_inode *parent_inode, char *dirname) { int status; - int inodeno; + int inodeno = 0; int totalbytes; int templength; int direct_blk_idx; long int blknr; - int found = 0; char *ptr = NULL; unsigned char *block_buffer = NULL; struct ext2_dirent *dir = NULL; - struct ext2_dirent *previous_dir = NULL; struct ext_filesystem *fs = get_fs();
/* read the block no allocated to a file */ @@ -530,7 +528,7 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname) if (blknr == 0) goto fail;
- /* read the blocks of parenet inode */ + /* read the blocks of parent inode */ block_buffer = zalloc(fs->blksz); if (!block_buffer) goto fail; @@ -550,15 +548,9 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname) * space in the block that means * it is a last entry of directory entry */ - if (strlen(dirname) == dir->namelen) { + if (dir->inode && (strlen(dirname) == dir->namelen)) { if (strncmp(dirname, ptr + sizeof(struct ext2_dirent), dir->namelen) == 0) { - uint16_t new_len; - new_len = le16_to_cpu(previous_dir->direntlen); - new_len += le16_to_cpu(dir->direntlen); - previous_dir->direntlen = cpu_to_le16(new_len); inodeno = le32_to_cpu(dir->inode); - dir->inode = 0; - found = 1; break; } } @@ -569,19 +561,15 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname) /* traversing the each directory entry */ templength = le16_to_cpu(dir->direntlen); totalbytes = totalbytes + templength; - previous_dir = dir; dir = (struct ext2_dirent *)((char *)dir + templength); ptr = (char *)dir; }
- if (found == 1) { - free(block_buffer); - block_buffer = NULL; - return inodeno; - } - free(block_buffer); block_buffer = NULL; + + if (inodeno > 0) + return inodeno; }
fail: @@ -757,15 +745,13 @@ fail: return result_inode_no; }
-static int check_filename(char *filename, unsigned int blknr) +static int unlink_filename(char *filename, unsigned int blknr) { - unsigned int first_block_no_of_root; int totalbytes = 0; int templength = 0; int status, inodeno; int found = 0; char *root_first_block_buffer = NULL; - char *root_first_block_addr = NULL; struct ext2_dirent *dir = NULL; struct ext2_dirent *previous_dir = NULL; char *ptr = NULL; @@ -773,18 +759,15 @@ static int check_filename(char *filename, unsigned int blknr) int ret = -1;
/* get the first block of root */ - first_block_no_of_root = blknr; root_first_block_buffer = zalloc(fs->blksz); if (!root_first_block_buffer) return -ENOMEM; - root_first_block_addr = root_first_block_buffer; - status = ext4fs_devread((lbaint_t)first_block_no_of_root * - fs->sect_perblk, 0, + status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk, 0, fs->blksz, root_first_block_buffer); if (status == 0) goto fail;
- if (ext4fs_log_journal(root_first_block_buffer, first_block_no_of_root)) + if (ext4fs_log_journal(root_first_block_buffer, blknr)) goto fail; dir = (struct ext2_dirent *)root_first_block_buffer; ptr = (char *)dir; @@ -796,19 +779,20 @@ static int check_filename(char *filename, unsigned int blknr) * is free availble space in the block that * means it is a last entry of directory entry */ - if (strlen(filename) == dir->namelen) { - if (strncmp(filename, ptr + sizeof(struct ext2_dirent), - dir->namelen) == 0) { + if (dir->inode && (strncmp(ptr + sizeof(struct ext2_dirent), + filename, dir->namelen) == 0)) { + printf("file found, deleting\n"); + inodeno = le32_to_cpu(dir->inode); + if (previous_dir) { uint16_t new_len; - printf("file found deleting\n"); new_len = le16_to_cpu(previous_dir->direntlen); new_len += le16_to_cpu(dir->direntlen); previous_dir->direntlen = cpu_to_le16(new_len); - inodeno = le32_to_cpu(dir->inode); + } else { dir->inode = 0; - found = 1; - break; } + found = 1; + break; }
if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen)) @@ -824,8 +808,7 @@ static int check_filename(char *filename, unsigned int blknr)
if (found == 1) { - if (ext4fs_put_metadata(root_first_block_addr, - first_block_no_of_root)) + if (ext4fs_put_metadata(root_first_block_buffer, blknr)) goto fail; ret = inodeno; } @@ -835,7 +818,7 @@ fail: return ret; }
-int ext4fs_filename_check(char *filename) +int ext4fs_filename_unlink(char *filename) { short direct_blk_idx = 0; long int blknr = -1; @@ -847,7 +830,7 @@ int ext4fs_filename_check(char *filename) blknr = read_allocated_block(g_parent_inode, direct_blk_idx); if (blknr == 0) break; - inodeno = check_filename(filename, blknr); + inodeno = unlink_filename(filename, blknr); if (inodeno != -1) return inodeno; } diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c index d03d692..f5811aa 100644 --- a/fs/ext4/ext4_write.c +++ b/fs/ext4/ext4_write.c @@ -865,7 +865,7 @@ int ext4fs_write(const char *fname, unsigned char *buffer, if (ext4fs_iget(parent_inodeno, g_parent_inode)) goto fail; /* check if the filename is already present in root */ - existing_file_inodeno = ext4fs_filename_check(filename); + existing_file_inodeno = ext4fs_filename_unlink(filename); if (existing_file_inodeno != -1) { ret = ext4fs_delete_file(existing_file_inodeno); fs->first_pass_bbmap = 0; diff --git a/include/ext4fs.h b/include/ext4fs.h index 13d2c56..e3f6216 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -124,7 +124,7 @@ extern int gindex;
int ext4fs_init(void); void ext4fs_deinit(void); -int ext4fs_filename_check(char *filename); +int ext4fs_filename_unlink(char *filename); int ext4fs_write(const char *fname, unsigned char *buffer, unsigned long sizebytes); int ext4_write_file(const char *filename, void *buf, loff_t offset, loff_t len,
On Sun, Aug 14, 2016 at 05:11:04AM +0200, Stefan Brüns wrote:
The following command triggers a segfault in search_dir: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ; ext4write host 0 0 /./foo 0x10'
The following command triggers a segfault in check_filename: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ; ext4write host 0 0 /. 0x10'
"." is the first entry in the directory, thus previous_dir is NULL. The whole previous_dir block in search_dir seems to be a bad copy from check_filename(...). As the changed data is not written to disk, the statement is mostly harmless, save the possible NULL-ptr reference.
Typically a file is unlinked by extending the direntlen of the previous entry. If the entry is the first entry in the directory block, it is invalidated by setting inode=0.
The inode==0 case is hard to trigger without crafted filesystems. It only hits if the first entry in a directory block is deleted and later a lookup for the entry (by name) is done.
Signed-off-by: Stefan Brüns stefan.bruens@rwth-aachen.de
fs/ext4/ext4_common.c | 57 ++++++++++++++++++--------------------------------- fs/ext4/ext4_write.c | 2 +- include/ext4fs.h | 2 +- 3 files changed, 22 insertions(+), 39 deletions(-)
Can you please add the test case to the existing scripts? Thanks!
On Freitag, 19. August 2016 15:54:51 CEST you wrote:
On Sun, Aug 14, 2016 at 05:11:04AM +0200, Stefan Brüns wrote:
The following command triggers a segfault in search_dir: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
ext4write host 0 0 /./foo 0x10'The following command triggers a segfault in check_filename: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
ext4write host 0 0 /. 0x10'"." is the first entry in the directory, thus previous_dir is NULL. The whole previous_dir block in search_dir seems to be a bad copy from check_filename(...). As the changed data is not written to disk, the statement is mostly harmless, save the possible NULL-ptr reference.
Typically a file is unlinked by extending the direntlen of the previous entry. If the entry is the first entry in the directory block, it is invalidated by setting inode=0.
The inode==0 case is hard to trigger without crafted filesystems. It only hits if the first entry in a directory block is deleted and later a lookup for the entry (by name) is done.
Signed-off-by: Stefan Brüns stefan.bruens@rwth-aachen.de
fs/ext4/ext4_common.c | 57 ++++++++++++++++++--------------------------------- fs/ext4/ext4_write.c | 2 +- include/ext4fs.h | 2 +- 3 files changed, 22 insertions(+), 39 deletions(-)
Can you please add the test case to the existing scripts? Thanks!
Adding this to the current test script is somewhat problematic. The test runs all tests for fat and ext4, so each testcase should be file system agnostic. Unfortunately fat and ext4 (at least as implemented in U-Boot) have different semantics, as ext4 in U-Boot requires all path to absolute paths, whereas fat seems to require something else (relative path? absolute path, but without leading '/'?).
Calling 'fatwrite host 0 0 /. 0x10' happily creates a directory! called '/.', 'fatwrite host 0 0 /./foo 0x10' creates a file and copletely messes up the filesystem (according to fsck.vfat and mounting the fs in linux).
Any advise?
Kind regards,
Stefan
On Sun, Aug 28, 2016 at 09:44:41PM +0200, Stefan Bruens wrote:
On Freitag, 19. August 2016 15:54:51 CEST you wrote:
On Sun, Aug 14, 2016 at 05:11:04AM +0200, Stefan Brüns wrote:
The following command triggers a segfault in search_dir: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
ext4write host 0 0 /./foo 0x10'The following command triggers a segfault in check_filename: ./sandbox/u-boot -c 'host bind 0 ./sandbox/test/fs/3GB.ext4.img ;
ext4write host 0 0 /. 0x10'"." is the first entry in the directory, thus previous_dir is NULL. The whole previous_dir block in search_dir seems to be a bad copy from check_filename(...). As the changed data is not written to disk, the statement is mostly harmless, save the possible NULL-ptr reference.
Typically a file is unlinked by extending the direntlen of the previous entry. If the entry is the first entry in the directory block, it is invalidated by setting inode=0.
The inode==0 case is hard to trigger without crafted filesystems. It only hits if the first entry in a directory block is deleted and later a lookup for the entry (by name) is done.
Signed-off-by: Stefan Brüns stefan.bruens@rwth-aachen.de
fs/ext4/ext4_common.c | 57 ++++++++++++++++++--------------------------------- fs/ext4/ext4_write.c | 2 +- include/ext4fs.h | 2 +- 3 files changed, 22 insertions(+), 39 deletions(-)
Can you please add the test case to the existing scripts? Thanks!
Adding this to the current test script is somewhat problematic. The test runs all tests for fat and ext4, so each testcase should be file system agnostic. Unfortunately fat and ext4 (at least as implemented in U-Boot) have different semantics, as ext4 in U-Boot requires all path to absolute paths, whereas fat seems to require something else (relative path? absolute path, but without leading '/'?).
Calling 'fatwrite host 0 0 /. 0x10' happily creates a directory! called '/.', 'fatwrite host 0 0 /./foo 0x10' creates a file and copletely messes up the filesystem (according to fsck.vfat and mounting the fs in linux).
Any advise?
Can we fix this up in the argument parsing? This sounds like it's showing some bugs in the fatwrite parsing code itself.
On Freitag, 2. September 2016 10:53:08 CEST you wrote:
Adding this to the current test script is somewhat problematic. The test runs all tests for fat and ext4, so each testcase should be file system agnostic. Unfortunately fat and ext4 (at least as implemented in U-Boot) have different semantics, as ext4 in U-Boot requires all path to absolute paths, whereas fat seems to require something else (relative path? absolute path, but without leading '/'?).
Calling 'fatwrite host 0 0 /. 0x10' happily creates a directory! called '/.', 'fatwrite host 0 0 /./foo 0x10' creates a file and copletely messes up the filesystem (according to fsck.vfat and mounting the fs in linux).
Any advise?
Can we fix this up in the argument parsing? This sounds like it's showing some bugs in the fatwrite parsing code itself.
The fatwrite code is hardly doing any parsing at all. It does not strip any "/" or "" characters, does not interpret these as dir delimiters, and just pushes whatever it gets into the directory.
For the lookup, it uses a function which is quite similar to the fatload/fatls function, but still different. It only traverses the root directory.
The whole fatwrite seems to be a 50% almost verbatim copy of the read implementation and shares hardly any code. The problem is the "almost" copy, most functions have minor differences.
I think lots of code could be removed from fatwrite if the read implementation where better structured, but e.g. the main entry point is a huge function which, depending on some flags either prints the directory listing while walking/traversing the tree, returns the file size, loads a specified file into a buffer, or errors out in case some path element was not reachable.
So, currently there are two options for the bad fatwrite behaviour: a) add even more duplicate code to fatwrite b) restructure fatread to be reusable
Opinions, please!
Kind regards,
Stefan
participants (4)
-
Brüns, Stefan -
Stefan Bruens
-
Stefan Brüns
-
Tom Rini