Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: Eric Biggers <ebiggers@kernel.org>
Cc: ceph-devel@vger.kernel.org, linux-fscrypt@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [RFC PATCH v3 16/16] ceph: create symlinks with encrypted and base64-encoded targets
Date: Tue, 15 Sep 2020 10:05:53 -0400	[thread overview]
Message-ID: <5bdc7608df4ff480c07eb6a0e85514ebd986e5d9.camel@kernel.org> (raw)
In-Reply-To: <20200915020725.GM899@sol.localdomain>

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.
-- 
Jeff Layton <jlayton@kernel.org>


  reply	other threads:[~2020-09-16  0:28 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-14 19:16 [RFC PATCH v3 00/16] ceph+fscrypt: context, filename and symlink support Jeff Layton
2020-09-14 19:16 ` [RFC PATCH v3 01/16] vfs: export new_inode_pseudo Jeff Layton
2020-09-14 23:33   ` Eric Biggers
2020-09-23  3:41   ` Al Viro
2020-09-23 11:19     ` Jeff Layton
2020-09-14 19:16 ` [RFC PATCH v3 02/16] fscrypt: export fscrypt_base64_encode and fscrypt_base64_decode Jeff Layton
2020-09-14 23:44   ` Eric Biggers
2020-09-14 19:16 ` [RFC PATCH v3 03/16] fscrypt: export fscrypt_d_revalidate Jeff Layton
2020-09-15  0:04   ` Eric Biggers
2020-09-14 19:16 ` [RFC PATCH v3 04/16] fscrypt: add fscrypt_context_for_new_inode Jeff Layton
2020-09-15  0:15   ` Eric Biggers
2020-09-14 19:16 ` [RFC PATCH v3 05/16] fscrypt: make fscrypt_fname_disk_to_usr return whether result is nokey name Jeff Layton
2020-09-15  0:23   ` Eric Biggers
2020-09-14 19:16 ` [RFC PATCH v3 06/16] ceph: add fscrypt ioctls Jeff Layton
2020-09-15  0:45   ` Eric Biggers
2020-09-15 12:08     ` Jeff Layton
2020-09-14 19:16 ` [RFC PATCH v3 07/16] ceph: crypto context handling for ceph Jeff Layton
2020-09-15  1:00   ` Eric Biggers
2020-09-14 19:16 ` [RFC PATCH v3 08/16] ceph: implement -o test_dummy_encryption mount option Jeff Layton
2020-09-15  1:23   ` Eric Biggers
2020-09-16 12:49     ` Jeff Layton
2020-09-14 19:17 ` [RFC PATCH v3 09/16] ceph: preallocate inode for ops that may create one Jeff Layton
2020-09-15  1:30   ` Eric Biggers
2020-09-16 12:41     ` Jeff Layton
2020-09-14 19:17 ` [RFC PATCH v3 10/16] ceph: add routine to create context prior to RPC Jeff Layton
2020-09-15  1:37   ` Eric Biggers
2020-09-16 12:18     ` Jeff Layton
2020-09-14 19:17 ` [RFC PATCH v3 11/16] ceph: make ceph_msdc_build_path use ref-walk Jeff Layton
2020-09-14 19:17 ` [RFC PATCH v3 12/16] ceph: add encrypted fname handling to ceph_mdsc_build_path Jeff Layton
2020-09-15  1:41   ` Eric Biggers
2020-09-16 12:30     ` Jeff Layton
2020-09-16 17:36       ` Eric Biggers
2020-09-16 18:04         ` Jeff Layton
2020-09-16 18:42           ` Eric Biggers
2020-09-14 19:17 ` [RFC PATCH v3 13/16] ceph: make d_revalidate call fscrypt revalidator for encrypted dentries Jeff Layton
2020-09-14 19:17 ` [RFC PATCH v3 14/16] ceph: add support to readdir for encrypted filenames Jeff Layton
2020-09-15  1:57   ` Eric Biggers
2020-09-15 13:27     ` Jeff Layton
2020-09-15 20:40       ` Eric Biggers
2020-09-16 12:16         ` Jeff Layton
2020-09-14 19:17 ` [RFC PATCH v3 15/16] ceph: add fscrypt support to ceph_fill_trace Jeff Layton
2020-09-14 19:17 ` [RFC PATCH v3 16/16] ceph: create symlinks with encrypted and base64-encoded targets Jeff Layton
2020-09-15  2:07   ` Eric Biggers
2020-09-15 14:05     ` Jeff Layton [this message]
2020-09-15 20:49       ` Eric Biggers
2020-09-16 12:15         ` Jeff Layton
2020-09-15  2:13 ` [RFC PATCH v3 00/16] ceph+fscrypt: context, filename and symlink support Eric Biggers
2020-09-15 13:38   ` Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5bdc7608df4ff480c07eb6a0e85514ebd986e5d9.camel@kernel.org \
    --to=jlayton@kernel.org \
    --cc=ceph-devel@vger.kernel.org \
    --cc=ebiggers@kernel.org \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --subject='Re: [RFC PATCH v3 16/16] ceph: create symlinks with encrypted and base64-encoded targets' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).