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.sig>
More information about the kernel-team
mailing list