linux-ext4 - Re: [PATCH] ext4: Buffer Head Leak In mmp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 14 Mar 2016 02:41:34 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Theodore Ts'o <tytso@....edu>
Cc:	"vikram.jadhav07" <vikramjadhavpucsd2007@...il.com>,
	linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: Buffer Head Leak In mmp

On Mar 13, 2016, at 3:57 PM, Theodore Ts'o <tytso@....edu> wrote:
> 
> I've made the changes Andreas suggested, and ended up with this.  Andreas, PTAL?

Sorry, not sure what "PTAL" means... :-)

In any case, the patch looks good to me.

Reviewed-by: Andreas Dilger <adilger@...ger.ca.

> commit 0304688676bdfc8159e165313d71da19c118ba27
> Author: vikram.jadhav07 <vikramjadhavpucsd2007@...il.com>
> Date:   Sun Mar 13 17:56:52 2016 -0400
> 
>    ext4: clean up error handling in the MMP support
> 
>    There is memory leak as both caller function kmmpd() and callee
>    read_mmp_block() not releasing bh_check  (i.e buffer_head).
>    Given patch fixes this problem.
> 
>    [ Additional changes suggested by Andreas Dilger -- TYT ]
> 
>    Signed-off-by: Jadhav Vikram <vikramjadhavpucsd2007@...il.com>
>    Signed-off-by: Theodore Ts'o <tytso@....edu>
> 
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 0a512aa..2444527 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -91,21 +91,22 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
> 	submit_bh(READ_SYNC | REQ_META | REQ_PRIO, *bh);
> 	wait_on_buffer(*bh);
> 	if (!buffer_uptodate(*bh)) {
> -		brelse(*bh);
> -		*bh = NULL;
> 		ret = -EIO;
> 		goto warn_exit;
> 	}
> -
> 	mmp = (struct mmp_struct *)((*bh)->b_data);
> -	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC)
> +	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
> 		ret = -EFSCORRUPTED;
> -	else if (!ext4_mmp_csum_verify(sb, mmp))
> +		goto warn_exit;
> +	}
> +	if (!ext4_mmp_csum_verify(sb, mmp)) {
> 		ret = -EFSBADCRC;
> -	else
> -		return 0;
> -
> +		goto warn_exit;
> +	}
> +	return 0;
> warn_exit:
> +	brelse(*bh);
> +	*bh = NULL;
> 	ext4_warning(sb, "Error %d while reading MMP block %llu",
> 		     ret, mmp_block);
> 	return ret;
> @@ -181,15 +182,13 @@ static int kmmpd(void *data)
> 		    EXT4_FEATURE_INCOMPAT_MMP)) {
> 			ext4_warning(sb, "kmmpd being stopped since MMP feature"
> 				     " has been disabled.");
> -			EXT4_SB(sb)->s_mmp_tsk = NULL;
> -			goto failed;
> +			goto exit_thread;
> 		}
> 
> 		if (sb->s_flags & MS_RDONLY) {
> 			ext4_warning(sb, "kmmpd being stopped since filesystem "
> 				     "has been remounted as readonly.");
> -			EXT4_SB(sb)->s_mmp_tsk = NULL;
> -			goto failed;
> +			goto exit_thread;
> 		}
> 
> 		diff = jiffies - last_update_time;
> @@ -211,9 +210,7 @@ static int kmmpd(void *data)
> 			if (retval) {
> 				ext4_error(sb, "error reading MMP data: %d",
> 					   retval);
> -
> -				EXT4_SB(sb)->s_mmp_tsk = NULL;
> -				goto failed;
> +				goto exit_thread;
> 			}
> 
> 			mmp_check = (struct mmp_struct *)(bh_check->b_data);
> @@ -225,7 +222,9 @@ static int kmmpd(void *data)
> 					     "The filesystem seems to have been"
> 					     " multiply mounted.");
> 				ext4_error(sb, "abort");
> -				goto failed;
> +				put_bh(bh_check);
> +				retval = -EBUSY;
> +				goto exit_thread;
> 			}
> 			put_bh(bh_check);
> 		}
> @@ -248,7 +247,8 @@ static int kmmpd(void *data)
> 
> 	retval = write_mmp_block(sb, bh);
> 
> -failed:
> +exit_thread:
> +	EXT4_SB(sb)->s_mmp_tsk = NULL;
> 	kfree(data);
> 	brelse(bh);
> 	return retval;


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)