[20/22] ext4 crypto: Add symlink encryption

Change-Id: Ic92ebe4c615721650ccaf16b3175c2f4e931af2d
Signed-off-by: Uday Savagaonkar <savagaon@google.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/ext4_crypto.h |  20 ++++++++++
 fs/ext4/namei.c       |  63 +++++++++++++++++++++++++++---
 fs/ext4/symlink.c     | 104 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 180 insertions(+), 7 deletions(-)

On Apr 2, 2015, at 4:10 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> Change-Id: Ic92ebe4c615721650ccaf16b3175c2f4e931af2d
> Signed-off-by: Uday Savagaonkar <savagaon@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> fs/ext4/ext4_crypto.h |  20 ++++++++++
> fs/ext4/namei.c       |  63 +++++++++++++++++++++++++++---
> fs/ext4/symlink.c     | 104 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 180 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/ext4_crypto.h b/fs/ext4/ext4_crypto.h
> index 68e95d8..4597530 100644
> --- a/fs/ext4/ext4_crypto.h
> +++ b/fs/ext4/ext4_crypto.h
> @@ -117,4 +117,24 @@ struct ext4_fname_crypto_ctx {
> 	unsigned ctfm_key_is_ready : 1;
> };
> 
> +/**
> + * For encrypted symlinks, the ciphertext length is stored at the beginning
> + * of the string in little-endian format.
> + */
> +struct ext4_encrypted_symlink_data {
> +	__le32 len;
> +	char encrypted_path[1];
> +} __attribute__((__packed__));

We can't have a symlink size larger than a block (or possibly PATH_MAX),
can we?  That would allow using __le16 for the symlink length, and
checkpatch.pl will complain about __attribute__((__packed__)) and
request the use of __packed instead.

