mbox series

[0/2] ceph: fscrypt: fix atomic open bug for encrypted directories

Message ID 20230313123310.13040-1-lhenriques@suse.de
Headers
Series ceph: fscrypt: fix atomic open bug for encrypted directories |

Message

Luis Henriques March 13, 2023, 12:33 p.m. UTC 
  Hi!

I started seeing fstest generic/123 failing in ceph fscrypt, when running it
with 'test_dummy_encryption'.  This test is quite simple:

1. Creates a directory with write permissions for root only
2. Writes into a file in that directory
3. Uses 'su' to try to modify that file as a different user, and
   gets -EPERM

All the test steps succeed, but the test fails to cleanup: 'rm -rf <dir>'
will fail with -ENOTEMPTY.  'strace' shows that calling unlinkat() to remove
the file got a -ENOENT and then -ENOTEMPTY for the directory.

This is because 'su' does a drop_caches ('su (874): drop_caches: 2' in
dmesg), and ceph's atomic open will do:

	if (IS_ENCRYPTED(dir)) {
		set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
		if (!fscrypt_has_encryption_key(dir)) {
			spin_lock(&dentry->d_lock);
			dentry->d_flags |= DCACHE_NOKEY_NAME;
			spin_unlock(&dentry->d_lock);
		}
	}

Although 'dir' has the encryption key available, fscrypt_has_encryption_key()
will return 'false' because fscrypt info isn't yet set after the cache
cleanup.

The first patch will add a new helper for the atomic_open that will force
the fscrypt info to be loaded into an inode that has been evicted recently
but for which the key is still available.

The second patch switches ceph atomic_open to use the new fscrypt helper.

Cheers,
--
Luís

Changes since initial RFC (after Eric's review):
- Added kerneldoc comments to the new fscrypt helper
- Dropped '__' from helper name (now fscrypt_prepare_atomic_open())
- Added IS_ENCRYPTED() check in helper
- DCACHE_NOKEY_NAME is not set if fscrypt_get_encryption_info() returns an
  error
- Fixed helper for !CONFIG_FS_ENCRYPTION (now defined 'static inline')

Luís Henriques (2):
  fscrypt: new helper function - fscrypt_prepare_atomic_open()
  ceph: switch atomic open to use new fscrypt helper

 fs/ceph/file.c          |  8 +++-----
 fs/crypto/hooks.c       | 35 +++++++++++++++++++++++++++++++++++
 include/linux/fscrypt.h |  7 +++++++
 3 files changed, 45 insertions(+), 5 deletions(-)

Comments

Jeff Layton March 13, 2023, 5:11 p.m. UTC | #1 
On Mon, 2023-03-13 at 12:33 +0000, Luís Henriques wrote:
> Hi!
> 
> I started seeing fstest generic/123 failing in ceph fscrypt, when running it
> with 'test_dummy_encryption'.  This test is quite simple:
> 
> 1. Creates a directory with write permissions for root only
> 2. Writes into a file in that directory
> 3. Uses 'su' to try to modify that file as a different user, and
>    gets -EPERM
> 
> All the test steps succeed, but the test fails to cleanup: 'rm -rf <dir>'
> will fail with -ENOTEMPTY.  'strace' shows that calling unlinkat() to remove
> the file got a -ENOENT and then -ENOTEMPTY for the directory.
> 
> This is because 'su' does a drop_caches ('su (874): drop_caches: 2' in
> dmesg), and ceph's atomic open will do:
> 
> 	if (IS_ENCRYPTED(dir)) {
> 		set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
> 		if (!fscrypt_has_encryption_key(dir)) {
> 			spin_lock(&dentry->d_lock);
> 			dentry->d_flags |= DCACHE_NOKEY_NAME;
> 			spin_unlock(&dentry->d_lock);
> 		}
> 	}
> 
> Although 'dir' has the encryption key available, fscrypt_has_encryption_key()
> will return 'false' because fscrypt info isn't yet set after the cache
> cleanup.
> 
> The first patch will add a new helper for the atomic_open that will force
> the fscrypt info to be loaded into an inode that has been evicted recently
> but for which the key is still available.
> 
> The second patch switches ceph atomic_open to use the new fscrypt helper.
> 
> Cheers,
> --
> Luís
> 
> Changes since initial RFC (after Eric's review):
> - Added kerneldoc comments to the new fscrypt helper
> - Dropped '__' from helper name (now fscrypt_prepare_atomic_open())
> - Added IS_ENCRYPTED() check in helper
> - DCACHE_NOKEY_NAME is not set if fscrypt_get_encryption_info() returns an
>   error
> - Fixed helper for !CONFIG_FS_ENCRYPTION (now defined 'static inline')
> 
> Luís Henriques (2):
>   fscrypt: new helper function - fscrypt_prepare_atomic_open()
>   ceph: switch atomic open to use new fscrypt helper
> 
>  fs/ceph/file.c          |  8 +++-----
>  fs/crypto/hooks.c       | 35 +++++++++++++++++++++++++++++++++++
>  include/linux/fscrypt.h |  7 +++++++
>  3 files changed, 45 insertions(+), 5 deletions(-)
> 

Looks like a nice cleanup.

Reviewed-by: Jeff Layton <jlayton@kernel.org>