[U-Boot] [PATCH 2/3] ext4: Fix handling of direntlen in unlink_filename

[Lukasz Majewski]

Hi Stefan,

> The direntlen checks were quite bogus, i.e. the loop termination used
> "len + offset == blocksize" (exact match only), and checked for a
> direntlen less than 0. The latter can never happen as the len is
> unsigned, this has been reported by Coverity, CID 153384.
> 
> Use the same code as in search_dir for directory traversal. This code
> has the correct checks for direntlen >= sizeof(struct dirent), and
> offset < blocksize.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 45
> +++++++++++++++++---------------------------- 1 file changed, 17
> insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 699a640..11be0b7 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -854,16 +854,15 @@ fail:
>  
>  static int unlink_filename(char *filename, unsigned int blknr)
>  {
> -	int templength = 0;
> -	int status, inodeno;
> -	int found = 0;
> +	int status;
> +	int inodeno = 0;
>  	int offset;
>  	char *block_buffer = NULL;
>  	struct ext2_dirent *dir = NULL;
> -	struct ext2_dirent *previous_dir = NULL;
> -	char *ptr = NULL;
> +	struct ext2_dirent *previous_dir;
>  	struct ext_filesystem *fs = get_fs();
>  	int ret = -1;
> +	char *direntname;
>  
>  	block_buffer = zalloc(fs->blksz);
>  	if (!block_buffer)
> @@ -877,20 +876,18 @@ static int unlink_filename(char *filename,
> unsigned int blknr) 
>  	if (ext4fs_log_journal(block_buffer, blknr))
>  		goto fail;
> -	dir = (struct ext2_dirent *)block_buffer;
> -	ptr = (char *)dir;
>  	offset = 0;
> -	while (le16_to_cpu(dir->direntlen) >= 0) {
> -		/*
> -		 * blocksize-offset because last
> -		 * directory length i.e., *dir->direntlen
> -		 * is free availble space in the block that
> -		 * means it is a last entry of directory entry
> -		 */
> +	do {
> +		previous_dir = dir;
> +		dir = (struct ext2_dirent *)(block_buffer + offset);
> +		direntname = (char *)(dir) + sizeof(struct
> ext2_dirent); +
> +		int direntlen = le16_to_cpu(dir->direntlen);
> +		if (direntlen < sizeof(struct ext2_dirent))
> +			break;
> +
>  		if (dir->inode && (strlen(filename) == dir->namelen)
> &&
> -		    (strncmp(ptr + sizeof(struct ext2_dirent),
> -			     filename, dir->namelen) == 0)) {
> -			printf("file found, deleting\n");
> +		    (strncmp(direntname, filename, dir->namelen) ==
> 0)) { inodeno = le32_to_cpu(dir->inode);
>  			if (previous_dir) {
>  				uint16_t new_len;
> @@ -900,23 +897,15 @@ static int unlink_filename(char *filename,
> unsigned int blknr) } else {
>  				dir->inode = 0;
>  			}
> -			found = 1;
>  			break;
>  		}
>  
> -		if (fs->blksz - offset ==
> le16_to_cpu(dir->direntlen))
> -			break;
> +		offset += direntlen;
>  
> -		/* traversing the each directory entry */
> -		templength = le16_to_cpu(dir->direntlen);
> -		offset = offset + templength;
> -		previous_dir = dir;
> -		dir = (struct ext2_dirent *)((char *)dir +
> templength);
> -		ptr = (char *)dir;
> -	}
> +	} while (offset < fs->blksz);
>  
> +	if (inodeno > 0) {
>  
> -	if (found == 1) {
>  		if (ext4fs_put_metadata(block_buffer, blknr))
>  			goto fail;
>  		ret = inodeno;

Reviewed-by: Lukasz Majewski <l.majewski at samsung.com>

-- 
Best regards,

Lukasz Majewski

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