> +
> +/**
> + * This function is used to calculate the disk space required to
> + * store a filename of length l in encrypted symlink format.
> + */
> +static inline u32 encrypted_symlink_data_len(u32 l)
> +{
> +	return ((l + EXT4_CRYPTO_BLOCK_SIZE - 1) / EXT4_CRYPTO_BLOCK_SIZE)
> +		* EXT4_CRYPTO_BLOCK_SIZE
> +		+ sizeof(struct ext4_encrypted_symlink_data) - 1;

Coding style has operators at the end of the line instead of the start.

> +}
> +
> #endif	/* _EXT4_CRYPTO_H */
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 80a3979..57db793 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3198,14 +3198,31 @@ static int ext4_symlink(struct inode *dir,
> 	struct inode *inode;
> 	int l, err, retries = 0;
> 	int credits;
> +	bool encryption_required = false;
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> +	int l2;
> +	struct ext4_fname_crypto_ctx *ctx = NULL;
> +	struct qstr istr;
> +	struct ext4_str ostr;
> +	struct ext4_encrypted_symlink_data *sd = NULL;
> +	struct ext4_sb_info *sbi = EXT4_SB(dir->i_sb);
> +#endif
> 
> -	l = strlen(symname)+1;
> +	l = strlen(symname) + 1;
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> +	l2 = encrypted_symlink_data_len(l - 1);
> +	encryption_required = (ext4_encrypted_inode(dir) ||
> +			       unlikely(sbi->s_mount_flags &
> +					EXT4_MF_TEST_DUMMY_ENCRYPTION));
> +	if (encryption_required && l2 > dir->i_sb->s_blocksize)
> +#else
> 	if (l > dir->i_sb->s_blocksize)
> +#endif
> 		return -ENAMETOOLONG;
> 
> 	dquot_initialize(dir);
> 
> -	if (l > EXT4_N_BLOCKS * 4) {
> +	if ((l > EXT4_N_BLOCKS * 4) || encryption_required) {
> 		/*
> 		 * For non-fast symlinks, we just allocate inode and put it on
> 		 * orphan list in the first transaction => we need bitmap,
> @@ -3233,7 +3250,7 @@ retry:
> 	if (IS_ERR(inode))
> 		goto out_stop;
> 
> -	if (l > EXT4_N_BLOCKS * 4) {
> +	if ((l > EXT4_N_BLOCKS * 4) || encryption_required) {
> 		inode->i_op = &ext4_symlink_inode_operations;
> 		ext4_set_aops(inode);
> 		/*
> @@ -3251,9 +3268,40 @@ retry:
> 		ext4_journal_stop(handle);
> 		if (err)
> 			goto err_drop_inode;
> -		err = __page_symlink(inode, symname, l, 1);
> -		if (err)
> -			goto err_drop_inode;
> +		if (!encryption_required) {
> +			err = __page_symlink(inode, symname, l, 1);
> +			if (err)
> +				goto err_drop_inode;
> +		}
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> +		else {
> +			sd = kmalloc(l2 + 1, GFP_NOFS);
> +			if (!sd) {
> +				err = -ENOMEM;
> +				goto err_drop_inode;
> +			}
> +			sd->encrypted_path[l2] = '\0';
> +			err = ext4_inherit_context(dir, inode);
> +			ctx = ext4_get_fname_crypto_ctx(
> +				inode, inode->i_sb->s_blocksize);
> +			if (IS_ERR_OR_NULL(ctx)) {
> +				/* We just set the policy, so ctx should
> +				   not be NULL */
> +				err = (ctx == NULL) ? -EIO : PTR_ERR(ctx);
> +				goto err_drop_inode;
> +			}
> +			istr.name = (const unsigned char *) symname;
> +			istr.len = l - 1;
> +			ostr.name = sd->encrypted_path;
> +			err = ext4_fname_usr_to_disk(ctx, &istr, &ostr);
> +			ext4_put_fname_crypto_ctx(&ctx);
> +			if (err < 0)
> +				goto err_drop_inode;
> +			sd->len = cpu_to_le32(ostr.len);
> +			err = __page_symlink(inode, (char *)sd, l2 + 1, 1);
> +

No blank line at the end of this scope.

Can "sd" moved inside this scope?  It doesn't appear to be used outside.

> +		}
> +#endif
> 		/*
> 		 * Now inode is being linked into dir (EXT4_DATA_TRANS_BLOCKS
> 		 * + EXT4_INDEX_EXTRA_TRANS_BLOCKS), inode is also modified
> @@ -3293,6 +3341,9 @@ out_stop:
> err_drop_inode:
> 	unlock_new_inode(inode);
> 	iput(inode);
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> +	kfree(sd);
> +#endif
> 	return err;
> }
> 
> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index ff37119..d788891 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -22,9 +22,106 @@
> #include <linux/namei.h>
> #include "ext4.h"
> #include "xattr.h"
> +#include "ext4_crypto.h"
> 
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> static void *ext4_follow_link(struct dentry *dentry, struct nameidata *nd)
> {
> +	struct page *cpage = NULL;
> +	char *caddr, *paddr;
> +	struct ext4_str cstr, pstr;
> +	struct inode *inode = dentry->d_inode;
> +	struct ext4_fname_crypto_ctx *ctx = NULL;
> +	struct ext4_encrypted_symlink_data *sd;
> +	loff_t size = min(inode->i_size, (loff_t) PAGE_SIZE-1);

No space after typecast.  Should this use min_t() instead?

> +	int res;
> +	u32 plen, plen2;
> +
> +	ctx = ext4_get_fname_crypto_ctx(inode, inode->i_sb->s_blocksize);
> +	if (IS_ERR(ctx))
> +		return ctx;
> +
> +	cpage = read_mapping_page(inode->i_mapping, 0, NULL);
> +	if (IS_ERR(cpage)) {
> +		ext4_put_fname_crypto_ctx(&ctx);
> +		return cpage;
> +	}
> +	caddr = kmap(cpage);
> +	caddr[size] = 0;
> +
> +	if (!ctx) {
> +		/* Symlink is unencrypted */
> +		plen = strnlen((char *)caddr, inode->i_sb->s_blocksize);
> +		plen2 = (plen < inode->i_sb->s_blocksize) ? plen + 1 : plen;
> +		paddr = kmalloc(plen2, GFP_NOFS);
> +		if (!paddr) {
> +			ext4_put_fname_crypto_ctx(&ctx);
> +			kunmap(cpage);
> +			page_cache_release(cpage);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		memcpy(paddr, caddr, plen);
> +		if (plen < inode->i_sb->s_blocksize)
> +			paddr[plen] = '\0';
> +	} else {
> +		/* Symlink is encrypted */
> +		sd = (struct ext4_encrypted_symlink_data *)caddr;
> +		cstr.name = sd->encrypted_path;
> +		cstr.len  = le32_to_cpu(sd->len);
> +		if ((cstr.len + sizeof(struct ext4_encrypted_symlink_data) - 1)
> +			> inode->i_sb->s_blocksize) {

Operator at the end of the previous line.
Continued line should align after '(' from previous line.

> +			/* Symlink data on the disk is corrupted */
> +			kunmap(cpage);
> +			page_cache_release(cpage);
> +			return ERR_PTR(-EIO);
> +		}
> +		plen = (cstr.len < EXT4_FNAME_CRYPTO_DIGEST_SIZE*2)
> +			? EXT4_FNAME_CRYPTO_DIGEST_SIZE*2
> +			: cstr.len;

Operators at the end of the previous line.

> +		paddr = kmalloc(plen + 1, GFP_NOFS);
> +		if (!paddr) {
> +			ext4_put_fname_crypto_ctx(&ctx);
> +			kunmap(cpage);
> +			page_cache_release(cpage);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		pstr.name = paddr;
> +		res = _ext4_fname_disk_to_usr(ctx, &cstr, &pstr);
> +		if (res < 0) {
> +			ext4_put_fname_crypto_ctx(&ctx);
> +			kunmap(cpage);
> +			page_cache_release(cpage);
> +			kfree(paddr);
> +			return ERR_PTR(res);
> +		}
> +		/* Null-terminate the name */
> +		if (res <= plen)
> +			paddr[res] = '\0';
> +	}
> +	nd_set_link(nd, paddr);
> +	ext4_put_fname_crypto_ctx(&ctx);
> +	return cpage;
> +}
> +
> +static void ext4_put_link(struct dentry *dentry, struct nameidata *nd,
> +			  void *cookie)
> +{
> +	struct page *page = cookie;
> +	char *buf = nd_get_link(nd);
> +
> +	if (page) {
> +		kunmap(page);
> +		page_cache_release(page);
> +	}
> +	if (buf) {
> +		nd_set_link(nd, NULL);
> +		kfree(buf);
> +	}
> +}
> +#endif
> +
> +static void *ext4_follow_fast_link(struct dentry *dentry, struct nameidata *nd)
> +{
> 	struct ext4_inode_info *ei = EXT4_I(dentry->d_inode);
> 	nd_set_link(nd, (char *) ei->i_data);
> 	return NULL;
> @@ -32,8 +129,13 @@ static void *ext4_follow_link(struct dentry *dentry, struct nameidata *nd)
> 
> const struct inode_operations ext4_symlink_inode_operations = {
> 	.readlink	= generic_readlink,
> +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> +	.follow_link    = ext4_follow_link,
> +	.put_link       = ext4_put_link,
> +#else

What about instantiating a different inode_operations struct for
encrypted symlinks?  That avoids the need to handle unencrypted
symlinks inline in ext4_follow_link(), and avoids overhead in the
more common unencrypted symlink case.  In that case, the name of
the new function should be ext4_follow_crypto_link() or similar.

> 	.follow_link	= page_follow_link_light,
> 	.put_link	= page_put_link,
> +#endif
> 	.setattr	= ext4_setattr,
> 	.setxattr	= generic_setxattr,
> 	.getxattr	= generic_getxattr,
> @@ -43,7 +145,7 @@ const struct inode_operations ext4_symlink_inode_operations = {
> 
> const struct inode_operations ext4_fast_symlink_inode_operations = {
> 	.readlink	= generic_readlink,
> -	.follow_link	= ext4_follow_link,
> +	.follow_link    = ext4_follow_fast_link,
> 	.setattr	= ext4_setattr,
> 	.setxattr	= generic_setxattr,
> 	.getxattr	= generic_getxattr,
> -- 
> 2.3.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Apr 08, 2015 at 11:58:02AM -0600, Andreas Dilger wrote:
> On Apr 2, 2015, at 4:10 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> > +/**
> > + * For encrypted symlinks, the ciphertext length is stored at the beginning
> > + * of the string in little-endian format.
> > + */
> > +struct ext4_encrypted_symlink_data {
> > +	__le32 len;
> > +	char encrypted_path[1];
> > +} __attribute__((__packed__));
> 
> We can't have a symlink size larger than a block (or possibly PATH_MAX),
> can we?  That would allow using __le16 for the symlink length, and
> checkpatch.pl will complain about __attribute__((__packed__)) and
> request the use of __packed instead.

Yes, although it's a bit pointless since right now we don't support
fast encrypted symlinks.  What we should probably do is to switch to
using CTS, and then add fast encrypted symlinks, and then use __le16
here.

When did checkpatch start complaining about
__attribute__((__packed__))?  It's not complaining for me.

> > +static inline u32 encrypted_symlink_data_len(u32 l)
> > +{
> > +	return ((l + EXT4_CRYPTO_BLOCK_SIZE - 1) / EXT4_CRYPTO_BLOCK_SIZE)
> > +		* EXT4_CRYPTO_BLOCK_SIZE
> > +		+ sizeof(struct ext4_encrypted_symlink_data) - 1;
> 
> Coding style has operators at the end of the line instead of the start.

Thanks, fixed.

> > +		else {
> > +			sd = kmalloc(l2 + 1, GFP_NOFS);
    			   ...
> 
> Can "sd" moved inside this scope?  It doesn't appear to be used outside.

Good point, thanks.

> > diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> > index ff37119..d788891 100644
> > --- a/fs/ext4/symlink.c
> > +++ b/fs/ext4/symlink.c
> > @@ -22,9 +22,106 @@
> > #include <linux/namei.h>
> > #include "ext4.h"
> > #include "xattr.h"
> > +#include "ext4_crypto.h"
> > 
> > +#ifdef CONFIG_EXT4_FS_ENCRYPTION
> > static void *ext4_follow_link(struct dentry *dentry, struct nameidata *nd)
> > {
> > +	struct page *cpage = NULL;
> > +	char *caddr, *paddr;
> > +	struct ext4_str cstr, pstr;
> > +	struct inode *inode = dentry->d_inode;
> > +	struct ext4_fname_crypto_ctx *ctx = NULL;
> > +	struct ext4_encrypted_symlink_data *sd;
> > +	loff_t size = min(inode->i_size, (loff_t) PAGE_SIZE-1);
> 
> No space after typecast.  Should this use min_t() instead?

I'll change it to use min_t, thanks.

> > +		if ((cstr.len + sizeof(struct ext4_encrypted_symlink_data) - 1)
> > +			> inode->i_sb->s_blocksize) {
> 
> Operator at the end of the previous line.
> Continued line should align after '(' from previous line.

Thanks, will fix.

> > +		plen = (cstr.len < EXT4_FNAME_CRYPTO_DIGEST_SIZE*2)
> > +			? EXT4_FNAME_CRYPTO_DIGEST_SIZE*2
> > +			: cstr.len;
> 
> Operators at the end of the previous line.

Thanks, will fix.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html