linux-ext4 - Re: [PATCH] ext4: don't BUG when truncating encrypted inodes on the orphan list

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: <20170213181955.GA44321@google.com>
Date:   Mon, 13 Feb 2017 10:19:55 -0800
From:   Eric Biggers <ebiggers@...gle.com>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Eric Biggers <ebiggers3@...il.com>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: don't BUG when truncating encrypted inodes on the
 orphan list

On Mon, Feb 13, 2017 at 10:19:27AM -0500, Theodore Ts'o wrote:
> 
> > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > > index bc282f9d0969..831d025e59ad 100644
> > > --- a/fs/ext4/inode.c
> > > +++ b/fs/ext4/inode.c
> > > @@ -3944,6 +3944,10 @@ static int ext4_block_truncate_page(handle_t *handle,
> > >  	unsigned blocksize;
> > >  	struct inode *inode = mapping->host;
> > >  
> > > +	/* If we are processing an encrypted inode during orphan list handling */
> > > +	if (!fscrypt_has_encryption_key(inode))
> > > +		return 0;
> > > +
> > >  	blocksize = inode->i_sb->s_blocksize;
> > >  	length = blocksize - (offset & (blocksize - 1));
> > 
> > Shouldn't it be:
> > 
> > 	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode) &&
> > 	    !fscrypt_has_encryption_key(inode))
> > 		return 0;
> > 
> > ... since only encrypted regular files should be skipped?
> 
> We certainly don't want to add the ext4_encrypted_inode() test, since
> this can fail even if -O encrypt is not enabled.  As for checking for
> regular files, we could potentially fall into this code path for
> non-regular files too (symlinks with length greater than 60 bytes come
> to mind), and if we don't have the encryption key, there's no *point*
> to try to zero beyond i_size --- and if we do, we're going to fail
> with a BUG.
> 

Are you sure?  ext4_encrypted_inode() only checks the inode flag; it doesn't
check the superblock flag too.  If we check for just
!fscrypt_has_encryption_key(), the zeroing will get skipped for all
*unencrypted* files too.

Also, my suggestion matches the logic in
__ext4_block_zero_page_range() where the BUG() is actually being hit:

                if (S_ISREG(inode->i_mode) &&
                    ext4_encrypted_inode(inode)) {
                        /* We expect the key to be set. */
                        BUG_ON(!fscrypt_has_encryption_key(inode));
                        BUG_ON(blocksize != PAGE_SIZE);
                        WARN_ON_ONCE(fscrypt_decrypt_page(page->mapping->host,
                                                page, PAGE_SIZE, 0, page->index));
                }


- Eric