Linux-FSCrypt Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] fscrypt: don't print name of busy file when removing key
@ 2020-01-20  6:07 Eric Biggers
  2020-01-22 22:59 ` Eric Biggers
  0 siblings, 1 reply; 2+ messages in thread
From: Eric Biggers @ 2020-01-20  6:07 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: linux-ext4, linux-f2fs-devel

From: Eric Biggers <ebiggers@google.com>

When an encryption key can't be fully removed due to file(s) protected
by it still being in-use, we shouldn't really print the path to one of
these files to the kernel log, since parts of this path are likely to be
encrypted on-disk, and (depending on how the system is set up) the
confidentiality of this path might be lost by printing it to the log.

This is a trade-off: a single file path often doesn't matter at all,
especially if it's a directory; the kernel log might still be protected
in some way; and I had originally hoped that any "inode(s) still busy"
bugs (which are security weaknesses in their own right) would be quickly
fixed and that to do so it would be super helpful to always know the
file path and not have to run 'find dir -inum $inum' after the fact.

But in practice, these bugs can be hard to fix (e.g. due to asynchronous
process killing that is difficult to eliminate, for performance
reasons), and also not tied to specific files, so knowing a file path
doesn't necessarily help.

So to be safe, for now let's just show the inode number, not the path.
If someone really wants to know a path they can use 'find -inum'.

Fixes: b1c0ec3599f4 ("fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl")
Cc: <stable@vger.kernel.org> # v5.4+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/keyring.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index 098ff2e0f0bb4..ab41b25d4fa1b 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -776,9 +776,6 @@ static int check_for_busy_inodes(struct super_block *sb,
 	struct list_head *pos;
 	size_t busy_count = 0;
 	unsigned long ino;
-	struct dentry *dentry;
-	char _path[256];
-	char *path = NULL;
 
 	spin_lock(&mk->mk_decrypted_inodes_lock);
 
@@ -797,22 +794,14 @@ static int check_for_busy_inodes(struct super_block *sb,
 					 struct fscrypt_info,
 					 ci_master_key_link)->ci_inode;
 		ino = inode->i_ino;
-		dentry = d_find_alias(inode);
 	}
 	spin_unlock(&mk->mk_decrypted_inodes_lock);
 
-	if (dentry) {
-		path = dentry_path(dentry, _path, sizeof(_path));
-		dput(dentry);
-	}
-	if (IS_ERR_OR_NULL(path))
-		path = "(unknown)";
-
 	fscrypt_warn(NULL,
-		     "%s: %zu inode(s) still busy after removing key with %s %*phN, including ino %lu (%s)",
+		     "%s: %zu inode(s) still busy after removing key with %s %*phN, including ino %lu",
 		     sb->s_id, busy_count, master_key_spec_type(&mk->mk_spec),
 		     master_key_spec_len(&mk->mk_spec), (u8 *)&mk->mk_spec.u,
-		     ino, path);
+		     ino);
 	return -EBUSY;
 }
 
-- 
2.25.0


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] fscrypt: don't print name of busy file when removing key
  2020-01-20  6:07 [PATCH] fscrypt: don't print name of busy file when removing key Eric Biggers
@ 2020-01-22 22:59 ` Eric Biggers
  0 siblings, 0 replies; 2+ messages in thread
From: Eric Biggers @ 2020-01-22 22:59 UTC (permalink / raw)
  To: linux-fscrypt; +Cc: linux-ext4, linux-f2fs-devel

On Sun, Jan 19, 2020 at 10:07:32PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> When an encryption key can't be fully removed due to file(s) protected
> by it still being in-use, we shouldn't really print the path to one of
> these files to the kernel log, since parts of this path are likely to be
> encrypted on-disk, and (depending on how the system is set up) the
> confidentiality of this path might be lost by printing it to the log.
> 
> This is a trade-off: a single file path often doesn't matter at all,
> especially if it's a directory; the kernel log might still be protected
> in some way; and I had originally hoped that any "inode(s) still busy"
> bugs (which are security weaknesses in their own right) would be quickly
> fixed and that to do so it would be super helpful to always know the
> file path and not have to run 'find dir -inum $inum' after the fact.
> 
> But in practice, these bugs can be hard to fix (e.g. due to asynchronous
> process killing that is difficult to eliminate, for performance
> reasons), and also not tied to specific files, so knowing a file path
> doesn't necessarily help.
> 
> So to be safe, for now let's just show the inode number, not the path.
> If someone really wants to know a path they can use 'find -inum'.
> 
> Fixes: b1c0ec3599f4 ("fscrypt: add FS_IOC_REMOVE_ENCRYPTION_KEY ioctl")
> Cc: <stable@vger.kernel.org> # v5.4+
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Applied to fscrypt.git#master for 5.6.

- Eric

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20  6:07 [PATCH] fscrypt: don't print name of busy file when removing key Eric Biggers
2020-01-22 22:59 ` Eric Biggers

Linux-FSCrypt Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lore.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git