| Message ID | 20200914191707.380444-15-jlayton@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Sep 14, 2020 at 03:17:05PM -0400, Jeff Layton wrote: > Add helper functions for buffer management and for decrypting filenames > returned by the MDS. Wire those into the readdir codepaths. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/crypto.c | 47 +++++++++++++++++++++++++++++++++++++++ > fs/ceph/crypto.h | 35 +++++++++++++++++++++++++++++ > fs/ceph/dir.c | 58 +++++++++++++++++++++++++++++++++++++++--------- > fs/ceph/inode.c | 31 +++++++++++++++++++++++--- > 4 files changed, 157 insertions(+), 14 deletions(-) > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c > index f037a4939026..e3038c88c7a0 100644 > --- a/fs/ceph/crypto.c > +++ b/fs/ceph/crypto.c > @@ -107,3 +107,50 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, > ceph_pagelist_release(pagelist); > return ret; > } > + > +int ceph_fname_to_usr(struct inode *parent, char *name, u32 len, > + struct fscrypt_str *tname, struct fscrypt_str *oname, > + bool *is_nokey) > +{ > + int ret, declen; > + u32 save_len; > + struct fscrypt_str myname = FSTR_INIT(NULL, 0); > + > + if (!IS_ENCRYPTED(parent)) { > + oname->name = name; > + oname->len = len; > + return 0; > + } > + > + ret = fscrypt_get_encryption_info(parent); > + if (ret) > + return ret; > + > + if (tname) { > + save_len = tname->len; > + } else { > + int err; > + > + save_len = 0; > + err = fscrypt_fname_alloc_buffer(NAME_MAX, &myname); > + if (err) > + return err; > + tname = &myname; The 'err' variable isn't needed, since 'ret' can be used instead. > + } > + > + declen = fscrypt_base64_decode(name, len, tname->name); > + if (declen < 0 || declen > NAME_MAX) { > + ret = -EIO; > + goto out; > + } declen <= 0, to cover the empty name case. Also, is there a point in checking for > NAME_MAX? > + > + tname->len = declen; > + > + ret = fscrypt_fname_disk_to_usr(parent, 0, 0, tname, oname, is_nokey); > + > + if (save_len) > + tname->len = save_len; This logic for temporarily overwriting the length is weird. How about something like the following instead: int ceph_fname_to_usr(struct inode *parent, char *name, u32 len, struct fscrypt_str *tname, struct fscrypt_str *oname, bool *is_nokey) { int err, declen; struct fscrypt_str _tname = FSTR_INIT(NULL, 0); struct fscrypt_str iname; if (!IS_ENCRYPTED(parent)) { oname->name = name; oname->len = len; return 0; } err = fscrypt_get_encryption_info(parent); if (err) return err; if (!tname) { err = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname); if (err) return err; tname = &_tname; } declen = fscrypt_base64_decode(name, len, tname->name); if (declen <= 0) { err = -EIO; goto out; } iname.name = tname->name; iname.len = declen; err = fscrypt_fname_disk_to_usr(parent, 0, 0, &iname, oname, is_nokey); out: fscrypt_fname_free_buffer(&_tname); return err; }
On Mon, 2020-09-14 at 18:57 -0700, Eric Biggers wrote: > On Mon, Sep 14, 2020 at 03:17:05PM -0400, Jeff Layton wrote: > > Add helper functions for buffer management and for decrypting filenames > > returned by the MDS. Wire those into the readdir codepaths. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/crypto.c | 47 +++++++++++++++++++++++++++++++++++++++ > > fs/ceph/crypto.h | 35 +++++++++++++++++++++++++++++ > > fs/ceph/dir.c | 58 +++++++++++++++++++++++++++++++++++++++--------- > > fs/ceph/inode.c | 31 +++++++++++++++++++++++--- > > 4 files changed, 157 insertions(+), 14 deletions(-) > > > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c > > index f037a4939026..e3038c88c7a0 100644 > > --- a/fs/ceph/crypto.c > > +++ b/fs/ceph/crypto.c > > @@ -107,3 +107,50 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, > > ceph_pagelist_release(pagelist); > > return ret; > > } > > + > > +int ceph_fname_to_usr(struct inode *parent, char *name, u32 len, > > + struct fscrypt_str *tname, struct fscrypt_str *oname, > > + bool *is_nokey) > > +{ > > + int ret, declen; > > + u32 save_len; > > + struct fscrypt_str myname = FSTR_INIT(NULL, 0); > > + > > + if (!IS_ENCRYPTED(parent)) { > > + oname->name = name; > > + oname->len = len; > > + return 0; > > + } > > + > > + ret = fscrypt_get_encryption_info(parent); > > + if (ret) > > + return ret; > > + > > + if (tname) { > > + save_len = tname->len; > > + } else { > > + int err; > > + > > + save_len = 0; > > + err = fscrypt_fname_alloc_buffer(NAME_MAX, &myname); > > + if (err) > > + return err; > > + tname = &myname; > > The 'err' variable isn't needed, since 'ret' can be used instead. > > > + } > > + > > + declen = fscrypt_base64_decode(name, len, tname->name); > > + if (declen < 0 || declen > NAME_MAX) { > > + ret = -EIO; > > + goto out; > > + } > > declen <= 0, to cover the empty name case. > > Also, is there a point in checking for > NAME_MAX? > IDK. We're getting these strings from the MDS and they could end up being corrupt if there are bugs there (or if the MDS is compromised). Of course, if we get a name longer than NAME_MAX then we've overrun the buffer. Maybe we should add a maxlen parameter to fscrypt_base64_encode/decode ? Or maybe I should just have fscrypt_fname_alloc_buffer allocate a buffer the same size as "len"? It might be a little larger than necessary, but that would be safer. > > + > > + tname->len = declen; > > + > > + ret = fscrypt_fname_disk_to_usr(parent, 0, 0, tname, oname, is_nokey); > > + > > + if (save_len) > > + tname->len = save_len; > > This logic for temporarily overwriting the length is weird. > How about something like the following instead: > Yeah, it is odd. I think I got spooked by the way that length in struct fscrypt_str is handled. Some functions treat it as representing the length of the allocated buffer (e.g. fscrypt_fname_alloc_buffer), but others treat it as representing the length of the string in ->name (e.g. fscrypt_encode_nokey_name). Your suggestion works around that though, so I'll probably adopt something like it. Thanks! > int ceph_fname_to_usr(struct inode *parent, char *name, u32 len, > struct fscrypt_str *tname, struct fscrypt_str *oname, > bool *is_nokey) > { > int err, declen; > struct fscrypt_str _tname = FSTR_INIT(NULL, 0); > struct fscrypt_str iname; > > if (!IS_ENCRYPTED(parent)) { > oname->name = name; > oname->len = len; > return 0; > } > > err = fscrypt_get_encryption_info(parent); > if (err) > return err; > > if (!tname) { > err = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname); > if (err) > return err; > tname = &_tname; > } > > declen = fscrypt_base64_decode(name, len, tname->name); > if (declen <= 0) { > err = -EIO; > goto out; > } > > iname.name = tname->name; > iname.len = declen; > err = fscrypt_fname_disk_to_usr(parent, 0, 0, &iname, oname, is_nokey); > out: > fscrypt_fname_free_buffer(&_tname); > return err; > } -- Jeff Layton <jlayton@kernel.org>
On Tue, Sep 15, 2020 at 09:27:49AM -0400, Jeff Layton wrote: > > > + } > > > + > > > + declen = fscrypt_base64_decode(name, len, tname->name); > > > + if (declen < 0 || declen > NAME_MAX) { > > > + ret = -EIO; > > > + goto out; > > > + } > > > > declen <= 0, to cover the empty name case. > > > > Also, is there a point in checking for > NAME_MAX? > > > > IDK. We're getting these strings from the MDS and they could end up > being corrupt if there are bugs there (or if the MDS is compromised). > Of course, if we get a name longer than NAME_MAX then we've overrun the > buffer. > > Maybe we should add a maxlen parameter to fscrypt_base64_encode/decode ? > Or maybe I should just have fscrypt_fname_alloc_buffer allocate a buffer > the same size as "len"? It might be a little larger than necessary, but > that would be safer. How about checking that the base64-encoded filename is <= BASE64_CHARS(NAME_MAX) in length? Then decoding it can't give more than NAME_MAX bytes. fscrypt_setup_filename() does a similar check when decoding a no-key name. - Eric
On Tue, 2020-09-15 at 13:40 -0700, Eric Biggers wrote: > On Tue, Sep 15, 2020 at 09:27:49AM -0400, Jeff Layton wrote: > > > > + } > > > > + > > > > + declen = fscrypt_base64_decode(name, len, tname->name); > > > > + if (declen < 0 || declen > NAME_MAX) { > > > > + ret = -EIO; > > > > + goto out; > > > > + } > > > > > > declen <= 0, to cover the empty name case. > > > > > > Also, is there a point in checking for > NAME_MAX? > > > > > > > IDK. We're getting these strings from the MDS and they could end up > > being corrupt if there are bugs there (or if the MDS is compromised). > > Of course, if we get a name longer than NAME_MAX then we've overrun the > > buffer. > > > > Maybe we should add a maxlen parameter to fscrypt_base64_encode/decode ? > > Or maybe I should just have fscrypt_fname_alloc_buffer allocate a buffer > > the same size as "len"? It might be a little larger than necessary, but > > that would be safer. > > How about checking that the base64-encoded filename is <= BASE64_CHARS(NAME_MAX) > in length? Then decoding it can't give more than NAME_MAX bytes. > > fscrypt_setup_filename() does a similar check when decoding a no-key name. > Good idea. I'll roll that in. Thanks, -- Jeff Layton <jlayton@kernel.org>
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c index f037a4939026..e3038c88c7a0 100644 --- a/fs/ceph/crypto.c +++ b/fs/ceph/crypto.c @@ -107,3 +107,50 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, ceph_pagelist_release(pagelist); return ret; } + +int ceph_fname_to_usr(struct inode *parent, char *name, u32 len, + struct fscrypt_str *tname, struct fscrypt_str *oname, + bool *is_nokey) +{ + int ret, declen; + u32 save_len; + struct fscrypt_str myname = FSTR_INIT(NULL, 0); + + if (!IS_ENCRYPTED(parent)) { + oname->name = name; + oname->len = len; + return 0; + } + + ret = fscrypt_get_encryption_info(parent); + if (ret) + return ret; + + if (tname) { + save_len = tname->len; + } else { + int err; + + save_len = 0; + err = fscrypt_fname_alloc_buffer(NAME_MAX, &myname); + if (err) + return err; + tname = &myname; + } + + declen = fscrypt_base64_decode(name, len, tname->name); + if (declen < 0 || declen > NAME_MAX) { + ret = -EIO; + goto out; + } + + tname->len = declen; + + ret = fscrypt_fname_disk_to_usr(parent, 0, 0, tname, oname, is_nokey); + + if (save_len) + tname->len = save_len; +out: + fscrypt_fname_free_buffer(&myname); + return ret; +} diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h index c1b6ec4b2961..df1a093848bb 100644 --- a/fs/ceph/crypto.h +++ b/fs/ceph/crypto.h @@ -6,6 +6,8 @@ #ifndef _CEPH_CRYPTO_H #define _CEPH_CRYPTO_H +#include <linux/fscrypt.h> + #ifdef CONFIG_FS_ENCRYPTION #define CEPH_XATTR_NAME_ENCRYPTION_CONTEXT "encryption.ctx" @@ -14,6 +16,22 @@ void ceph_fscrypt_set_ops(struct super_block *sb); int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode, struct ceph_acl_sec_ctx *as); +static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname) +{ + if (!IS_ENCRYPTED(parent)) + return 0; + return fscrypt_fname_alloc_buffer(NAME_MAX, fname); +} + +static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname) +{ + if (IS_ENCRYPTED(parent)) + fscrypt_fname_free_buffer(fname); +} + +int ceph_fname_to_usr(struct inode *parent, char *name, u32 len, struct fscrypt_str *tname, + struct fscrypt_str *oname, bool *is_nokey); + #else /* CONFIG_FS_ENCRYPTION */ static inline int ceph_fscrypt_set_ops(struct super_block *sb) @@ -27,6 +45,23 @@ static inline int ceph_fscrypt_prepare_context(struct inode *dir, struct inode * return 0; } +static inline int ceph_fname_alloc_buffer(struct inode *parent, struct fscrypt_str *fname) +{ + return 0; +} + +static inline void ceph_fname_free_buffer(struct inode *parent, struct fscrypt_str *fname) +{ +} + +static inline int ceph_fname_to_usr(struct inode *inode, char *name, u32 len, + struct fscrypt_str *tname, struct fscrypt_str *oname, bool *is_nokey) +{ + oname->name = dname; + oname->len = dlen; + return 0; +} + #endif /* CONFIG_FS_ENCRYPTION */ #endif diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index b6e5d751024d..e9fdb9c07320 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -9,6 +9,7 @@ #include "super.h" #include "mds_client.h" +#include "crypto.h" /* * Directory operations: readdir, lookup, create, link, unlink, @@ -241,7 +242,9 @@ static int __dcache_readdir(struct file *file, struct dir_context *ctx, di = ceph_dentry(dentry); if (d_unhashed(dentry) || d_really_is_negative(dentry) || - di->lease_shared_gen != shared_gen) { + di->lease_shared_gen != shared_gen || + ((dentry->d_flags & DCACHE_ENCRYPTED_NAME) && + fscrypt_has_encryption_key(dir))) { spin_unlock(&dentry->d_lock); dput(dentry); err = -EAGAIN; @@ -313,6 +316,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) int err; unsigned frag = -1; struct ceph_mds_reply_info_parsed *rinfo; + struct fscrypt_str tname = FSTR_INIT(NULL, 0); + struct fscrypt_str oname = FSTR_INIT(NULL, 0); dout("readdir %p file %p pos %llx\n", inode, file, ctx->pos); if (dfi->file_info.flags & CEPH_F_ATEND) @@ -340,6 +345,12 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) ctx->pos = 2; } + if (IS_ENCRYPTED(inode)) { + err = fscrypt_get_encryption_info(inode); + if (err) + goto out; + } + spin_lock(&ci->i_ceph_lock); /* request Fx cap. if have Fx, we don't need to release Fs cap * for later create/unlink. */ @@ -360,6 +371,14 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) spin_unlock(&ci->i_ceph_lock); } + err = ceph_fname_alloc_buffer(inode, &tname); + if (err < 0) + goto out; + + err = ceph_fname_alloc_buffer(inode, &oname); + if (err < 0) + goto out; + /* proceed with a normal readdir */ more: /* do we have the correct frag content buffered? */ @@ -387,12 +406,14 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) dout("readdir fetching %llx.%llx frag %x offset '%s'\n", ceph_vinop(inode), frag, dfi->last_name); req = ceph_mdsc_create_request(mdsc, op, USE_AUTH_MDS); - if (IS_ERR(req)) - return PTR_ERR(req); + if (IS_ERR(req)) { + err = PTR_ERR(req); + goto out; + } err = ceph_alloc_readdir_reply_buffer(req, inode); if (err) { ceph_mdsc_put_request(req); - return err; + goto out; } /* hints to request -> mds selection code */ req->r_direct_mode = USE_AUTH_MDS; @@ -405,7 +426,8 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) req->r_path2 = kstrdup(dfi->last_name, GFP_KERNEL); if (!req->r_path2) { ceph_mdsc_put_request(req); - return -ENOMEM; + err = -ENOMEM; + goto out; } } else if (is_hash_order(ctx->pos)) { req->r_args.readdir.offset_hash = @@ -426,7 +448,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) err = ceph_mdsc_do_request(mdsc, NULL, req); if (err < 0) { ceph_mdsc_put_request(req); - return err; + goto out; } dout("readdir got and parsed readdir result=%d on " "frag %x, end=%d, complete=%d, hash_order=%d\n", @@ -479,7 +501,7 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) err = note_last_dentry(dfi, rde->name, rde->name_len, next_offset); if (err) - return err; + goto out; } else if (req->r_reply_info.dir_end) { dfi->next_offset = 2; /* keep last name */ @@ -506,9 +528,12 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) } } for (; i < rinfo->dir_nr; i++) { + bool is_nokey = false; struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i; + u32 olen = oname.len; BUG_ON(rde->offset < ctx->pos); + BUG_ON(!rde->inode.in); ctx->pos = rde->offset; dout("readdir (%d/%d) -> %llx '%.*s' %p\n", @@ -517,12 +542,20 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) BUG_ON(!rde->inode.in); - if (!dir_emit(ctx, rde->name, rde->name_len, + err = ceph_fname_to_usr(inode, rde->name, rde->name_len, &tname, &oname, &is_nokey); + if (err) + goto out; + + if (!dir_emit(ctx, oname.name, oname.len, ceph_present_ino(inode->i_sb, le64_to_cpu(rde->inode.in->ino)), le32_to_cpu(rde->inode.in->mode) >> 12)) { dout("filldir stopping us...\n"); - return 0; + err = 0; + goto out; } + + /* Reset the lengths to their original allocated vals */ + oname.len = olen; ctx->pos++; } @@ -577,9 +610,12 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx) dfi->dir_ordered_count); spin_unlock(&ci->i_ceph_lock); } - + err = 0; dout("readdir %p file %p done.\n", inode, file); - return 0; +out: + ceph_fname_free_buffer(inode, &tname); + ceph_fname_free_buffer(inode, &oname); + return err; } static void reset_readdir(struct ceph_dir_file_info *dfi) diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 2fda08518312..c3a7a9f5603d 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -1653,7 +1653,8 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req, struct ceph_mds_session *session) { struct dentry *parent = req->r_dentry; - struct ceph_inode_info *ci = ceph_inode(d_inode(parent)); + struct inode *inode = d_inode(parent); + struct ceph_inode_info *ci = ceph_inode(inode); struct ceph_mds_reply_info_parsed *rinfo = &req->r_reply_info; struct qstr dname; struct dentry *dn; @@ -1664,6 +1665,8 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req, u32 last_hash = 0; u32 fpos_offset; struct ceph_readdir_cache_control cache_ctl = {}; + struct fscrypt_str tname = FSTR_INIT(NULL, 0); + struct fscrypt_str oname = FSTR_INIT(NULL, 0); if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags)) return readdir_prepopulate_inodes_only(req, session); @@ -1715,14 +1718,29 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req, cache_ctl.index = req->r_readdir_cache_idx; fpos_offset = req->r_readdir_offset; + err = ceph_fname_alloc_buffer(inode, &tname); + if (err < 0) + goto out; + + err = ceph_fname_alloc_buffer(inode, &oname); + if (err < 0) + goto out; + /* FIXME: release caps/leases if error occurs */ for (i = 0; i < rinfo->dir_nr; i++) { + bool is_nokey = false; struct ceph_mds_reply_dir_entry *rde = rinfo->dir_entries + i; struct ceph_vino tvino; + u32 olen = oname.len; - dname.name = rde->name; - dname.len = rde->name_len; + err = ceph_fname_to_usr(inode, rde->name, rde->name_len, &tname, &oname, &is_nokey); + if (err) + goto out; + + dname.name = oname.name; + dname.len = oname.len; dname.hash = full_name_hash(parent, dname.name, dname.len); + oname.len = olen; tvino.ino = le64_to_cpu(rde->inode.in->ino); tvino.snap = le64_to_cpu(rde->inode.in->snapid); @@ -1753,6 +1771,11 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req, err = -ENOMEM; goto out; } + if (is_nokey) { + spin_lock(&dn->d_lock); + dn->d_flags |= DCACHE_ENCRYPTED_NAME; + spin_unlock(&dn->d_lock); + } } else if (d_really_is_positive(dn) && (ceph_ino(d_inode(dn)) != tvino.ino || ceph_snap(d_inode(dn)) != tvino.snap)) { @@ -1843,6 +1866,8 @@ int ceph_readdir_prepopulate(struct ceph_mds_request *req, req->r_readdir_cache_idx = cache_ctl.index; } ceph_readdir_cache_release(&cache_ctl); + ceph_fname_free_buffer(inode, &tname); + ceph_fname_free_buffer(inode, &oname); dout("readdir_prepopulate done\n"); return err; }
Add helper functions for buffer management and for decrypting filenames returned by the MDS. Wire those into the readdir codepaths. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/crypto.c | 47 +++++++++++++++++++++++++++++++++++++++ fs/ceph/crypto.h | 35 +++++++++++++++++++++++++++++ fs/ceph/dir.c | 58 +++++++++++++++++++++++++++++++++++++++--------- fs/ceph/inode.c | 31 +++++++++++++++++++++++--- 4 files changed, 157 insertions(+), 14 deletions(-)