diff mbox series

[RFC,v3,16/16] ceph: create symlinks with encrypted and base64-encoded targets

Message ID 20200914191707.380444-17-jlayton@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series ceph+fscrypt: context, filename and symlink support | expand

Commit Message

Jeff Layton Sept. 14, 2020, 7:17 p.m. UTC 
When creating symlinks in encrypted directories, encrypt and
base64-encode the target with the new inode's key before sending to the
MDS.

When filling a symlinked inode, base64-decode it into a buffer that
we'll keep in ci->i_symlink. When get_link is called, decrypt the buffer
into a new one that will hang off i_link.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/dir.c   | 32 +++++++++++++++++++---
 fs/ceph/inode.c | 71 ++++++++++++++++++++++++++++++++++++++-----------
 fs/ceph/super.h |  2 ++
 3 files changed, 86 insertions(+), 19 deletions(-)

Comments

Eric Biggers Sept. 15, 2020, 2:07 a.m. UTC | #1 
On Mon, Sep 14, 2020 at 03:17:07PM -0400, Jeff Layton wrote:
> +	if (IS_ENCRYPTED(req->r_new_inode)) {
> +		int len = strlen(dest);
> +
> +		err = fscrypt_prepare_symlink(dir, dest, len, PATH_MAX, &osd_link);
> +		if (err)
> +			goto out_req;
> +
> +		err = fscrypt_encrypt_symlink(req->r_new_inode, dest, len, &osd_link);
> +		if (err)
> +			goto out_req;
> +
> +		req->r_path2 = kmalloc(FSCRYPT_BASE64_CHARS(osd_link.len) + 1, GFP_KERNEL);

osd_link.len includes a null terminator.  It seems that's not what's wanted
here, and you should be subtracting 1 here.

(fscrypt_prepare_symlink() maybe should exclude the null terminator from the
length instead.  But for the other filesystems it was easier to include it...)

> @@ -996,26 +995,39 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page,
>  		inode->i_fop = &ceph_file_fops;
>  		break;
>  	case S_IFLNK:
> -		inode->i_op = &ceph_symlink_iops;
>  		if (!ci->i_symlink) {
>  			u32 symlen = iinfo->symlink_len;
>  			char *sym;
>  
>  			spin_unlock(&ci->i_ceph_lock);
>  
> -			if (symlen != i_size_read(inode)) {
> -				pr_err("%s %llx.%llx BAD symlink "
> -					"size %lld\n", __func__,
> -					ceph_vinop(inode),
> -					i_size_read(inode));
> +			if (IS_ENCRYPTED(inode)) {
> +				/* Do base64 decode so that we get the right size (maybe?) */
> +				err = -ENOMEM;
> +				sym = kmalloc(symlen + 1, GFP_NOFS);
> +				if (!sym)
> +					goto out;
> +
> +				symlen = fscrypt_base64_decode(iinfo->symlink, symlen, sym);
> +				/*
> +				 * i_size as reported by the MDS may be wrong, due to base64
> +				 * inflation and padding. Fix it up here.
> +				 */
>  				i_size_write(inode, symlen);

Note that fscrypt_base64_decode() can fail (return -1) if the input is not valid
base64.  That isn't being handled here.

> +static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
> +					   struct delayed_call *done)
> +{
> +	struct ceph_inode_info *ci = ceph_inode(inode);
> +
> +	if (!dentry)
> +		return ERR_PTR(-ECHILD);
> +
> +	return fscrypt_get_symlink(inode, ci->i_symlink, ksize(ci->i_symlink), done);

Using ksize() seems wrong here, since that would allow fscrypt_get_symlink() to
read beyond the part of the buffer that is actually initialized.

> -static const struct inode_operations ceph_symlink_iops = {
> +const struct inode_operations ceph_symlink_iops = {
>  	.get_link = simple_get_link,
>  	.setattr = ceph_setattr,
>  	.getattr = ceph_getattr,
>  	.listxattr = ceph_listxattr,
>  };
>  
> +const struct inode_operations ceph_encrypted_symlink_iops = {
> +	.get_link = ceph_encrypted_get_link,
> +	.setattr = ceph_setattr,
> +	.getattr = ceph_getattr,
> +	.listxattr = ceph_listxattr,
> +};

These don't need to be made global, as they're only used in fs/ceph/inode.c.

- Eric
Jeff Layton Sept. 15, 2020, 2:05 p.m. UTC | #2 
On Mon, 2020-09-14 at 19:07 -0700, Eric Biggers wrote:
> On Mon, Sep 14, 2020 at 03:17:07PM -0400, Jeff Layton wrote:
> > +	if (IS_ENCRYPTED(req->r_new_inode)) {
> > +		int len = strlen(dest);
> > +
> > +		err = fscrypt_prepare_symlink(dir, dest, len, PATH_MAX, &osd_link);
> > +		if (err)
> > +			goto out_req;
> > +
> > +		err = fscrypt_encrypt_symlink(req->r_new_inode, dest, len, &osd_link);
> > +		if (err)
> > +			goto out_req;
> > +
> > +		req->r_path2 = kmalloc(FSCRYPT_BASE64_CHARS(osd_link.len) + 1, GFP_KERNEL);
> 
> osd_link.len includes a null terminator.  It seems that's not what's wanted
> here, and you should be subtracting 1 here.
> 
> (fscrypt_prepare_symlink() maybe should exclude the null terminator from the
> length instead.  But for the other filesystems it was easier to include it...)
> 

Got it. Fixed.

> > @@ -996,26 +995,39 @@ int ceph_fill_inode(struct inode *inode, struct page *locked_page,
> >  		inode->i_fop = &ceph_file_fops;
> >  		break;
> >  	case S_IFLNK:
> > -		inode->i_op = &ceph_symlink_iops;
> >  		if (!ci->i_symlink) {
> >  			u32 symlen = iinfo->symlink_len;
> >  			char *sym;
> >  
> >  			spin_unlock(&ci->i_ceph_lock);
> >  
> > -			if (symlen != i_size_read(inode)) {
> > -				pr_err("%s %llx.%llx BAD symlink "
> > -					"size %lld\n", __func__,
> > -					ceph_vinop(inode),
> > -					i_size_read(inode));
> > +			if (IS_ENCRYPTED(inode)) {
> > +				/* Do base64 decode so that we get the right size (maybe?) */
> > +				err = -ENOMEM;
> > +				sym = kmalloc(symlen + 1, GFP_NOFS);
> > +				if (!sym)
> > +					goto out;
> > +
> > +				symlen = fscrypt_base64_decode(iinfo->symlink, symlen, sym);
> > +				/*
> > +				 * i_size as reported by the MDS may be wrong, due to base64
> > +				 * inflation and padding. Fix it up here.
> > +				 */
> >  				i_size_write(inode, symlen);
> 
> Note that fscrypt_base64_decode() can fail (return -1) if the input is not valid
> base64.  That isn't being handled here.
> 

Thanks, fixed. It'll return -EIO in that case now.

> > +static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
> > +					   struct delayed_call *done)
> > +{
> > +	struct ceph_inode_info *ci = ceph_inode(inode);
> > +
> > +	if (!dentry)
> > +		return ERR_PTR(-ECHILD);
> > +
> > +	return fscrypt_get_symlink(inode, ci->i_symlink, ksize(ci->i_symlink), done);
> 
> Using ksize() seems wrong here, since that would allow fscrypt_get_symlink() to
> read beyond the part of the buffer that is actually initialized.
> 

Is that actually a problem? I did have an earlier patch that carried
around the length, but it didn't seem to be necessary.

ISTM that that might end up decrypting more data than is actually
needed, but eventually there will be a NULL terminator in the data and
the rest would be ignored.

If it is a problem, then we should probably change the comment header
over fscrypt_get_symlink. It currently says:

   * @max_size: size of @caddr buffer

...which is another reason why I figured using ksize there was OK.

> > -static const struct inode_operations ceph_symlink_iops = {
> > +const struct inode_operations ceph_symlink_iops = {
> >  	.get_link = simple_get_link,
> >  	.setattr = ceph_setattr,
> >  	.getattr = ceph_getattr,
> >  	.listxattr = ceph_listxattr,
> >  };
> >  
> > +const struct inode_operations ceph_encrypted_symlink_iops = {
> > +	.get_link = ceph_encrypted_get_link,
> > +	.setattr = ceph_setattr,
> > +	.getattr = ceph_getattr,
> > +	.listxattr = ceph_listxattr,
> > +};
> 
> These don't need to be made global, as they're only used in fs/ceph/inode.c.
> 

Thanks, fixed.
Eric Biggers Sept. 15, 2020, 8:49 p.m. UTC | #3 
On Tue, Sep 15, 2020 at 10:05:53AM -0400, Jeff Layton wrote:
> > > +static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
> > > +					   struct delayed_call *done)
> > > +{
> > > +	struct ceph_inode_info *ci = ceph_inode(inode);
> > > +
> > > +	if (!dentry)
> > > +		return ERR_PTR(-ECHILD);
> > > +
> > > +	return fscrypt_get_symlink(inode, ci->i_symlink, ksize(ci->i_symlink), done);
> > 
> > Using ksize() seems wrong here, since that would allow fscrypt_get_symlink() to
> > read beyond the part of the buffer that is actually initialized.
> > 
> 
> Is that actually a problem? I did have an earlier patch that carried
> around the length, but it didn't seem to be necessary.
> 
> ISTM that that might end up decrypting more data than is actually
> needed, but eventually there will be a NULL terminator in the data and
> the rest would be ignored.
> 

Yes it's a problem.  The code that decrypts the symlink adds the null terminator
at the end.  So if the stated buffer size is wrong, then decrypted uninitialized
memory can be included into the symlink target that userspace then sees.

> If it is a problem, then we should probably change the comment header
> over fscrypt_get_symlink. It currently says:
> 
>    * @max_size: size of @caddr buffer
> 
> ...which is another reason why I figured using ksize there was OK.

ksize() is rarely used, as it should be.  (For one, it disables KASAN on the
buffer...)  I think that when people see "buffer size" they almost always think
the actual allocated size of the buffer, not ksize().  But we could change it to
say "allocated size" if that would make it clearer...

- Eric
Jeff Layton Sept. 16, 2020, 12:15 p.m. UTC | #4 
On Tue, 2020-09-15 at 13:49 -0700, Eric Biggers wrote:
> On Tue, Sep 15, 2020 at 10:05:53AM -0400, Jeff Layton wrote:
> > > > +static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
> > > > +					   struct delayed_call *done)
> > > > +{
> > > > +	struct ceph_inode_info *ci = ceph_inode(inode);
> > > > +
> > > > +	if (!dentry)
> > > > +		return ERR_PTR(-ECHILD);
> > > > +
> > > > +	return fscrypt_get_symlink(inode, ci->i_symlink, ksize(ci->i_symlink), done);
> > > 
> > > Using ksize() seems wrong here, since that would allow fscrypt_get_symlink() to
> > > read beyond the part of the buffer that is actually initialized.
> > > 
> > 
> > Is that actually a problem? I did have an earlier patch that carried
> > around the length, but it didn't seem to be necessary.
> > 
> > ISTM that that might end up decrypting more data than is actually
> > needed, but eventually there will be a NULL terminator in the data and
> > the rest would be ignored.
> > 
> 
> Yes it's a problem.  The code that decrypts the symlink adds the null terminator
> at the end.  So if the stated buffer size is wrong, then decrypted uninitialized
> memory can be included into the symlink target that userspace then sees.
> 
> > If it is a problem, then we should probably change the comment header
> > over fscrypt_get_symlink. It currently says:
> > 
> >    * @max_size: size of @caddr buffer
> > 
> > ...which is another reason why I figured using ksize there was OK.
> 
> ksize() is rarely used, as it should be.  (For one, it disables KASAN on the
> buffer...)  I think that when people see "buffer size" they almost always think
> the actual allocated size of the buffer, not ksize().  But we could change it to
> say "allocated size" if that would make it clearer...
> 

Ok, I'll rework it to carry around the length too. That should take care of the problem.

Thanks!
diff mbox series

Patch

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index e9fdb9c07320..65d82ab239b2 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -928,6 +928,7 @@  static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(dir->i_sb);
 	struct ceph_mds_request *req;
 	struct ceph_acl_sec_ctx as_ctx = {};
+	struct fscrypt_str osd_link = FSTR_INIT(NULL, 0);
 	umode_t mode = S_IFLNK | 0777;
 	int err;
 
@@ -953,11 +954,33 @@  static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 		goto out_req;
 	}
 
-	req->r_path2 = kstrdup(dest, GFP_KERNEL);
-	if (!req->r_path2) {
-		err = -ENOMEM;
-		goto out_req;
+	if (IS_ENCRYPTED(req->r_new_inode)) {
+		int len = strlen(dest);
+
+		err = fscrypt_prepare_symlink(dir, dest, len, PATH_MAX, &osd_link);
+		if (err)
+			goto out_req;
+
+		err = fscrypt_encrypt_symlink(req->r_new_inode, dest, len, &osd_link);
+		if (err)
+			goto out_req;
+
+		req->r_path2 = kmalloc(FSCRYPT_BASE64_CHARS(osd_link.len) + 1, GFP_KERNEL);
+		if (!req->r_path2) {
+			err = -ENOMEM;
+			goto out_req;
+		}
+
+		len = fscrypt_base64_encode(osd_link.name, osd_link.len, req->r_path2);
+		req->r_path2[len] = '\0';
+	} else {
+		req->r_path2 = kstrdup(dest, GFP_KERNEL);
+		if (!req->r_path2) {
+			err = -ENOMEM;
+			goto out_req;
+		}
 	}
+
 	req->r_parent = dir;
 	set_bit(CEPH_MDS_R_PARENT_LOCKED, &req->r_req_flags);
 	req->r_dentry = dget(dentry);
@@ -977,6 +1000,7 @@  static int ceph_symlink(struct inode *dir, struct dentry *dentry,
 	if (err)
 		d_drop(dentry);
 	ceph_release_acl_sec_ctx(&as_ctx);
+	fscrypt_fname_free_buffer(&osd_link);
 	return err;
 }
 
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 8e9fb1311bb8..4ac267cc9085 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -33,8 +33,6 @@ 
  * (typically because they are in the message handler path).
  */
 
-static const struct inode_operations ceph_symlink_iops;
-
 static void ceph_inode_work(struct work_struct *work);
 
 /*
@@ -593,6 +591,7 @@  void ceph_free_inode(struct inode *inode)
 	struct ceph_inode_info *ci = ceph_inode(inode);
 
 	kfree(ci->i_symlink);
+	fscrypt_free_inode(inode);
 	kmem_cache_free(ceph_inode_cachep, ci);
 }
 
@@ -996,26 +995,39 @@  int ceph_fill_inode(struct inode *inode, struct page *locked_page,
 		inode->i_fop = &ceph_file_fops;
 		break;
 	case S_IFLNK:
-		inode->i_op = &ceph_symlink_iops;
 		if (!ci->i_symlink) {
 			u32 symlen = iinfo->symlink_len;
 			char *sym;
 
 			spin_unlock(&ci->i_ceph_lock);
 
-			if (symlen != i_size_read(inode)) {
-				pr_err("%s %llx.%llx BAD symlink "
-					"size %lld\n", __func__,
-					ceph_vinop(inode),
-					i_size_read(inode));
+			if (IS_ENCRYPTED(inode)) {
+				/* Do base64 decode so that we get the right size (maybe?) */
+				err = -ENOMEM;
+				sym = kmalloc(symlen + 1, GFP_NOFS);
+				if (!sym)
+					goto out;
+
+				symlen = fscrypt_base64_decode(iinfo->symlink, symlen, sym);
+				/*
+				 * i_size as reported by the MDS may be wrong, due to base64
+				 * inflation and padding. Fix it up here.
+				 */
 				i_size_write(inode, symlen);
 				inode->i_blocks = calc_inode_blocks(symlen);
-			}
+			} else {
+				if (symlen != i_size_read(inode)) {
+					pr_err("%s %llx.%llx BAD symlink size %lld\n",
+						__func__, ceph_vinop(inode), i_size_read(inode));
+					i_size_write(inode, symlen);
+					inode->i_blocks = calc_inode_blocks(symlen);
+				}
 
-			err = -ENOMEM;
-			sym = kstrndup(iinfo->symlink, symlen, GFP_NOFS);
-			if (!sym)
-				goto out;
+				err = -ENOMEM;
+				sym = kstrndup(iinfo->symlink, symlen, GFP_NOFS);
+				if (!sym)
+					goto out;
+			}
 
 			spin_lock(&ci->i_ceph_lock);
 			if (!ci->i_symlink)
@@ -1023,7 +1035,18 @@  int ceph_fill_inode(struct inode *inode, struct page *locked_page,
 			else
 				kfree(sym); /* lost a race */
 		}
-		inode->i_link = ci->i_symlink;
+
+		if (IS_ENCRYPTED(inode)) {
+			/*
+			 * Encrypted symlinks need to be decrypted before we can
+			 * cache their targets in i_link. Leave it blank for now.
+			 */
+			inode->i_link = NULL;
+			inode->i_op = &ceph_encrypted_symlink_iops;
+		} else {
+			inode->i_link = ci->i_symlink;
+			inode->i_op = &ceph_symlink_iops;
+		}
 		break;
 	case S_IFDIR:
 		inode->i_op = &ceph_dir_iops;
@@ -2124,16 +2147,34 @@  static void ceph_inode_work(struct work_struct *work)
 	iput(inode);
 }
 
+static const char *ceph_encrypted_get_link(struct dentry *dentry, struct inode *inode,
+					   struct delayed_call *done)
+{
+	struct ceph_inode_info *ci = ceph_inode(inode);
+
+	if (!dentry)
+		return ERR_PTR(-ECHILD);
+
+	return fscrypt_get_symlink(inode, ci->i_symlink, ksize(ci->i_symlink), done);
+}
+
 /*
  * symlinks
  */
-static const struct inode_operations ceph_symlink_iops = {
+const struct inode_operations ceph_symlink_iops = {
 	.get_link = simple_get_link,
 	.setattr = ceph_setattr,
 	.getattr = ceph_getattr,
 	.listxattr = ceph_listxattr,
 };
 
+const struct inode_operations ceph_encrypted_symlink_iops = {
+	.get_link = ceph_encrypted_get_link,
+	.setattr = ceph_setattr,
+	.getattr = ceph_getattr,
+	.listxattr = ceph_listxattr,
+};
+
 int __ceph_setattr(struct inode *inode, struct iattr *attr)
 {
 	struct ceph_inode_info *ci = ceph_inode(inode);
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 5d5283552c03..d58296bd8235 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -939,6 +939,8 @@  struct ceph_mds_reply_dirfrag;
 struct ceph_acl_sec_ctx;
 
 extern const struct inode_operations ceph_file_iops;
+extern const struct inode_operations ceph_symlink_iops;
+extern const struct inode_operations ceph_encrypted_symlink_iops;
 
 extern struct inode *ceph_alloc_inode(struct super_block *sb);
 extern void ceph_evict_inode(struct inode *inode);