linux-ext4 - Re: Errors reported by Coverity in ext3

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]

Message-id: <20090420221441.GI3209@webber.adilger.int>
Date:	Mon, 20 Apr 2009 16:14:41 -0600
From:	Andreas Dilger <adilger@....com>
To:	Amir Goldor <amir73il@...rs.sourceforge.net>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: Errors reported by Coverity in ext3

Thanks for the patch, unfortunately it is a NACK, since there are a few
bugs in the error handling, and some additional cleanups that can be done.

On Apr 16, 2009  12:30 +0300, Amir Goldor wrote:
> Also, the patch that you posted back in 2006 mostly handled
> jbd error check for "retaking write access" in ext3_clear_blocks().
> Do you have a newer patch for that? are you still using that old patch?

I don't recall which patch you refer to.

More comments inline.

> diff -up linux-2.6.28.orig/fs/ext3/namei.c linux-2.6.28/fs/ext3/namei.c
> --- linux-2.6.28.orig/fs/ext3/namei.c	2009-04-16 12:11:44.000000000 +0300
> +++ linux-2.6.28/fs/ext3/namei.c	2009-04-16 12:13:32.000000000 +0300
> @@ -1770,7 +1772,13 @@ static int ext3_mkdir(struct inode *dir
>  	BUFFER_TRACE(dir_block, "get_write_access");
> -	ext3_journal_get_write_access(handle, dir_block);
> +	err = ext3_journal_get_write_access(handle, dir_block);
> +	if (err) {
> +		drop_nlink(inode); /* is this nlink == 0? */
> +		ext3_mark_inode_dirty(handle, inode);
> +		iput (inode);
> +		goto out_stop;
> +	}

For the 2.6.29+ kernels you need to also include "unlock_new_inode(inode)"
after drop_nlink() in the error handling path.

It seems in all kernels you need to add "brelse(dir_block)" here as well,
because there was a reference gotten in ext3_bread() just above.  It might
be enough to do:

	if (err) {
		brelse(dir_block);
		goto out_clear_inode;
	}

> @@ -2302,7 +2310,9 @@ static int ext3_rename (struct inode * o
>  		BUFFER_TRACE(new_bh, "get write access");
> -		ext3_journal_get_write_access(handle, new_bh);
> +		retval = ext3_journal_get_write_access(handle, new_bh);
> +		if (retval)
> +			goto end_rename;

Similarly, this also needs a "brelse(new_bh)" before "goto end_rename".

> @@ -2360,7 +2370,14 @@ static int ext3_rename (struct inode * o
>  	ext3_update_dx_flag(old_dir);
>  	if (dir_bh) {
>  		BUFFER_TRACE(dir_bh, "get_write_access");
> +		retval = ext3_journal_get_write_access(handle, dir_bh);
> +		if (retval) {
> +			ext3_warning(old_dir->i_sb, "ext3_rename",
> +					"Updating new directory (%lu) parent link, %d, error=%d",
> +					new_dir->i_ino, new_dir->i_nlink, retval);
> +		}
> +	}

At this point, if we cannot get write access to the directory buffer, it
is really a bit too late to do much about it.  It may make more sense to
instead call ext3_journal_get_write_access() right after ext3_bread() is
called dor this buffer, so that the error can be checked and the rename
aborted before any changes are made.

Note also that there are some places where ext3_journal_dirty_metadata()
are called in these same code paths, but the error is not checked by the
caller.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html