Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Eric Biggers <ebiggers@kernel.org>
To: Daniel Rosenberg <drosen@google.com>
Cc: Theodore Ts'o <tytso@mit.edu>,
	linux-ext4@vger.kernel.org, Jaegeuk Kim <jaegeuk@kernel.org>,
	Chao Yu <chao@kernel.org>,
	linux-f2fs-devel@lists.sourceforge.net,
	linux-fscrypt@vger.kernel.org,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org,
	Gabriel Krisman Bertazi <krisman@collabora.com>,
	kernel-team@android.com
Subject: Re: [PATCH v7 6/8] f2fs: Handle casefolding with Encryption
Date: Tue, 11 Feb 2020 21:10:13 -0800
Message-ID: <20200212051013.GG870@sol.localdomain> (raw)
In-Reply-To: <20200208013552.241832-7-drosen@google.com>

On Fri, Feb 07, 2020 at 05:35:50PM -0800, Daniel Rosenberg wrote:
> This expands f2fs's casefolding support to include encrypted
> directories. For encrypted directories, we use the siphash of the
> casefolded name. This ensures there is no direct way to go from an
> unencrypted name to the stored hash on disk without knowledge of the
> encryption policy keys.
> 
> Additionally, we switch to using the vfs layer's casefolding support
> instead of storing this information inside of f2fs's private data.
> 
> Signed-off-by: Daniel Rosenberg <drosen@google.com>
> ---
>  fs/f2fs/dir.c    | 65 ++++++++++++++++++++++++++++++++----------------
>  fs/f2fs/f2fs.h   |  8 +++---
>  fs/f2fs/hash.c   | 23 +++++++++++------
>  fs/f2fs/inline.c |  9 ++++---
>  fs/f2fs/super.c  |  6 -----
>  5 files changed, 68 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 38c0e6d589be4..3517dd4060341 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -112,30 +112,50 @@ static struct f2fs_dir_entry *find_in_block(struct inode *dir,
>   * doesn't match or less than zero on error.
>   */
>  int f2fs_ci_compare(const struct inode *parent, const struct qstr *name,
> -				const struct qstr *entry, bool quick)
> +		    unsigned char *name2, size_t len, bool quick)
>  {
>  	const struct super_block *sb = parent->i_sb;
>  	const struct unicode_map *um = sb->s_encoding;
> +	const struct fscrypt_str crypt_entry = FSTR_INIT(name2, len);
> +	struct fscrypt_str decrypted_entry;
> +	struct qstr decrypted;
> +	struct qstr entry = QSTR_INIT(name2, len);
> +	struct qstr *tocheck;
>  	int ret;
>  
> +	decrypted_entry.name = NULL;
> +
> +	if (IS_ENCRYPTED(parent) && fscrypt_has_encryption_key(parent)) {
> +		decrypted_entry.name = kmalloc(len, GFP_ATOMIC);
> +		decrypted.name = decrypted_entry.name;
> +		decrypted_entry.len = len;
> +		decrypted.len = len;
> +		if (!decrypted.name)
> +			return -ENOMEM;
> +		ret = fscrypt_fname_disk_to_usr(parent, 0, 0, &crypt_entry,
> +							&decrypted_entry);
> +		if (ret < 0)
> +			goto out;
> +	}
> +	tocheck = decrypted_entry.name ? &decrypted : &entry;
> +
>  	if (quick)
> -		ret = utf8_strncasecmp_folded(um, name, entry);
> +		ret = utf8_strncasecmp_folded(um, name, tocheck);
>  	else
> -		ret = utf8_strncasecmp(um, name, entry);
> -
> +		ret = utf8_strncasecmp(um, name, tocheck);
>  	if (ret < 0) {
>  		/* Handle invalid character sequence as either an error
>  		 * or as an opaque byte sequence.
>  		 */
>  		if (sb_has_enc_strict_mode(sb))
> -			return -EINVAL;
> -
> -		if (name->len != entry->len)
> -			return 1;
> -
> -		return !!memcmp(name->name, entry->name, name->len);
> +			ret = -EINVAL;
> +		else if (name->len != len)
> +			ret = 1;
> +		else
> +			ret = !!memcmp(name->name, tocheck->name, len);
>  	}
> -
> +out:
> +	kfree(decrypted_entry.name);
>  	return ret;
>  }

