FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Ubuntu > Ubuntu Kernel Team

 
 
LinkBack Thread Tools
 
Old 03-23-2009, 12:10 AM
 
Default LP #345766 - NULL crypt_stat dereference during lookup

 
Old 03-23-2009, 07:52 AM
Andy Whitcroft
 
Default LP #345766 - NULL crypt_stat dereference during lookup

On Sun, Mar 22, 2009 at 07:10:19PM -0600, Tim Gardner wrote:
> From ddc873efd85138c99b34be4ab9552c1d72b10e94 Mon Sep 17 00:00:00 2001
> From: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
> Date: Fri, 20 Mar 2009 02:23:57 -0500
> Subject: [PATCH] eCryptfs: NULL crypt_stat dereference during lookup
>
> Bug: #345766
>
> 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>
> Cc: Dan Carpenter <error27@gmail.com>
> Cc: Serge Hallyn <serue@us.ibm.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (cherry picked from commit 2aac0cf88681bfa092f731553bc7fbd23516be73)
>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Looks good to me.

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

-apw

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 03-23-2009, 08:49 AM
Stefan Bader
 
Default LP #345766 - NULL crypt_stat dereference during lookup

Tim Gardner wrote:
> From ddc873efd85138c99b34be4ab9552c1d72b10e94 Mon Sep 17 00:00:00 2001
> From: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
> Date: Fri, 20 Mar 2009 02:23:57 -0500
> Subject: [PATCH] eCryptfs: NULL crypt_stat dereference during lookup
>
> Bug: #345766
>
> 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>
> Cc: Dan Carpenter <error27@gmail.com>
> Cc: Serge Hallyn <serue@us.ibm.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (cherry picked from commit 2aac0cf88681bfa092f731553bc7fbd23516be73)
>
> Signed-off-by: Tim Gardner <tim.gardner@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 bdca1f4..1607e09 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -2208,17 +2208,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 fa6f1f4..1d0b5f8 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 17a96ec..82378fb 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_dentr y, lower_dentry,
> - crypt_stat, ecryptfs_dir_inode,
> + ecryptfs_dir_inode,
> ecryptfs_nd);
> goto out;
> out_d_drop:

ACK. The patch itself looks ok to me. I assume it has been tested as well.

--

When all other means of communication fail, try words!



--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 
Old 03-23-2009, 12:21 PM
Tim Gardner
 
Default LP #345766 - NULL crypt_stat dereference during lookup

Stefan Bader wrote:
> Tim Gardner wrote:
>> From ddc873efd85138c99b34be4ab9552c1d72b10e94 Mon Sep 17 00:00:00 2001
>> From: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
>> Date: Fri, 20 Mar 2009 02:23:57 -0500
>> Subject: [PATCH] eCryptfs: NULL crypt_stat dereference during lookup
>>
>> Bug: #345766
>>
>> 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>
>> Cc: Dan Carpenter <error27@gmail.com>
>> Cc: Serge Hallyn <serue@us.ibm.com>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> (cherry picked from commit 2aac0cf88681bfa092f731553bc7fbd23516be73)
>>
>> Signed-off-by: Tim Gardner <tim.gardner@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 bdca1f4..1607e09 100644
>> --- a/fs/ecryptfs/crypto.c
>> +++ b/fs/ecryptfs/crypto.c
>> @@ -2208,17 +2208,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 fa6f1f4..1d0b5f8 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 17a96ec..82378fb 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_dentr y, lower_dentry,
>> - crypt_stat, ecryptfs_dir_inode,
>> + ecryptfs_dir_inode,
>> ecryptfs_nd);
>> goto out;
>> out_d_drop:
>
> ACK. The patch itself looks ok to me. I assume it has been tested as well.
>

Yes - I asked Dustin Kirkland to confirm what his Acked-by implied. He
said he'd built a vanilla 2.6.29-rc kernel with this patch in order to
prove that it fixed the problem. I've verified that Jaunty fs/ecryptfs
is within a smidge of being identical to upstream, so I'm comfortable
that this patch is correct.

rtg
--
Tim Gardner tim.gardner@canonical.com

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team
 

Thread Tools




All times are GMT. The time now is 10:06 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org