ACKED: [PATCH] eCryptfs: Improve statfs reporting

Tyler Hicks tyhicks at canonical.com
Fri Dec 23 15:42:07 UTC 2011


On 2011-12-23 11:36:55, Andy Whitcroft wrote:
> On Thu, Dec 22, 2011 at 03:22:28PM -0600, Tyler Hicks wrote:
> > statfs() calls on eCryptfs files returned the wrong filesystem type and,
> > when using filename encryption, the wrong maximum filename length.
> > 
> > If mount-wide filename encryption is enabled, the cipher block size and
> > the lower filesystem's max filename length will determine the max
> > eCryptfs filename length. Pre-tested, known good lengths are used when
> > the lower filesystem's namelen is 255 and a cipher with 8 or 16 byte
> > block sizes is used. In other, less common cases, we fall back to a safe
> > rounded-down estimate when determining the eCryptfs namelen.
> 
> I assume the point here is that userspace cannot do the right thing and
> use filenames which fit if we lie to it about how large a filename may
> be.  So that makes sense.

Correct

> 
> > BugLink: https://launchpad.net/bugs/885744
> > 
> > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> > Reported-by: Kees Cook <keescook at chromium.org>
> > Reviewed-by: Kees Cook <keescook at chromium.org>
> > Reviewed-by: John Johansen <john.johansen at canonical.com>
> > ---
> >  fs/ecryptfs/crypto.c          |   68 ++++++++++++++++++++++++++++++++++++----
> >  fs/ecryptfs/ecryptfs_kernel.h |    5 +++
> >  fs/ecryptfs/keystore.c        |    9 ++---
> >  fs/ecryptfs/super.c           |   14 ++++++++-
> >  4 files changed, 82 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> > index 2a83425..d3c8776 100644
> > --- a/fs/ecryptfs/crypto.c
> > +++ b/fs/ecryptfs/crypto.c
> > @@ -2026,6 +2026,17 @@ out:
> >  	return;
> >  }
> >  
> > +static size_t ecryptfs_max_decoded_size(size_t encoded_size)
> > +{
> > +	/* Not exact; conservatively long. Every block of 4
> > +	 * encoded characters decodes into a block of 3
> > +	 * decoded characters. This segment of code provides
> > +	 * the caller with the maximum amount of allocated
> > +	 * space that @dst will need to point to in a
> > +	 * subsequent call. */
> > +	return ((encoded_size + 1) * 3) / 4;
> > +}
> > +
> >  /**
> >   * ecryptfs_decode_from_filename
> >   * @dst: If NULL, this function only sets @dst_size and returns. If
> > @@ -2044,13 +2055,7 @@ ecryptfs_decode_from_filename(unsigned char *dst, size_t *dst_size,
> >  	size_t dst_byte_offset = 0;
> >  
> >  	if (dst == NULL) {
> > -		/* Not exact; conservatively long. Every block of 4
> > -		 * encoded characters decodes into a block of 3
> > -		 * decoded characters. This segment of code provides
> > -		 * the caller with the maximum amount of allocated
> > -		 * space that @dst will need to point to in a
> > -		 * subsequent call. */
> > -		(*dst_size) = (((src_size + 1) * 3) / 4);
> > +		(*dst_size) = ecryptfs_max_decoded_size(src_size);
> >  		goto out;
> >  	}
> >  	while (src_byte_offset < src_size) {
> > @@ -2275,3 +2280,52 @@ out_free:
> >  out:
> >  	return rc;
> >  }
> > +
> > +#define ENC_NAME_MAX_BLOCKLEN_8_OR_16	143
> > +
> > +int ecryptfs_set_f_namelen(long *namelen, long lower_namelen,
> > +			   struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
> > +{
> > +	struct blkcipher_desc desc;
> > +	struct mutex *tfm_mutex;
> > +	size_t cipher_blocksize;
> > +	int rc;
> > +
> > +	if (!(mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES)) {
> > +		(*namelen) = lower_namelen;
> > +		return 0;
> > +	}
> > +
> > +	rc = ecryptfs_get_tfm_and_mutex_for_cipher_name(&desc.tfm, &tfm_mutex,
> > +			mount_crypt_stat->global_default_fn_cipher_name);
> > +	if (unlikely(rc)) {
> > +		(*namelen) = 0;
> > +		return rc;
> > +	}
> > +
> > +	mutex_lock(tfm_mutex);
> > +	cipher_blocksize = crypto_blkcipher_blocksize(desc.tfm);
> > +	mutex_unlock(tfm_mutex);
> > +
> > +	/* Return an exact amount for the common cases */
> > +	if (lower_namelen == NAME_MAX
> > +	    && (cipher_blocksize == 8 || cipher_blocksize == 16)) {
> > +		(*namelen) = ENC_NAME_MAX_BLOCKLEN_8_OR_16;
> > +		return 0;
> > +	}
> > +
> > +	/* Return a safe estimate for the uncommon cases */
> > +	(*namelen) = lower_namelen;
> > +	(*namelen) -= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE;
> This is outside the encrypted part.
> > +	/* Since this is the max decoded size, subtract 1 "decoded block" len */
> > +	(*namelen) = ecryptfs_max_decoded_size(*namelen) - 3;
> > +	(*namelen) -= ECRYPTFS_TAG_70_MAX_METADATA_SIZE;
> > +	(*namelen) -= ECRYPTFS_FILENAME_MIN_RANDOM_PREPEND_BYTES;
> These are inside the encrypted part.
> > +	/* Worst case is that the filename is padded nearly a full block size */
> > +	(*namelen) -= cipher_blocksize - 1;
> 
> Ouch that is painful, we can waste most of an encoded block and that
> waste tip us into a new cipher block as well which is then wasted.
> Gah.  That seems like the worst case calculation.

Yes, it is painful. But keep in mind that we *won't* be using this
calculation 99% of the time. When eCryptfs is stacked on filesystems
with a max namelen on 255, we'll use the hardcoded value above. If we're
stacked on encfs, on top of sshfs, on top of fat16, we'll use this ugly
calculation. :)

> 
> I suspect that there is a slightly more accuract way to calculate this
> as rather than taking the mad decode block-1 and cypher_blocksize-1 off.
> Those presumably are aligned with respect to their starts and we might
> be able to use that to get closer.  Something like this (untested,
> approximate, ish, just to illustrate whats in my mind):
> 
> 	(*namelen) = ecryptfs_max_decoded_size(*namelen + 3) & ~3;
> and
> 	(*namelen) -= (*namelen + cipher_blocksize - 1) % cipher_blocksize;

Good points. I'll test these out before upstreaming the patch.

> 
> Anyhow, that isn't going to make this patch any less valid as an under
> estimate is significantly better that what we have now, this would be
> an optimisation.
> 
> Other than that perhaps personally I would have calculated the namelen in a
> local variable assigning it once if valid, it looks to do what is intended.

I had it stored in the private data of the superblock for a while. But
then I figured that we do these same calculations (plus a lot more) on
readdir() and cold lookup()'s, so doing it in statfs() isn't insane.

> 
> > +
> > +	if ((*namelen) < 0)
> > +		(*namelen) = 0;
> > +
> > +	return 0;
> > +}
> > diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> > index a9f29b1..7cad6b0 100644
> > --- a/fs/ecryptfs/ecryptfs_kernel.h
> > +++ b/fs/ecryptfs/ecryptfs_kernel.h
> > @@ -157,6 +157,9 @@ ecryptfs_get_key_payload_data(struct key *key)
> >  #define ECRYPTFS_NON_NULL 0x42 /* A reasonable substitute for NULL */
> >  #define MD5_DIGEST_SIZE 16
> >  #define ECRYPTFS_TAG_70_DIGEST_SIZE MD5_DIGEST_SIZE
> > +#define ECRYPTFS_TAG_70_MIN_METADATA_SIZE (1 + 1 + ECRYPTFS_SIG_SIZE + 1 + 1)
> > +#define ECRYPTFS_TAG_70_MAX_METADATA_SIZE (ECRYPTFS_TAG_70_MIN_METADATA_SIZE \
> > +					   + 1)
> >  #define ECRYPTFS_FEK_ENCRYPTED_FILENAME_PREFIX "ECRYPTFS_FEK_ENCRYPTED."
> >  #define ECRYPTFS_FEK_ENCRYPTED_FILENAME_PREFIX_SIZE 23
> >  #define ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX "ECRYPTFS_FNEK_ENCRYPTED."
> > @@ -696,6 +699,8 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> >  			     size_t *packet_size,
> >  			     struct ecryptfs_mount_crypt_stat *mount_crypt_stat,
> >  			     char *data, size_t max_packet_size);
> > +int ecryptfs_set_f_namelen(long *namelen, long lower_namelen,
> > +			   struct ecryptfs_mount_crypt_stat *mount_crypt_stat);
> >  int ecryptfs_derive_iv(char *iv, struct ecryptfs_crypt_stat *crypt_stat,
> >  		       loff_t offset);
> >  
> > diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> > index ac1ad48..06aab59 100644
> > --- a/fs/ecryptfs/keystore.c
> > +++ b/fs/ecryptfs/keystore.c
> > @@ -678,10 +678,7 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
> >  	 * Octets N3-N4: Block-aligned encrypted filename
> >  	 *  - Consists of a minimum number of random characters, a \0
> >  	 *    separator, and then the filename */
> > -	s->max_packet_size = (1                   /* Tag 70 identifier */
> > -			      + 3                 /* Max Tag 70 packet size */
> > -			      + ECRYPTFS_SIG_SIZE /* FNEK sig */
> > -			      + 1                 /* Cipher identifier */
> > +	s->max_packet_size = (ECRYPTFS_TAG_70_MAX_METADATA_SIZE
> >  			      + s->block_aligned_filename_size);
> >  	if (dest == NULL) {
> >  		(*packet_size) = s->max_packet_size;
> > @@ -933,10 +930,10 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
> >  		goto out;
> >  	}
> >  	s->desc.flags = CRYPTO_TFM_REQ_MAY_SLEEP;
> > -	if (max_packet_size < (1 + 1 + ECRYPTFS_SIG_SIZE + 1 + 1)) {
> > +	if (max_packet_size < ECRYPTFS_TAG_70_MIN_METADATA_SIZE) {
> >  		printk(KERN_WARNING "%s: max_packet_size is [%zd]; it must be "
> >  		       "at least [%d]\n", __func__, max_packet_size,
> > -			(1 + 1 + ECRYPTFS_SIG_SIZE + 1 + 1));
> > +		       ECRYPTFS_TAG_70_MIN_METADATA_SIZE);
> >  		rc = -EINVAL;
> >  		goto out;
> >  	}
> > diff --git a/fs/ecryptfs/super.c b/fs/ecryptfs/super.c
> > index dbd52d40..a04d9ef 100644
> > --- a/fs/ecryptfs/super.c
> > +++ b/fs/ecryptfs/super.c
> > @@ -30,6 +30,8 @@
> >  #include <linux/seq_file.h>
> >  #include <linux/file.h>
> >  #include <linux/crypto.h>
> > +#include <linux/statfs.h>
> > +#include <linux/magic.h>
> >  #include "ecryptfs_kernel.h"
> >  
> >  struct kmem_cache *ecryptfs_inode_info_cache;
> > @@ -103,10 +105,20 @@ static void ecryptfs_destroy_inode(struct inode *inode)
> >  static int ecryptfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> >  {
> >  	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> > +	int rc;
> >  
> >  	if (!lower_dentry->d_sb->s_op->statfs)
> >  		return -ENOSYS;
> > -	return lower_dentry->d_sb->s_op->statfs(lower_dentry, buf);
> 
> So before we just handed over the stat call to the lower filesystem and
> assumed that was good.

Right

> 
> > +
> > +	rc = lower_dentry->d_sb->s_op->statfs(lower_dentry, buf);
> > +	if (rc)
> > +		return rc;
> > +
> > +	buf->f_type = ECRYPTFS_SUPER_MAGIC;
> > +	rc = ecryptfs_set_f_namelen(&buf->f_namelen, buf->f_namelen,
> > +	       &ecryptfs_superblock_to_private(dentry->d_sb)->mount_crypt_stat);
> 
> Now we update the fstype and the namelen.  Seems much more sensible.
> The only other question is can we make some estimate of our overhead on
> size and reduce the maximum capacity perhaps; likely impossible to get a
> reasonable value and out of scope for this patch.

Our overhead per file is known, but since it is per file, I don't know
if it is possible to use that to reduce maximum capacity.

> 
> > +
> > +	return rc;
> >  }
> >  
> >  /**
> 
> Based on the above:
> 
> Acked-by: Andy Whitcroft <apw at canonical.com>

Thanks for the review!

Tyler

> 
> -apw
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20111223/b783b5d7/attachment.pgp>


More information about the kernel-team mailing list