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, 8 Feb 2016 14:40:21 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	"vikram.jadhav07" <vikramjadhavpucsd2007@...il.com>
Cc:	Theodore Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: Buffer Head Leak In mmp

On Jan 20, 2016, at 8:48 AM, vikram.jadhav07 <vikramjadhavpucsd2007@...il.com> wrote:
> 
> Description:  ext4: Buffer Head Leak In mmp
> 
> 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.

I was going to give this a Signed-off-by, since it matches patches that we
have included into the Lustre ext4 patch series, but it seems the code is
different in newer kernels because of the metadata checksum feature and
could be improved a bit further.

> Signed-off-by: Jadhav Vikram <vikramjadhavpucsd2007@...il.com>
> 
> Patch:
> =====
> --- /root/linux.orig/fs/ext4/mmp.c      2016-01-18 07:23:06.227519005 +0530
> +++ /root/linux/fs/ext4/mmp.c   2016-01-18 07:31:18.545518552 +0530
> @@ -98,12 +98,17 @@
>         }
> 
>         mmp = (struct mmp_struct *)((*bh)->b_data);
> +       if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
>                ret = -EFSCORRUPTED;
> +               brelse(*bh);
> +               *bh = NULL;
> +       } else if (!ext4_mmp_csum_verify(sb, mmp)) {
>                ret = -EFSBADCRC;
> +               brelse(*bh);
> +               *bh = NULL;
> +       } else {
>                return 0;
> +       }

Having the normal return be at the end of the error handling is non-standard
and would be better structured to have "goto out_bh;" in each of the
error cases, and then the "naked" return at the end.  That avoids duplicating
the error handling, and will avoid bugs/leaks in the future if this code is
changed to add more failure conditions or returns.

	mmp = (struct mmp_struct *)((*bh)->b_data);
	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
		ret = -EFSCORRUPTED;
		goto out_bh;
	}
	if (!ext4_mmp_csum_verify(sb, mmp)) {
		ret = -EFSBADCRC;
		goto out_bh;
	}

	return 0;

out_bh:
	brelse(*bh);
	*bh = NULL;
warn_exit:
	ext4_warning(sb, "Error %d while reading MMP block %llu",
		     ret, mmp_block);
	return ret;

> @@ -225,6 +230,7 @@
>                                             "The filesystem seems to have been"
>                                             " multiply mounted.");
>                                ext4_error(sb, "abort");
> +                               put_bh(bh_check);
>                                goto failed;
>                        }
>                        put_bh(bh_check);

I was going to suggest something similar here to consolidate the "put_bh()"
calls, but it doesn't look like that can be done easily.

Instead I found a bug because this isn't setting s_mmp_tsk = NULL as the
other error branches are doing, which will lead to the filesystem cleanup
code trying to stop this thread again.  Could you please remove the duplicate
"s_mmp_tsk = NULL" assignments.  I also see that the error handling is still
not quite correct because "retval" is not set in this branch, which further
emphasizes the benefits of using the "goto NNN" style of error handling.
It should be something like:

			if (retval) {
				ext4_error(sb, "error reading MMP data: %d",
					   retval);
				goto out_tsk;
			}
			if (mmp->mmp_seq != ...) {
				dump_mmp_msg(sb, mmp_check, ...);
				ext4_error(sb, "abort due to multiple mount");
				put_bh(bh_check);
				retval = -EBUSY;
				goto out_tsk;
			}
			put_bh(bh_check);
		}

		:
		:
	}

	:
	:

out_tsk:
	EXT4_SB(sb)->s_mmp_tsk = NULL;
out_free:
	kfree(data);
	brelse(bh);
	return retval;
}

Note "failed:" is renamed to "out_free:" since that code path is also used
in non-failure cases, for normal thread exit, and also to ensure that the
other two cases that explicitly set s_mmp_tsk = NULL and jumped to "failed:"
will also be cleaned up.

Cheers, Andreas

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