The case-sensitive fallback is broken with encrypted filenames; it's checking
the length of the encrypted filename rather than the decrypted filename.  The
decrypted name may be shorter.

Can you please improve your testing to catch bugs like this?

IMO, part of the problem is that there are multiple lengths here, so the
variable named 'len' is ambiguous.  Can you please clean this function up to
name things properly?  Also, the 'tocheck' variable is unnecessary, and it's
confusing having both 'decrypted' and 'decrypted_entry', and to decrypt
conditionally when fscrypt_has_encryption_key() since that's already required.

How about:

int f2fs_ci_compare(const struct inode *parent, const struct qstr *name,
		    u8 *de_name, size_t de_name_len, bool quick)
{
	const struct super_block *sb = parent->i_sb;
	const struct unicode_map *um = sb->s_encoding;
	struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
	struct qstr entry = QSTR_INIT(de_name, de_name_len);
	int ret;

	if (IS_ENCRYPTED(parent)) {
		const struct fscrypt_str encrypted_name =
			FSTR_INIT(de_name, de_name_len);

		decrypted_name.name = kmalloc(de_name_len, GFP_ATOMIC);
		if (!decrypted_name.name)
			return -ENOMEM;
		ret = fscrypt_fname_disk_to_usr(parent, 0, 0, &encrypted_name,
						&decrypted_name);
		if (ret < 0)
			goto out;
		entry.name = decrypted_name.name;
		entry.len = decrypted_name.len;
	}

	if (quick)
		ret = utf8_strncasecmp_folded(um, name, &entry);
	else
		ret = utf8_strncasecmp(um, name, &entry);
	if (ret < 0) {
		/* Handle invalid character sequence as either an error
		 * or as an opaque byte sequence.
		 */
		if (sb_has_enc_strict_mode(sb))
			ret = -EINVAL;
		else if (name->len != entry.len)
			ret = 1;
		else
			ret = !!memcmp(name->name, entry.name, entry.len);
	}
out:
	kfree(decrypted_name.name);
	return ret;
}


Of course, all this applies to ext4_ci_compare() as well.

- Eric

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-08  1:35 [PATCH v7 0/8] Support fof Casefolding and Encryption Daniel Rosenberg
2020-02-08  1:35 ` [PATCH v7 1/8] unicode: Add utf8_casefold_iter Daniel Rosenberg
2020-02-12  3:38   ` Eric Biggers
2020-02-14 21:47     ` Daniel Rosenberg
2020-02-17 19:02       ` Gabriel Krisman Bertazi
2020-02-08  1:35 ` [PATCH v7 2/8] fs: Add standard casefolding support Daniel Rosenberg
2020-02-08  2:12   ` Al Viro
2020-02-10 23:11     ` Daniel Rosenberg
2020-02-10 23:42       ` Al Viro
2020-02-12  6:34         ` Eric Biggers
2020-02-12  6:57           ` Eric Biggers
2020-02-20  2:27             ` Daniel Rosenberg
2020-02-12  3:55   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 3/8] f2fs: Use generic " Daniel Rosenberg
2020-02-12  4:05   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 4/8] ext4: " Daniel Rosenberg
2020-02-08  1:35 ` [PATCH v7 5/8] fscrypt: Have filesystems handle their d_ops Daniel Rosenberg
2020-02-12  4:33   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 6/8] f2fs: Handle casefolding with Encryption Daniel Rosenberg
2020-02-12  5:10   ` Eric Biggers [this message]
2020-02-12  5:55     ` Al Viro
2020-02-12  6:06       ` Eric Biggers
2020-02-12  5:47   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 7/8] ext4: Hande casefolding with encryption Daniel Rosenberg
2020-02-12  5:59   ` Eric Biggers
2020-02-08  1:35 ` [PATCH v7 8/8] ext4: Optimize match for casefolded encrypted dirs Daniel Rosenberg
2020-02-12  6:12 ` [PATCH v7 0/8] Support fof Casefolding and Encryption Eric Biggers
2020-02-13  0:01   ` Daniel Rosenberg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200212051013.GG870@sol.localdomain \
    --to=ebiggers@kernel.org \
    --cc=adilger.kernel@dilger.ca \
    --cc=chao@kernel.org \
    --cc=corbet@lwn.net \
    --cc=drosen@google.com \
    --cc=jaegeuk@kernel.org \
    --cc=kernel-team@android.com \
    --cc=krisman@collabora.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=richard@nod.at \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/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-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

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


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