From e735298935b64541fc561bd9e978cd7af48c503e Mon Sep 17 00:00:00 2001 From: Sebastien Buisson Date: Tue, 31 Aug 2021 17:30:48 +0200 Subject: [PATCH] LU-13717 sec: filename encryption - symlink support On client side, call the appropriate llcrypt primitives from llite, to proceed with symlink encryption before sending requests to servers and symlink decryption upon request receipt. The tricky part is that llcrypt needs an inode to encrypt the target name. But by the time we prepare the symlink creation request to be sent to the server with the target name (in ll_new_node), we do not have an inode yet (it will be obtained only after we get the server reply). So we create a fake inode and associate the right encryption context to it, so that the symlink gets encrypted properly. In order to report the correct size for an encrypted symlink (which is ought to be the length of the symlink target), we need to read the symlink target and decrypt or decode it in ->getattr(). This has a performance hit, but given that the symlink target is cached in ->i_link (when the key is available), the symlink will not have to be read and decrypted again later when it is actually followed, readlink() is called, or lstat() is called again. This part of the patch is adapted from kernel commit d18760560593e5af921f51a8c9b64b6109d634c2 "fscrypt: add fscrypt_symlink_getattr() for computing st_size" With encrypted file names, a symlink target is binary. So make sure server side can handle that, by switching sp_symname to a struct lu_name in struct md_op_spec. Signed-off-by: Sebastien Buisson Change-Id: Ic6892fca8926a35001697c54aaf05d15563b139d Reviewed-on: https://review.whamcloud.com/43394 Tested-by: jenkins Reviewed-by: Patrick Farrell Reviewed-by: Andreas Dilger Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/include/md_object.h | 2 +- lustre/llite/namei.c | 75 +++++++++++++++++++++++++++++++++---- lustre/llite/symlink.c | 93 ++++++++++++++++++++++++++++++++++++++++++++-- lustre/mdd/mdd_dir.c | 14 ++++--- lustre/mdt/mdt_lib.c | 47 ++++++++++++----------- lustre/tests/sanity-sec.sh | 57 ++++++++++++++++++++++++++-- 6 files changed, 245 insertions(+), 43 deletions(-) diff --git a/lustre/include/md_object.h b/lustre/include/md_object.h index f449f93..1785744 100644 --- a/lustre/include/md_object.h +++ b/lustre/include/md_object.h @@ -143,7 +143,7 @@ struct md_attr { struct md_op_spec { union { /** symlink target */ - const char *sp_symname; + struct lu_name sp_symname; /** eadata for regular files */ struct md_spec_reg { void *eadata; diff --git a/lustre/llite/namei.c b/lustre/llite/namei.c index ed37f0c..9e314f1 100644 --- a/lustre/llite/namei.c +++ b/lustre/llite/namei.c @@ -1537,20 +1537,24 @@ unlock: } static int ll_new_node(struct inode *dir, struct dentry *dchild, - const char *tgt, umode_t mode, int rdev, __u32 opc) + const char *tgt, umode_t mode, __u64 rdev, __u32 opc) { struct qstr *name = &dchild->d_name; struct ptlrpc_request *request = NULL; struct md_op_data *op_data = NULL; struct inode *inode = NULL; struct ll_sb_info *sbi = ll_i2sbi(dir); - int tgt_len = 0; + struct llcrypt_str *disk_link = NULL; bool encrypt = false; int err; ENTRY; - if (unlikely(tgt != NULL)) - tgt_len = strlen(tgt) + 1; + if (unlikely(tgt != NULL)) { + disk_link = (struct llcrypt_str *)rdev; + rdev = 0; + if (!disk_link) + RETURN(-EINVAL); + } again: op_data = ll_prep_md_op_data(NULL, dir, NULL, name->name, @@ -1586,9 +1590,41 @@ again: err = llcrypt_inherit_context(dir, NULL, op_data, false); if (err) GOTO(err_exit, err); + + if (S_ISLNK(mode)) { + /* llcrypt needs inode to encrypt target name, so create + * a fake inode and associate encryption context got + * from llcrypt_inherit_context. + */ + struct inode *fakeinode = + dchild->d_sb->s_op->alloc_inode(dchild->d_sb); + + if (!fakeinode) + GOTO(err_exit, err = -ENOMEM); + fakeinode->i_sb = dchild->d_sb; + fakeinode->i_mode |= S_IFLNK; +#ifdef IOP_XATTR + fakeinode->i_opflags |= IOP_XATTR; +#endif + err = ll_set_encflags(fakeinode, + op_data->op_file_encctx, + op_data->op_file_encctx_size, + true); + if (!err) + err = __llcrypt_encrypt_symlink(fakeinode, tgt, + strlen(tgt), + disk_link); + + ll_xattr_cache_destroy(fakeinode); + llcrypt_put_encryption_info(fakeinode); + dchild->d_sb->s_op->destroy_inode(fakeinode); + if (err) + GOTO(err_exit, err); + } } - err = md_create(sbi->ll_md_exp, op_data, tgt, tgt_len, mode, + err = md_create(sbi->ll_md_exp, op_data, tgt ? disk_link->name : NULL, + tgt ? disk_link->len : 0, mode, from_kuid(&init_user_ns, current_fsuid()), from_kgid(&init_user_ns, current_fsgid()), current_cap(), rdev, &request); @@ -1691,6 +1727,21 @@ again: op_data->op_file_encctx_size, true); if (err) GOTO(err_exit, err); + + if (S_ISLNK(mode)) { + struct ll_inode_info *lli = ll_i2info(inode); + + /* Cache the plaintext symlink target + * for later use by get_link() + */ + OBD_ALLOC(lli->lli_symlink_name, strlen(tgt) + 1); + /* do not return an error if we cannot + * cache the symlink locally + */ + if (lli->lli_symlink_name) + memcpy(lli->lli_symlink_name, + tgt, strlen(tgt) + 1); + } } if (!(sbi->ll_flags & LL_SBI_FILE_SECCTX)) { @@ -1782,14 +1833,24 @@ static int ll_symlink(struct inode *dir, struct dentry *dchild, const char *oldpath) { ktime_t kstart = ktime_get(); + int len = strlen(oldpath); + struct llcrypt_str disk_link; int err; ENTRY; CDEBUG(D_VFSTRACE, "VFS Op:name=%pd, dir="DFID"(%p), target=%.*s\n", dchild, PFID(ll_inode2fid(dir)), dir, 3000, oldpath); - err = ll_new_node(dir, dchild, oldpath, S_IFLNK | S_IRWXUGO, 0, - LUSTRE_OPC_SYMLINK); + err = llcrypt_prepare_symlink(dir, oldpath, len, dir->i_sb->s_blocksize, + &disk_link); + if (err) + RETURN(err); + + err = ll_new_node(dir, dchild, oldpath, S_IFLNK | S_IRWXUGO, + (__u64)&disk_link, LUSTRE_OPC_SYMLINK); + + if (disk_link.name != (unsigned char *)oldpath) + kfree(disk_link.name); if (!err) ll_stats_ops_tally(ll_i2sbi(dir), LPROC_LL_SYMLINK, diff --git a/lustre/llite/symlink.c b/lustre/llite/symlink.c index 20a61be..1319c09 100644 --- a/lustre/llite/symlink.c +++ b/lustre/llite/symlink.c @@ -38,8 +38,18 @@ #include "llite_internal.h" /* Must be called with lli_size_mutex locked */ +/* HAVE_IOP_GET_LINK is defined from kernel 4.5, whereas + * IS_ENCRYPTED is brought by kernel 4.14. + * So there is no need to handle encryption case otherwise. + */ +#ifdef HAVE_IOP_GET_LINK +static int ll_readlink_internal(struct inode *inode, + struct ptlrpc_request **request, + char **symname, struct delayed_call *done) +#else static int ll_readlink_internal(struct inode *inode, struct ptlrpc_request **request, char **symname) +#endif { struct ll_inode_info *lli = ll_i2info(inode); struct ll_sb_info *sbi = ll_i2sbi(inode); @@ -98,7 +108,9 @@ static int ll_readlink_internal(struct inode *inode, } *symname = req_capsule_server_get(&(*request)->rq_pill, &RMF_MDT_MD); - if (!*symname || strnlen(*symname, symlen) != symlen - 1) { + if (!*symname || + (!IS_ENCRYPTED(inode) && + strnlen(*symname, symlen) != symlen - 1)) { /* not full/NULL terminated */ CERROR("%s: inode "DFID": symlink not NULL terminated string of length %d\n", sbi->ll_fsname, @@ -106,6 +118,23 @@ static int ll_readlink_internal(struct inode *inode, GOTO(failed, rc = -EPROTO); } +#ifdef HAVE_IOP_GET_LINK + if (IS_ENCRYPTED(inode)) { + const char *target = llcrypt_get_symlink(inode, *symname, + symlen, done); + if (IS_ERR(target)) + RETURN(PTR_ERR(target)); + symlen = strlen(target) + 1; + *symname = (char *)target; + + /* Do not cache symlink targets encoded without the key, + * since those become outdated once the key is added. + */ + if (!llcrypt_has_encryption_key(inode)) + RETURN(0); + } +#endif + OBD_ALLOC(lli->lli_symlink_name, symlen); /* do not return an error if we cannot cache the symlink locally */ if (lli->lli_symlink_name) { @@ -181,11 +210,12 @@ static const char *ll_get_link(struct dentry *dentry, int rc; ENTRY; - CDEBUG(D_VFSTRACE, "VFS Op\n"); + CDEBUG(D_VFSTRACE, "VFS Op:name=%pd, inode="DFID"(%p)\n", + dentry, PFID(ll_inode2fid(inode)), inode); if (!dentry) RETURN(ERR_PTR(-ECHILD)); ll_inode_size_lock(inode); - rc = ll_readlink_internal(inode, &request, &symname); + rc = ll_readlink_internal(inode, &request, &symname, done); ll_inode_size_unlock(inode); if (rc < 0) { ptlrpc_req_finished(request); @@ -228,6 +258,61 @@ static const char *ll_follow_link(struct dentry *dentry, void **cookie) # endif /* HAVE_IOP_GET_LINK */ #endif /* HAVE_SYMLINK_OPS_USE_NAMEIDATA */ +#ifdef HAVE_INODEOPS_ENHANCED_GETATTR +/** + * ll_getattr_link() - link-specific getattr to set the correct st_size + * for encrypted symlinks + * + * Override st_size of encrypted symlinks to be the length of the decrypted + * symlink target (or the no-key encoded symlink target, if the key is + * unavailable) rather than the length of the encrypted symlink target. This is + * necessary for st_size to match the symlink target that userspace actually + * sees. POSIX requires this, and some userspace programs depend on it. + * + * For non encrypted symlinks, this is a just calling ll_getattr(). + * For encrypted symlinks, this additionally requires reading the symlink target + * from disk if needed, setting up the inode's encryption key if possible, and + * then decrypting or encoding the symlink target. This makes lstat() more + * heavyweight than is normally the case. However, decrypted symlink targets + * will be cached in ->i_link, so usually the symlink won't have to be read and + * decrypted again later if/when it is actually followed, readlink() is called, + * or lstat() is called again. + * + * Return: 0 on success, -errno on failure + */ +static int ll_getattr_link(const struct path *path, struct kstat *stat, + u32 request_mask, unsigned int flags) +{ + struct dentry *dentry = path->dentry; + struct inode *inode = d_inode(dentry); + DEFINE_DELAYED_CALL(done); + const char *link; + int rc; + + rc = ll_getattr(path, stat, request_mask, flags); + if (rc || !IS_ENCRYPTED(inode)) + return rc; + + /* + * To get the symlink target that userspace will see (whether it's the + * decrypted target or the no-key encoded target), we can just get it + * in the same way the VFS does during path resolution and readlink(). + */ + link = READ_ONCE(inode->i_link); + if (!link) { + link = inode->i_op->get_link(dentry, inode, &done); + if (IS_ERR(link)) + return PTR_ERR(link); + } + stat->size = strlen(link); + do_delayed_call(&done); + return 0; +} +#else /* HAVE_INODEOPS_ENHANCED_GETATTR */ +#define ll_getattr_link ll_getattr +#endif + + const struct inode_operations ll_fast_symlink_inode_operations = { #ifdef HAVE_IOP_GENERIC_READLINK .readlink = generic_readlink, @@ -239,7 +324,7 @@ const struct inode_operations ll_fast_symlink_inode_operations = { .follow_link = ll_follow_link, .put_link = ll_put_link, #endif - .getattr = ll_getattr, + .getattr = ll_getattr_link, .permission = ll_inode_permission, #ifdef HAVE_IOP_XATTR .setxattr = ll_setxattr, diff --git a/lustre/mdd/mdd_dir.c b/lustre/mdd/mdd_dir.c index cb6fbea..3c8240c 100644 --- a/lustre/mdd/mdd_dir.c +++ b/lustre/mdd/mdd_dir.c @@ -2120,7 +2120,7 @@ static int mdd_create_sanity_check(const struct lu_env *env, switch (cattr->la_mode & S_IFMT) { case S_IFLNK: { - unsigned int symlen = strlen(spec->u.sp_symname) + 1; + unsigned int symlen = spec->u.sp_symname.ln_namelen + 1; if (symlen > m->mdd_dt_conf.ddp_symlink_max) RETURN(-ENAMETOOLONG); @@ -2213,8 +2213,8 @@ static int mdd_declare_create_object(const struct lu_env *env, } if (S_ISLNK(attr->la_mode)) { - const char *target_name = spec->u.sp_symname; - int sym_len = strlen(target_name); + const char *target_name = spec->u.sp_symname.ln_name; + int sym_len = spec->u.sp_symname.ln_namelen; const struct lu_buf *buf; buf = mdd_buf_get_const(env, target_name, sym_len); @@ -2447,8 +2447,8 @@ static int mdd_create_object(const struct lu_env *env, struct mdd_object *pobj, if (S_ISLNK(attr->la_mode)) { struct dt_object *dt = mdd_object_child(son); - const char *target_name = spec->u.sp_symname; - int sym_len = strlen(target_name); + const char *target_name = spec->u.sp_symname.ln_name; + int sym_len = spec->u.sp_symname.ln_namelen; loff_t pos = 0; buf = mdd_buf_get_const(env, target_name, sym_len); @@ -3802,7 +3802,9 @@ static int mdd_declare_migrate_create(const struct lu_env *env, lum->lum_hash_type |= cpu_to_le32(LMV_HASH_FLAG_MIGRATION); } else if (S_ISLNK(attr->la_mode)) { - spec->u.sp_symname = sbuf->lb_buf; + spec->u.sp_symname.ln_name = sbuf->lb_buf; + /* don't count NUL */ + spec->u.sp_symname.ln_namelen = sbuf->lb_len - 1; } else if (S_ISREG(attr->la_mode)) { spec->sp_cr_flags |= MDS_OPEN_DELAY_CREATE; spec->sp_cr_flags &= ~MDS_OPEN_HAS_EA; diff --git a/lustre/mdt/mdt_lib.c b/lustre/mdt/mdt_lib.c index e06fd77..b5c75e9 100644 --- a/lustre/mdt/mdt_lib.c +++ b/lustre/mdt/mdt_lib.c @@ -1288,35 +1288,38 @@ static int mdt_create_unpack(struct mdt_thread_info *info) uc->uc_suppgids[1] = -1; uc->uc_umask = rec->cr_umask; - rr->rr_fid1 = &rec->cr_fid1; - rr->rr_fid2 = &rec->cr_fid2; - attr->la_mode = rec->cr_mode; - attr->la_rdev = rec->cr_rdev; - attr->la_uid = rec->cr_fsuid; - attr->la_gid = rec->cr_fsgid; - attr->la_ctime = rec->cr_time; - attr->la_mtime = rec->cr_time; - attr->la_atime = rec->cr_time; + rr->rr_fid1 = &rec->cr_fid1; + rr->rr_fid2 = &rec->cr_fid2; + attr->la_mode = rec->cr_mode; + attr->la_rdev = rec->cr_rdev; + attr->la_uid = rec->cr_fsuid; + attr->la_gid = rec->cr_fsgid; + attr->la_ctime = rec->cr_time; + attr->la_mtime = rec->cr_time; + attr->la_atime = rec->cr_time; attr->la_valid = LA_MODE | LA_RDEV | LA_UID | LA_GID | LA_TYPE | - LA_CTIME | LA_MTIME | LA_ATIME; - memset(&sp->u, 0, sizeof(sp->u)); - sp->sp_cr_flags = get_mrc_cr_flags(rec); + LA_CTIME | LA_MTIME | LA_ATIME; + memset(&sp->u, 0, sizeof(sp->u)); + sp->sp_cr_flags = get_mrc_cr_flags(rec); rc = mdt_name_unpack(pill, &RMF_NAME, &rr->rr_name, 0); if (rc < 0) RETURN(rc); if (S_ISLNK(attr->la_mode)) { - const char *tgt = NULL; - - req_capsule_extend(pill, &RQF_MDS_REINT_CREATE_SYM); - if (req_capsule_get_size(pill, &RMF_SYMTGT, RCL_CLIENT)) { - tgt = req_capsule_client_get(pill, &RMF_SYMTGT); - sp->u.sp_symname = tgt; - } - if (tgt == NULL) - RETURN(-EFAULT); - } else { + const char *tgt = NULL; + int sz; + + req_capsule_extend(pill, &RQF_MDS_REINT_CREATE_SYM); + sz = req_capsule_get_size(pill, &RMF_SYMTGT, RCL_CLIENT); + if (sz) { + tgt = req_capsule_client_get(pill, &RMF_SYMTGT); + sp->u.sp_symname.ln_name = tgt; + sp->u.sp_symname.ln_namelen = sz - 1; /* skip NUL */ + } + if (tgt == NULL) + RETURN(-EFAULT); + } else { req_capsule_extend(pill, &RQF_MDS_REINT_CREATE_ACL); if (S_ISDIR(attr->la_mode) && req_capsule_get_size(pill, &RMF_EADATA, RCL_CLIENT) > 0) { diff --git a/lustre/tests/sanity-sec.sh b/lustre/tests/sanity-sec.sh index 8938082..751f4ec 100755 --- a/lustre/tests/sanity-sec.sh +++ b/lustre/tests/sanity-sec.sh @@ -3334,7 +3334,7 @@ test_47() { stack_trap cleanup_for_enc_tests EXIT setup_for_enc_tests - dd if=/dev/zero of=$tmpfile bs=512K count=1 + dd if=/dev/urandom of=$tmpfile bs=512K count=1 mrename $tmpfile $testfile && error "rename from unencrypted to encrypted dir should fail" @@ -3350,10 +3350,23 @@ test_47() { ln $testfile2 $testfile || error "link from within encrypted dir should succeed" + cmp -bl $testfile2 $testfile || + error "cannot read from hard link (1.1)" + echo a >> $testfile || error "cannot write to hard link (1)" + cancel_lru_locks + cmp -bl $testfile2 $testfile || + error "cannot read from hard link (1.2)" rm -f $testfile ln $testfile2 $tmpfile || error "link from unencrypted to encrypted dir should succeed" + cancel_lru_locks + cmp -bl $testfile2 $tmpfile || + error "cannot read from hard link (2.1)" + echo a >> $tmpfile || error "cannot write to hard link (2)" + cancel_lru_locks + cmp -bl $testfile2 $tmpfile || + error "cannot read from hard link (2.2)" rm -f $tmpfile # check we are limited in the number of hard links @@ -3367,10 +3380,36 @@ test_47() { mrename $testfile2 $tmpfile && error "rename from encrypted to unencrypted dir should fail" rm -f $testfile2 - touch $tmpfile + dd if=/dev/urandom of=$tmpfile bs=512K count=1 - dd if=/dev/zero of=$testfile bs=512K count=1 + dd if=/dev/urandom of=$testfile bs=512K count=1 mkdir $DIR/$tdir/mydir + + ln -s $testfile ${testfile}.sym || + error "symlink from within encrypted dir should succeed" + cancel_lru_locks + cmp -bl $testfile ${testfile}.sym || + error "cannot read from sym link (1.1)" + echo a >> ${testfile}.sym || error "cannot write to sym link (1)" + cancel_lru_locks + cmp -bl $testfile ${testfile}.sym || + error "cannot read from sym link (1.2)" + [ $(stat -c %s ${testfile}.sym) -eq ${#testfile} ] || + error "wrong symlink size (1)" + + ln -s $tmpfile ${testfile}.sl || + error "symlink from encrypted to unencrypted dir should succeed" + cancel_lru_locks + cmp -bl $tmpfile ${testfile}.sl || + error "cannot read from sym link (2.1)" + echo a >> ${testfile}.sl || error "cannot write to sym link (2)" + cancel_lru_locks + cmp -bl $tmpfile ${testfile}.sl || + error "cannot read from sym link (2.2)" + [ $(stat -c %s ${testfile}.sl) -eq ${#tmpfile} ] || + error "wrong symlink size (2)" + rm -f ${testfile}.sl + sync ; echo 3 > /proc/sys/vm/drop_caches # remove fscrypt key from keyring @@ -3380,6 +3419,7 @@ test_47() { scrambleddir=$(find $DIR/$tdir/ -maxdepth 1 -mindepth 1 -type d) scrambledfile=$(find $DIR/$tdir/ -maxdepth 1 -type f) + scrambledlink=$(find $DIR/$tdir/ -maxdepth 1 -type l) ln $scrambledfile $scrambleddir/linkfile && error "ln linkfile should have failed" mrename $scrambledfile $DIR/onefile2 && @@ -3387,6 +3427,17 @@ test_47() { touch $DIR/onefile mrename $DIR/onefile $scrambleddir/otherfile && error "mrename to $scrambleddir should have failed" + readlink $scrambledlink || + error "link should be read without key" + [ $(stat -c %s $scrambledlink) -eq \ + $(expr length "$(readlink $scrambledlink)") ] || + error "wrong symlink size without key" + readlink -e $scrambledlink && + error "link should not point to anywhere useful" + ln -s $scrambledfile ${scrambledfile}.sym && + error "symlink without key should fail (1)" + ln -s $tmpfile ${scrambledfile}.sl && + error "symlink without key should fail (2)" rm -f $tmpfile $DIR/onefile } -- 1.8.3.1