Re: [U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries
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 > > > --- > > > > > > 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. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries
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 > > --- > > > > 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 -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 work: +49 2405 49936-424 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries
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 > --- > 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! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[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
---
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 (!
