From patchwork Wed Apr 19 20:44:48 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jaegeuk Kim X-Patchwork-Id: 752457 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3w7YsW3hFpz9s7B for ; Thu, 20 Apr 2017 06:45:03 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1766009AbdDSUpB (ORCPT ); Wed, 19 Apr 2017 16:45:01 -0400 Received: from mail.kernel.org ([198.145.29.136]:51316 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1765977AbdDSUpB (ORCPT ); Wed, 19 Apr 2017 16:45:01 -0400 Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 318F4201F2; Wed, 19 Apr 2017 20:44:59 +0000 (UTC) Received: from localhost (107-1-141-74-ip-static.hfc.comcastbusiness.net [107.1.141.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id C3116201B9; Wed, 19 Apr 2017 20:44:56 +0000 (UTC) Date: Wed, 19 Apr 2017 13:44:48 -0700 From: Jaegeuk Kim To: Eric Biggers Cc: Gwendal Grignou , hashimoto@chromium.org, ebiggers@google.com, linux-f2fs-devel@lists.sourceforge.net, linux-fscrypt@vger.kernel.org, linux-mtd@lists.infradead.org, tytso@mit.edu, linux-ext4@vger.kernel.org, kinaba@chromium.org Subject: Re: [f2fs-dev] [PATCH] fscrypt: use 32 bytes of encrypted filename Message-ID: <20170419204448.GA1021@jaegeuk.local> References: <20170418210642.6039-1-gwendal@chromium.org> <20170418230136.GA96152@gmail.com> <20170419001005.GA143911@gmail.com> <20170419014209.GB12215@jaegeuk.local> <20170419040138.GA563@zzz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170419040138.GA563@zzz> User-Agent: Mutt/1.7.0 (2016-08-17) X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, T_FILL_THIS_FORM_SHORT, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On 04/18, Eric Biggers wrote: > On Tue, Apr 18, 2017 at 06:42:09PM -0700, Jaegeuk Kim wrote: > > Hi Eric, > > > > On 04/18, Eric Biggers wrote: > > > On Tue, Apr 18, 2017 at 04:01:36PM -0700, Eric Biggers wrote: > > > > > > > > Strangely, f2fs and ubifs do not use the bytes from the filename at all when > > > > trying to find a specific directory entry in this case. So this patch doesn't > > > > really affect them. This seems unreliable; perhaps we should introduce a > > > > function like "fscrypt_name_matches()" which all the filesystems could call? > > > > Can any of the f2fs and ubifs developers explain why they don't look at any > > > > bytes from the filename? > > > > > > > > The fscrypt_setup_filename sets fname->hash in the bigname case, but doesn't > > give fname->disk_name. If it's not such the bigname case, we check its name > > since fname->hash is zero. > > > > Yes, that's what it does now. The question is, in the "bigname" case why > doesn't f2fs check the 16 bytes of ciphertext in fname->crypto_buf too? f2fs > doesn't even use 'minor_hash'; it can't possibly be the case that there are > never any collisions of a 32-bit hash in a directory, can it? > > I actually tested it, and it definitely happens if you put a lot of files in an > encrypted directory on f2fs. An example with 100000 files: > > # seq -f "edir/abcdefghijklmnopqrstuvwxyz012345%.0f" 100000 | xargs touch > # find edir -type f | xargs stat -c %i | sort | uniq | wc -l > 100000 > # sync > # echo 3 > /proc/sys/vm/drop_caches > # keyctl new_session > # find edir -type f | xargs stat -c %i | sort | uniq | wc -l > 99999 > > So when I tried accessing the encrypted directory without the key, two dentries > showed the same inode, due to a hash collision. Thank you for sharing more details. I could reproduce this issue and reach out to what you mentioned. In order to resolve this, I wrote a patch for f2fs first to act like ext4 for easy backports. Then, I expect a global fscrypt function is easily able to clean them up. Thanks, From 63ca24a64fb1625dac9740a2183fd8966388e185 Mon Sep 17 00:00:00 2001 From: Jaegeuk Kim Date: Wed, 19 Apr 2017 10:49:21 -0700 Subject: [PATCH] f2fs: check entire encrypted bigname when finding a dentry If user has no key under an encrypted dir, fscrypt gives digested dentries. Previously, when looking up a dentry, f2fs only checks its hash value with first 4 bytes of the digested dentry, which didn't handle hash collisions fully. This patch enhances to check entire dentry bytes likewise ext4. Eric reported how to reproduce this issue by: # seq -f "edir/abcdefghijklmnopqrstuvwxyz012345%.0f" 100000 | xargs touch # find edir -type f | xargs stat -c %i | sort | uniq | wc -l 100000 # sync # echo 3 > /proc/sys/vm/drop_caches # keyctl new_session # find edir -type f | xargs stat -c %i | sort | uniq | wc -l 99999 Cc: Reported-by: Eric Biggers Signed-off-by: Jaegeuk Kim --- fs/f2fs/dir.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index c143dffcae6e..007c3b4adc85 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -130,19 +130,29 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, continue; } - /* encrypted case */ + if (de->hash_code != namehash) + goto not_match; + de_name.name = d->filename[bit_pos]; de_name.len = le16_to_cpu(de->name_len); - /* show encrypted name */ - if (fname->hash) { - if (de->hash_code == cpu_to_le32(fname->hash)) - goto found; - } else if (de_name.len == name->len && - de->hash_code == namehash && - !memcmp(de_name.name, name->name, name->len)) +#ifdef CONFIG_F2FS_FS_ENCRYPTION + if (unlikely(!name->name)) { + if (fname->usr_fname->name[0] == '_') { + if (de_name.len >= 16 && + !memcmp(de_name.name + de_name.len - 16, + fname->crypto_buf.name + 8, 16)) + goto found; + goto not_match; + } + name->name = fname->crypto_buf.name; + name->len = fname->crypto_buf.len; + } +#endif + if (de_name.len == name->len && + !memcmp(de_name.name, name->name, name->len)) goto found; - +not_match: if (max_slots && max_len > *max_slots) *max_slots = max_len; max_len = 0;