linux-ext4 - Re: [PATCH] ext4: fix reading new encrypted symlinks on no-journal filesystems

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: <20161201195731.GA131121@google.com>
Date:   Thu, 1 Dec 2016 11:57:31 -0800
From:   Eric Biggers <ebiggers@...gle.com>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Andreas Dilger <adilger@...ger.ca>,
        linux-ext4 <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: fix reading new encrypted symlinks on no-journal
 filesystems

On Thu, Dec 01, 2016 at 02:27:05PM -0500, Theodore Ts'o wrote:
> So in the long term I think we can move to using i_size to determine
> fast symlinks, but I think there's a bigger issue hiding here, which
> is that we shouldn't be using delayed allocation for symlinks in the
> first place.  In the first place, symlinks will never be more than a
> block, so there's no advantage in using delalloc.  In the second
> place, it means that on a crash the symlink could invalid (zero
> length) --- and on a commit the symlink should be commited to disk.
> 
> Eric, do you have a test case which verifies this?  Normally I would
> think this rarely happens because the dentry cache should hide this
> particular issue.  I think a simpler fix up, which also avoids the
> "symlink could be lost on a crash" problem, is this:
> 
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b48ca0392b9c..4ffb680780e5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2902,7 +2902,8 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
>  
>  	index = pos >> PAGE_SHIFT;
>  
> -	if (ext4_nonda_switch(inode->i_sb)) {
> +	if (ext4_nonda_switch(inode->i_sb) ||
> +	    S_ISLNK(inode->i_mode)) {
>  		*fsdata = (void *)FALL_BACK_TO_NONDELALLOC;
>  		return ext4_write_begin(file, mapping, pos,
>  					len, flags, pagep, fsdata);
> 
> 
> 					     	    - Ted
> 

Hi Ted,

The problem of a slow encrypted symlink being misinterpreted as a fast one can
be reproduced by generic/360 if you run it just right:

	kvm-xfstests -c nojournal -m test_dummy_encryption generic/360

It can also be reproduced by generic/402 from v2 of my encryption xfstests
patchset with 'kvm-xfstests -c nojournal generic/402'.  But running that one
requires applying xfstests and xfsprogs patches (until they get upstream).

The problem can be reliably reproduced because the symlink target is not cached
by the VFS.  ext4_encrypted_get_link() gets called whenever the symlink is
followed or whenever someone does sys_readlink.

I agree that delayed allocation doesn't make sense for symlinks so your proposed
fix is better.  I verified that it passes both of the xfstests mentioned above.

Eric
--
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