Re: [U-Boot] [PATCH 1/3 v2] ext4: fix possible crash on directory traversal, ignore deleted entries

Hi Stefan,

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

v2: Fix bad filename compare on delete, used substring only

fs/ext4/ext4_common.c | 58 +++++++++++++++++++-------------------------------- fs/ext4/ext4_write.c | 2 +- include/ext4fs.h | 2 +- 3 files changed, 23 insertions(+), 39 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index b00b84f..3ecd9a8 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,21 @@ 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 && (strlen(filename) == dir->namelen)
  

&&

•     (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 +809,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 +819,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 +831,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,

Reviewed-by: Lukasz Majewski l.majewski@samsung.com