eCryptfs: Improve statfs reporting

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.

BugLink: https://launchpad.net/bugs/885744

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Reported-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: John Johansen <john.johansen@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(-)

On 2011-12-22 15:22:28, 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.
> 
> BugLink: https://launchpad.net/bugs/885744
> 
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> Reported-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---

To provide some more background, this patch is an attempt to help with
the filename length limitation when using encrypted filenames with
eCryptfs.

The patch is currently living in the eCryptfs linux-next branch on
kernel.org, while waiting for the 3.3 merge window. I wanted to get it
in front of the Ubuntu kernel team before then, as the filename length
limitation has been a big problem for some applications and users.

This patch applies cleanly to current ubuntu-precise/master-next.

Tyler

>  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;
> +	/* 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;
> +	/* Worst case is that the filename is padded nearly a full block size */
> +	(*namelen) -= cipher_blocksize - 1;
> +
> +	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);
> +
> +	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);
> +
> +	return rc;
>  }
>  
>  /**
> -- 
> 1.7.5.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

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.

> BugLink: https://launchpad.net/bugs/885744
> 
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> Reported-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@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.

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;

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.

> +
> +	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.

> +
> +	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.

> +
> +	return rc;
>  }
>  
>  /**

Based on the above:

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

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@canonical.com>
> > Reported-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: John Johansen <john.johansen@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@canonical.com>

Thanks for the review!

Tyler

> 
> -apw

I think the commit subject could be a bit less disingenuous. Perhaps 
something like "eCryptfs: Report correct filesystem type and FNEK 
maximum filename length"

See subsequent comments inline.

On 12/22/2011 02:22 PM, 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.
>
> BugLink: https://launchpad.net/bugs/885744
>
> Signed-off-by: Tyler Hicks<tyhicks@canonical.com>
> Reported-by: Kees Cook<keescook@chromium.org>
> Reviewed-by: Kees Cook<keescook@chromium.org>
> Reviewed-by: John Johansen<john.johansen@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

Perhaps some explanation about how you calculated this magic number.

> +
> +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;
> +	}

How different are the answers for the common case vs the uncommon cases? 
I kind of object to having 2 code paths for this, one of which is 
unlikely to get exercised very often and has largely untested complexity.

> +
> +	/* Return a safe estimate for the uncommon cases */
> +	(*namelen) = lower_namelen;
> +	(*namelen) -= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE;
> +	/* 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;
> +	/* Worst case is that the filename is padded nearly a full block size */
> +	(*namelen) -= cipher_blocksize - 1;
> +
> +	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)

You should update the comment block for this function so that it 
mentions the possible side effect on buf->f_namelen.

>   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);
> +
> +	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);
> +
> +	return rc;
>   }
>
>   /**

On 12/23/2011 08:49 AM, Tim Gardner wrote:

<snip>

>> + /* 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;
>> + }
>
> How different are the answers for the common case vs the uncommon cases?
> I kind of object to having 2 code paths for this, one of which is
> unlikely to get exercised very often and has largely untested complexity.
>
>> +
>> + /* Return a safe estimate for the uncommon cases */
>> + (*namelen) = lower_namelen;
>> + (*namelen) -= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE;
>> + /* 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;
>> + /* Worst case is that the filename is padded nearly a full block
>> size */
>> + (*namelen) -= cipher_blocksize - 1;
>> +

The difference is 143 vs 127 when mounted on ext4.

On 2011-12-23 08:49:13, Tim Gardner wrote:
> I think the commit subject could be a bit less disingenuous. Perhaps
> something like "eCryptfs: Report correct filesystem type and FNEK
> maximum filename length"

It could use some more detail. I'll fix that.

> 
> See subsequent comments inline.
> 
> On 12/22/2011 02:22 PM, 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.
> >
> >BugLink: https://launchpad.net/bugs/885744
> >
> >Signed-off-by: Tyler Hicks<tyhicks@canonical.com>
> >Reported-by: Kees Cook<keescook@chromium.org>
> >Reviewed-by: Kees Cook<keescook@chromium.org>
> >Reviewed-by: John Johansen<john.johansen@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
> 
> Perhaps some explanation about how you calculated this magic number.

Through testing. I'll add a comment.

> 
> >+
> >+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;
> >+	}
> 
> How different are the answers for the common case vs the uncommon
> cases? I kind of object to having 2 code paths for this, one of
> which is unlikely to get exercised very often and has largely
> untested complexity.

The method used for the uncommon cases is actually exercised often
because it is composed of the estimations used throughout the filename
encryption code paths. If any of them are wrong, then we are overflowing
buffers.

However, I see what you're getting at and I'll try to come up with a
single code path to set f_namelen.

Tyler

> 
> >+
> >+	/* Return a safe estimate for the uncommon cases */
> >+	(*namelen) = lower_namelen;
> >+	(*namelen) -= ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE;
> >+	/* 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;
> >+	/* Worst case is that the filename is padded nearly a full block size */
> >+	(*namelen) -= cipher_blocksize - 1;
> >+
> >+	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)
> 
> You should update the comment block for this function so that it
> mentions the possible side effect on buf->f_namelen.
> 
> >  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);
> >+
> >+	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);
> >+
> >+	return rc;
> >  }
> >
> >  /**
> 
> 
> -- 
> Tim Gardner tim.gardner@canonical.com

On Fri, Dec 23, 2011 at 09:42:07AM -0600, Tyler Hicks wrote:

> > > +	/* 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;
[...]
> > 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.

Ahh I didn't mean store it for all time, just calculate the thing in
long x, and assign X to (*namelen) at the end.

-apw

Applied to Precise. If it appears to work, then lets think about 
backporting as far as Lucid.

rtg