Messages in this thread | | | Date | Fri, 20 Mar 2009 17:43:06 -0500 | Subject | Re: [PATCH] eCryptfs: NULL crypt_stat dereference during lookup | From | Dustin Kirkland <> |
| |
On Fri, Mar 20, 2009 at 2:23 AM, Tyler Hicks <tyhicks@linux.vnet.ibm.com> wrote: > If ecryptfs_encrypted_view or ecryptfs_xattr_metadata were being > specified as mount options, a NULL pointer dereference of crypt_stat > was possible during lookup. > > This patch moves the crypt_stat assignment into > ecryptfs_lookup_and_interpose_lower(), ensuring that crypt_stat > will not be NULL before we attempt to dereference it. > > Thanks to Dan Carpenter and his static analysis tool, smatch, for > finding this bug. > > Signed-off-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
Acked-by: Dustin Kirkland <kirkland@canonical.com>
> --- > fs/ecryptfs/crypto.c | 10 ++++++---- > fs/ecryptfs/ecryptfs_kernel.h | 1 - > fs/ecryptfs/inode.c | 32 ++++++++++++-------------------- > 3 files changed, 18 insertions(+), 25 deletions(-) > > diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c > index 75bee99..8b65f28 100644 > --- a/fs/ecryptfs/crypto.c > +++ b/fs/ecryptfs/crypto.c > @@ -2221,17 +2221,19 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name, > struct dentry *ecryptfs_dir_dentry, > const char *name, size_t name_size) > { > + struct ecryptfs_mount_crypt_stat *mount_crypt_stat = > + &ecryptfs_superblock_to_private( > + ecryptfs_dir_dentry->d_sb)->mount_crypt_stat; > char *decoded_name; > size_t decoded_name_size; > size_t packet_size; > int rc = 0; > > - if ((name_size > ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE) > + if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) > + && !(mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED) > + && (name_size > ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE) > && (strncmp(name, ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX, > ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE) == 0)) { > - struct ecryptfs_mount_crypt_stat *mount_crypt_stat = > - &ecryptfs_superblock_to_private( > - ecryptfs_dir_dentry->d_sb)->mount_crypt_stat; > const char *orig_name = name; > size_t orig_name_size = name_size; > > diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h > index eb2267e..ac749d4 100644 > --- a/fs/ecryptfs/ecryptfs_kernel.h > +++ b/fs/ecryptfs/ecryptfs_kernel.h > @@ -620,7 +620,6 @@ int ecryptfs_interpose(struct dentry *hidden_dentry, > u32 flags); > int ecryptfs_lookup_and_interpose_lower(struct dentry *ecryptfs_dentry, > struct dentry *lower_dentry, > - struct ecryptfs_crypt_stat *crypt_stat, > struct inode *ecryptfs_dir_inode, > struct nameidata *ecryptfs_nd); > int ecryptfs_decode_and_decrypt_filename(char **decrypted_name, > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index 5697899..55b3145 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -246,7 +246,6 @@ out: > */ > int ecryptfs_lookup_and_interpose_lower(struct dentry *ecryptfs_dentry, > struct dentry *lower_dentry, > - struct ecryptfs_crypt_stat *crypt_stat, > struct inode *ecryptfs_dir_inode, > struct nameidata *ecryptfs_nd) > { > @@ -254,6 +253,7 @@ int ecryptfs_lookup_and_interpose_lower(struct dentry *ecryptfs_dentry, > struct vfsmount *lower_mnt; > struct inode *lower_inode; > struct ecryptfs_mount_crypt_stat *mount_crypt_stat; > + struct ecryptfs_crypt_stat *crypt_stat; > char *page_virt = NULL; > u64 file_size; > int rc = 0; > @@ -314,6 +314,11 @@ int ecryptfs_lookup_and_interpose_lower(struct dentry *ecryptfs_dentry, > goto out_free_kmem; > } > } > + crypt_stat = &ecryptfs_inode_to_private( > + ecryptfs_dentry->d_inode)->crypt_stat; > + /* TODO: lock for crypt_stat comparison */ > + if (!(crypt_stat->flags & ECRYPTFS_POLICY_APPLIED)) > + ecryptfs_set_default_sizes(crypt_stat); > rc = ecryptfs_read_and_validate_header_region(page_virt, > ecryptfs_dentry->d_inode); > if (rc) { > @@ -362,9 +367,7 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, > { > char *encrypted_and_encoded_name = NULL; > size_t encrypted_and_encoded_name_size; > - struct ecryptfs_crypt_stat *crypt_stat = NULL; > struct ecryptfs_mount_crypt_stat *mount_crypt_stat = NULL; > - struct ecryptfs_inode_info *inode_info; > struct dentry *lower_dir_dentry, *lower_dentry; > int rc = 0; > > @@ -388,26 +391,15 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, > } > if (lower_dentry->d_inode) > goto lookup_and_interpose; > - inode_info = ecryptfs_inode_to_private(ecryptfs_dentry->d_inode); > - if (inode_info) { > - crypt_stat = &inode_info->crypt_stat; > - /* TODO: lock for crypt_stat comparison */ > - if (!(crypt_stat->flags & ECRYPTFS_POLICY_APPLIED)) > - ecryptfs_set_default_sizes(crypt_stat); > - } > - if (crypt_stat) > - mount_crypt_stat = crypt_stat->mount_crypt_stat; > - else > - mount_crypt_stat = &ecryptfs_superblock_to_private( > - ecryptfs_dentry->d_sb)->mount_crypt_stat; > - if (!(crypt_stat && (crypt_stat->flags & ECRYPTFS_ENCRYPT_FILENAMES)) > - && !(mount_crypt_stat && (mount_crypt_stat->flags > - & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES))) > + mount_crypt_stat = &ecryptfs_superblock_to_private( > + ecryptfs_dentry->d_sb)->mount_crypt_stat; > + if (!(mount_crypt_stat > + && (mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES))) > goto lookup_and_interpose; > dput(lower_dentry); > rc = ecryptfs_encrypt_and_encode_filename( > &encrypted_and_encoded_name, &encrypted_and_encoded_name_size, > - crypt_stat, mount_crypt_stat, ecryptfs_dentry->d_name.name, > + NULL, mount_crypt_stat, ecryptfs_dentry->d_name.name, > ecryptfs_dentry->d_name.len); > if (rc) { > printk(KERN_ERR "%s: Error attempting to encrypt and encode " > @@ -426,7 +418,7 @@ static struct dentry *ecryptfs_lookup(struct inode *ecryptfs_dir_inode, > } > lookup_and_interpose: > rc = ecryptfs_lookup_and_interpose_lower(ecryptfs_dentry, lower_dentry, > - crypt_stat, ecryptfs_dir_inode, > + ecryptfs_dir_inode, > ecryptfs_nd); > goto out; > out_d_drop: > -- > 1.5.3.7 > >
-- :-Dustin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |