| Message ID | 20200208013552.241832-7-drosen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support fof Casefolding and Encryption | expand |
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
On Fri, Feb 07, 2020 at 05:35:50PM -0800, Daniel Rosenberg wrote: > @@ -173,24 +193,24 @@ static inline bool f2fs_match_name(struct f2fs_dentry_ptr *d, > { > #ifdef CONFIG_UNICODE > struct inode *parent = d->inode; > - struct super_block *sb = parent->i_sb; > - struct qstr entry; > + unsigned char *name; > + int len; > #endif > > if (de->hash_code != namehash) > return false; > > #ifdef CONFIG_UNICODE > - entry.name = d->filename[bit_pos]; > - entry.len = de->name_len; > + name = d->filename[bit_pos]; > + len = de->name_len; This is missing le16_to_cpu(). > int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name, > const struct qstr *orig_name, > + f2fs_hash_t dentry_hash, > struct inode *inode, nid_t ino, umode_t mode) > { > unsigned int bit_pos; > unsigned int level; > unsigned int current_depth; > unsigned long bidx, block; > - f2fs_hash_t dentry_hash; > unsigned int nbucket, nblock; > struct page *dentry_page = NULL; > struct f2fs_dentry_block *dentry_blk = NULL; > @@ -632,7 +652,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name, > > level = 0; > slots = GET_DENTRY_SLOTS(new_name->len); > - dentry_hash = f2fs_dentry_hash(dir, new_name, NULL); Why was the call to f2fs_dentry_hash() moved to the caller, but for f2fs_add_inline_entry() a different approach was taken? > @@ -718,17 +737,19 @@ int f2fs_add_dentry(struct inode *dir, struct fscrypt_name *fname, > struct inode *inode, nid_t ino, umode_t mode) > { > struct qstr new_name; > + f2fs_hash_t dentry_hash; > int err = -EAGAIN; > > new_name.name = fname_name(fname); > new_name.len = fname_len(fname); > > if (f2fs_has_inline_dentry(dir)) > - err = f2fs_add_inline_entry(dir, &new_name, fname->usr_fname, > + err = f2fs_add_inline_entry(dir, &new_name, fname, > inode, ino, mode); I'm really confused. Why are you passing around both new_name and fname? We already have new_name == fname.disk_name. So isn't just the 'struct fscrypt_name' sufficient? > +static f2fs_hash_t __f2fs_dentry_hash(const struct inode *dir, > + const struct qstr *name_info, > + const struct fscrypt_name *fname) > { > __u32 hash; > f2fs_hash_t f2fs_hash; > @@ -85,6 +86,11 @@ static f2fs_hash_t __f2fs_dentry_hash(const struct qstr *name_info, > if (is_dot_dotdot(name_info)) > return 0; > > + if (IS_CASEFOLDED(dir) && IS_ENCRYPTED(dir)) { > + f2fs_hash = fscrypt_fname_siphash(dir, name_info); > + return f2fs_hash; > + } This is missing cpu_to_le32(). Also, above we have: /* encrypted bigname case */ if (fname && !fname->disk_name.name) return cpu_to_le32(fname->hash); That won't work with encrypted+casefolded directories without the key, because now sometimes the hash from the no-key name is needed even when the disk_name is available. This will cause a crash in fscrypt_fname_siphash() being called without the key. I think you want: if (fname && fname->is_ciphertext_name) return cpu_to_le32(fname->hash); Can you please write xfstests for encrypt+casefold? - Eric
On Tue, Feb 11, 2020 at 09:10:13PM -0800, Eric Biggers wrote: > 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)) { oops. parent->d_inode is unstable here; could have become NULL by that point. > 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. > */ Really? How would the callers possibly tell mismatch from an error? And if they could, would would they *do* with that error, seeing that it might be an effect of a race with rename()? Again, ->d_compare() is NOT given a stable name. Or *parent. Or (parent->d_inode).
On Wed, Feb 12, 2020 at 05:55:11AM +0000, Al Viro wrote: > On Tue, Feb 11, 2020 at 09:10:13PM -0800, Eric Biggers wrote: > > > 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)) { > > oops. parent->d_inode is unstable here; could have become NULL by that > point. > > > 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. > > */ > > Really? How would the callers possibly tell mismatch from an > error? And if they could, would would they *do* with that > error, seeing that it might be an effect of a race with > rename()? > > Again, ->d_compare() is NOT given a stable name. Or *parent. Or > (parent->d_inode). After the patch earlier in the series that created generic_ci_d_compare() and switched f2fs to use it, f2fs_ci_compare() is only called when the filesystem is actually searching a directory, not from ->d_compare(). So the names and parent->d_inode are stable in it. But, that also means the GFP_ATOMIC isn't needed, and f2fs_ci_compare() should be made 'static'. - Eric
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; } @@ -173,24 +193,24 @@ static inline bool f2fs_match_name(struct f2fs_dentry_ptr *d, { #ifdef CONFIG_UNICODE struct inode *parent = d->inode; - struct super_block *sb = parent->i_sb; - struct qstr entry; + unsigned char *name; + int len; #endif if (de->hash_code != namehash) return false; #ifdef CONFIG_UNICODE - entry.name = d->filename[bit_pos]; - entry.len = de->name_len; + name = d->filename[bit_pos]; + len = de->name_len; - if (sb->s_encoding && IS_CASEFOLDED(parent)) { + if (needs_casefold(parent)) { if (cf_str->name) { struct qstr cf = {.name = cf_str->name, .len = cf_str->len}; - return !f2fs_ci_compare(parent, &cf, &entry, true); + return !f2fs_ci_compare(parent, &cf, name, len, true); } - return !f2fs_ci_compare(parent, fname->usr_fname, &entry, + return !f2fs_ci_compare(parent, fname->usr_fname, name, len, false); } #endif @@ -616,13 +636,13 @@ void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d, int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name, const struct qstr *orig_name, + f2fs_hash_t dentry_hash, struct inode *inode, nid_t ino, umode_t mode) { unsigned int bit_pos; unsigned int level; unsigned int current_depth; unsigned long bidx, block; - f2fs_hash_t dentry_hash; unsigned int nbucket, nblock; struct page *dentry_page = NULL; struct f2fs_dentry_block *dentry_blk = NULL; @@ -632,7 +652,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name, level = 0; slots = GET_DENTRY_SLOTS(new_name->len); - dentry_hash = f2fs_dentry_hash(dir, new_name, NULL); current_depth = F2FS_I(dir)->i_current_depth; if (F2FS_I(dir)->chash == dentry_hash) { @@ -718,17 +737,19 @@ int f2fs_add_dentry(struct inode *dir, struct fscrypt_name *fname, struct inode *inode, nid_t ino, umode_t mode) { struct qstr new_name; + f2fs_hash_t dentry_hash; int err = -EAGAIN; new_name.name = fname_name(fname); new_name.len = fname_len(fname); if (f2fs_has_inline_dentry(dir)) - err = f2fs_add_inline_entry(dir, &new_name, fname->usr_fname, + err = f2fs_add_inline_entry(dir, &new_name, fname, inode, ino, mode); + dentry_hash = f2fs_dentry_hash(dir, &new_name, fname); if (err == -EAGAIN) err = f2fs_add_regular_entry(dir, &new_name, fname->usr_fname, - inode, ino, mode); + dentry_hash, inode, ino, mode); f2fs_update_time(F2FS_I_SB(dir), REQ_TIME); return err; diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 0fc153b5a5c09..ad8e5fb00372f 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -3075,7 +3075,7 @@ struct dentry *f2fs_get_parent(struct dentry *child); extern int f2fs_ci_compare(const struct inode *parent, const struct qstr *name, - const struct qstr *entry, + unsigned char *name2, size_t len, bool quick); /* @@ -3111,7 +3111,7 @@ void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d, const struct qstr *name, f2fs_hash_t name_hash, unsigned int bit_pos); int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name, - const struct qstr *orig_name, + const struct qstr *orig_name, f2fs_hash_t dentry_hash, struct inode *inode, nid_t ino, umode_t mode); int f2fs_add_dentry(struct inode *dir, struct fscrypt_name *fname, struct inode *inode, nid_t ino, umode_t mode); @@ -3144,7 +3144,7 @@ int f2fs_sanity_check_ckpt(struct f2fs_sb_info *sbi); * hash.c */ f2fs_hash_t f2fs_dentry_hash(const struct inode *dir, - const struct qstr *name_info, struct fscrypt_name *fname); + const struct qstr *name_info, const struct fscrypt_name *fname); /* * node.c @@ -3656,7 +3656,7 @@ struct f2fs_dir_entry *f2fs_find_in_inline_dir(struct inode *dir, int f2fs_make_empty_inline_dir(struct inode *inode, struct inode *parent, struct page *ipage); int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name, - const struct qstr *orig_name, + const struct fscrypt_name *fname, struct inode *inode, nid_t ino, umode_t mode); void f2fs_delete_inline_entry(struct f2fs_dir_entry *dentry, struct page *page, struct inode *dir, diff --git a/fs/f2fs/hash.c b/fs/f2fs/hash.c index 28acb24e7a7a8..6d7ddf2fd308f 100644 --- a/fs/f2fs/hash.c +++ b/fs/f2fs/hash.c @@ -68,8 +68,9 @@ static void str2hashbuf(const unsigned char *msg, size_t len, *buf++ = pad; } -static f2fs_hash_t __f2fs_dentry_hash(const struct qstr *name_info, - struct fscrypt_name *fname) +static f2fs_hash_t __f2fs_dentry_hash(const struct inode *dir, + const struct qstr *name_info, + const struct fscrypt_name *fname) { __u32 hash; f2fs_hash_t f2fs_hash; @@ -85,6 +86,11 @@ static f2fs_hash_t __f2fs_dentry_hash(const struct qstr *name_info, if (is_dot_dotdot(name_info)) return 0; + if (IS_CASEFOLDED(dir) && IS_ENCRYPTED(dir)) { + f2fs_hash = fscrypt_fname_siphash(dir, name_info); + return f2fs_hash; + } + /* Initialize the default seed for the hash checksum functions */ buf[0] = 0x67452301; buf[1] = 0xefcdab89; @@ -106,7 +112,7 @@ static f2fs_hash_t __f2fs_dentry_hash(const struct qstr *name_info, } f2fs_hash_t f2fs_dentry_hash(const struct inode *dir, - const struct qstr *name_info, struct fscrypt_name *fname) + const struct qstr *name_info, const struct fscrypt_name *fname) { #ifdef CONFIG_UNICODE struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb); @@ -114,27 +120,30 @@ f2fs_hash_t f2fs_dentry_hash(const struct inode *dir, int r, dlen; unsigned char *buff; struct qstr folded; + const struct qstr *name = fname ? fname->usr_fname : name_info; if (!name_info->len || !IS_CASEFOLDED(dir)) goto opaque_seq; + if (IS_ENCRYPTED(dir) && !fscrypt_has_encryption_key(dir)) + goto opaque_seq; + buff = f2fs_kzalloc(sbi, sizeof(char) * PATH_MAX, GFP_KERNEL); if (!buff) return -ENOMEM; - - dlen = utf8_casefold(um, name_info, buff, PATH_MAX); + dlen = utf8_casefold(um, name, buff, PATH_MAX); if (dlen < 0) { kvfree(buff); goto opaque_seq; } folded.name = buff; folded.len = dlen; - r = __f2fs_dentry_hash(&folded, fname); + r = __f2fs_dentry_hash(dir, &folded, fname); kvfree(buff); return r; opaque_seq: #endif - return __f2fs_dentry_hash(name_info, fname); + return __f2fs_dentry_hash(dir, name_info, fname); } diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index 4167e54081518..4b8fc643df735 100644 --- a/fs/f2fs/inline.c +++ b/fs/f2fs/inline.c @@ -465,8 +465,8 @@ static int f2fs_add_inline_entries(struct inode *dir, void *inline_dentry) ino = le32_to_cpu(de->ino); fake_mode = f2fs_get_de_type(de) << S_SHIFT; - err = f2fs_add_regular_entry(dir, &new_name, NULL, NULL, - ino, fake_mode); + err = f2fs_add_regular_entry(dir, &new_name, NULL, + de->hash_code, NULL, ino, fake_mode); if (err) goto punch_dentry_pages; @@ -578,7 +578,7 @@ int f2fs_try_convert_inline_dir(struct inode *dir, struct dentry *dentry) } int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name, - const struct qstr *orig_name, + const struct fscrypt_name *fname, struct inode *inode, nid_t ino, umode_t mode) { struct f2fs_sb_info *sbi = F2FS_I_SB(dir); @@ -589,6 +589,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name, struct f2fs_dentry_ptr d; int slots = GET_DENTRY_SLOTS(new_name->len); struct page *page = NULL; + const struct qstr *orig_name = fname->usr_fname; int err = 0; ipage = f2fs_get_node_page(sbi, dir->i_ino); @@ -619,7 +620,7 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name, f2fs_wait_on_page_writeback(ipage, NODE, true, true); - name_hash = f2fs_dentry_hash(dir, new_name, NULL); + name_hash = f2fs_dentry_hash(dir, new_name, fname); f2fs_update_dentry(ino, mode, &d, new_name, name_hash, bit_pos); set_page_dirty(ipage); diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 7520a9c04c75e..b30925a44ac71 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -3228,12 +3228,6 @@ static int f2fs_setup_casefold(struct f2fs_sb_info *sbi) struct unicode_map *encoding; __u16 encoding_flags; - if (f2fs_sb_has_encrypt(sbi)) { - f2fs_err(sbi, - "Can't mount with encoding and encryption"); - return -EINVAL; - } - if (f2fs_sb_read_encoding(sbi->raw_super, &encoding_info, &encoding_flags)) { f2fs_err(sbi,
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(-)