linux-ext4 - Re: [PATCH v2 0/5] fscrypt: d_revalidate fixes and cleanups

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: <20190401171119.GC131675@gmail.com>
Date:   Mon, 1 Apr 2019 10:11:21 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     linux-fscrypt@...r.kernel.org, Richard Weinberger <richard@....at>,
        Miklos Szeredi <miklos@...redi.hu>
Cc:     linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-mtd@...ts.infradead.org, linux-unionfs@...r.kernel.org,
        Sarthak Kukreti <sarthakkukreti@...omium.org>,
        Gao Xiang <gaoxiang25@...wei.com>
Subject: Re: [PATCH v2 0/5] fscrypt: d_revalidate fixes and cleanups

On Wed, Mar 20, 2019 at 11:39:08AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.com>
> 
> This patch series improves dentry revalidation in fscrypt.
> 
> To recap, fscrypt (aka ext4/f2fs/ubifs encryption) encrypts both file
> contents and file names in individual directory trees.  A single
> filesystem can contain many encrypted directory trees using many
> different encryption keys.  Major users of fscrypt require the ability
> to delete encrypted files when their encryption key is unavailable, e.g.
> when the system needs to delete a removed user's home directory or free
> up space from a logged-out user's cache directory.
> 
> Therefore fscrypt allows listing, looking up, and deleting files in
> encrypted directories via encoded ciphertext names, but only before the
> key is added.  After the key is added, the ciphertext names are
> invalidated via ->d_revalidate() and plaintext names are shown instead.
> 
> fscrypt isn't a stacked filesystem, and it's explicitly for storage
> encryption, not OS-level access control.  Thus, whether each directory
> inode has its key or not is a global state, not per-process.  Also, the
> inode keeps its key until it's evicted from the inode cache.  So,
> plaintext names shouldn't ever get invalidated by ->d_revalidate().
> 
> This patch series makes the following improvements:
> 
> - Only assign ->d_revalidate() to ciphertext filenames, thus allowing
>   overlayfs to use an fscrypt-encrypted upperdir in some cases.
>   (Previous discussion: https://lkml.org/lkml/2019/3/13/255)
> 
> - Fix cases where plaintext filenames would wrongly be invalidated,
>   including a real-world bug recently reported on Chromium OS.
> 
> - Fix cases where ciphertext filenames would wrongly not be invalidated.
> 
> - Allow rcu-walk lookups in encrypted directories with the key, which
>   improves performance.
>   (Previous attempt: https://patchwork.kernel.org/patch/10594133/)
> 
> - Fix cases where rename() and link() could succeed on ciphertext names.
> 
> Changed since v1:
> 
> - Fixed comment in fscrypt_d_revalidate() to explain that dget_parent()
>   is actually still required.
> 
> - Moved clearing DCACHE_ENCRYPTED_NAME into fscrypt.h, to avoid an extra
>   #ifdef and cluttering up dcache.c.
> 
> Eric Biggers (5):
>   fscrypt: clean up and improve dentry revalidation
>   fscrypt: fix race allowing rename() and link() of ciphertext dentries
>   fs, fscrypt: clear DCACHE_ENCRYPTED_NAME when unaliasing directory
>   fscrypt: only set dentry_operations on ciphertext dentries
>   fscrypt: fix race where ->lookup() marks plaintext dentry as
>     ciphertext
> 
>  fs/crypto/crypto.c      | 58 ++++++++++++++++---------------
>  fs/crypto/fname.c       |  1 +
>  fs/crypto/hooks.c       | 28 ++++++++++-----
>  fs/dcache.c             |  2 ++
>  fs/ext4/ext4.h          | 62 +++++++++++++++++++++++++--------
>  fs/ext4/namei.c         | 76 ++++++++++++++++++++++++++++-------------
>  fs/f2fs/namei.c         | 17 +++++----
>  fs/ubifs/dir.c          |  8 ++---
>  include/linux/dcache.h  |  2 +-
>  include/linux/fscrypt.h | 61 +++++++++++++++++++++++----------
>  10 files changed, 208 insertions(+), 107 deletions(-)
> 
> -- 
> 2.21.0.225.g810b269d1ac-goog
> 

Any more comments on these patches?  I'd like to apply them to the fscrypt tree
for 5.2.  Richard, can you check whether these solve your overlayfs use case?
Also, patch 5 could use Acks from ext4, f2fs, and ubifs people.

- Eric