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

[Lukasz Majewski <l.majewski@samsung.com>]

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>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group