| Message ID | 4E9454D4.5010500@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 11.10.2011 15:38, Tim Gardner wrote: > On 10/11/2011 01:53 PM, Tim Gardner wrote: >> On 10/10/2011 04:26 PM, Tyler Hicks wrote: >>> On 2011-10-10 07:16:59, Leann Ogasawara wrote: >>>> On Mon, 2011-10-10 at 14:46 +0100, Tim Gardner wrote: >>>>> On 10/10/2011 02:42 PM, Leann Ogasawara wrote: >>>>>> On Sun, 2011-10-09 at 05:12 -0600, Tim Gardner wrote: >>>>>>> From 27ed7cb2b00512e81016419715c1d9b6794b06ae Mon Sep 17 00:00:00 >>>>>>> 2001 >>>>>>> From: Tyler Hicks<tyhicks@canonical.com> >>>>>>> Date: Fri, 7 Oct 2011 15:54:26 -0500 >>>>>>> Subject: [PATCH] eCryptfs: Clear ECRYPTFS_NEW_FILE flag during >>>>>>> truncate >>>>>>> >>>>>>> BugLink: http://bugs.launchpad.net/bugs/745836 >>>>>>> >>>>>>> The ECRYPTFS_NEW_FILE crypt_stat flag is set upon creation of a new >>>>>>> eCryptfs file. When the flag is set, eCryptfs reads directly from the >>>>>>> lower filesystem when bringing a page up to date. This means that no >>>>>>> offset translation (for the eCryptfs file metadata in the lower file) >>>>>>> and no decryption is performed. The flag is cleared just before the >>>>>>> first write is completed (at the beginning of >>>>>>> ecryptfs_write_begin()). >>>>>>> >>>>>>> It was discovered that if a new file was created and then extended >>>>>>> with >>>>>>> truncate, the ECRYPTFS_NEW_FILE flag was not cleared. If pages >>>>>>> corresponding to this file are ever reclaimed, any subsequent reads >>>>>>> would result in userspace seeing eCryptfs file metadata and encrypted >>>>>>> file contents instead of the expected decrypted file contents. >>>>>>> >>>>>>> Data corruption is possible if the file is written to before the >>>>>>> eCryptfs directory is unmounted. The data written will be copied into >>>>>>> pages which have been read directly from the lower file rather than >>>>>>> zeroed pages, as would be expected after extending the file with >>>>>>> truncate. >>>>>>> >>>>>>> This flag, and the functionality that used it, was removed in >>>>>>> upstream >>>>>>> kernels in 2.6.39 with the following commits: >>>>>>> >>>>>>> bd4f0fe8bb7c73c738e1e11bc90d6e2cf9c6e20e >>>>>>> fed8859b3ab94274c986cbdf7d27130e0545f02c >>>>>> >>>>>> Is there a reason we're not just cherry-picking the upstream patches? >>>>>> And so I would assume this patch should be marked as SAUCE? >>>>>> >>>>>> Thanks, >>>>>> Leann >>>>>> >>>>> >>>>> Yeah, 'UBUNTU: SAUCE:' for sure. Tyler said in the LP report that >>>>> backporting those 2 commits was getting too involved and complicated. >>>>> Given the simplicity of his ultimate solution I thought the backport >>>>> seemed better. >>>> >>>> Hrm, those two patches appear to cherry-pick cleanly for me, although I >>>> could be missing other external factors. Reading the bug report it >>>> sounds like Tyler originally thought this was fixed with upstream commit >>>> 3b06b3ebf44170c90c893c6c80916db6e922b9f2 and it was that commit which >>>> was problematic to backport (see comment #85). >>> >>> Yep, I was wrong about 3b06b3eb being the fix. Bad assumption on my >>> part. >>> >>>> It's in the following >>>> comment #86 that he identifies the actual fix being commits bd4f0fe8 and >>>> fed8859b. >>>> >>>> Regardless, the SAUCE patch looks fine to me. It's straightforward and >>>> tested. I was just more curious as to why we don't just cherry-pick the >>>> upstream patches. I've CC'd Tyler to get his reasoning. >>> >>> While bd4f0fe8 and fed8859b will cherry-pick cleanly and get rid of the >>> buggy code, they weren't intended to be bug fixes when I wrote them. >>> They were just intended to remove some functionality in order to make >>> the file creation process a bit faster. To me, it just didn't feel like >>> something that should be backported consider how simple the real fix >>> was. >>> >>> Tyler >> >> Tyler - these 2 patches are simple enough that I'd prefer the clean >> cherry-picks (which we try to use as a matter of policy). I'll retest >> and send out the results... >> >> rtg > > The attached patches are clean cherry-picks and produce the same result without > regression. > > rtg > Matches the upstream changes and was tested... Acked-by: Stefan Bader <smb@canonical.com>
Ack, looks good to me. Don't forget to add the BugLink to the commits before you push. Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com> On Tue, 2011-10-11 at 15:38 +0100, Tim Gardner wrote: > On 10/11/2011 01:53 PM, Tim Gardner wrote: > > On 10/10/2011 04:26 PM, Tyler Hicks wrote: > >> On 2011-10-10 07:16:59, Leann Ogasawara wrote: > >>> On Mon, 2011-10-10 at 14:46 +0100, Tim Gardner wrote: > >>>> On 10/10/2011 02:42 PM, Leann Ogasawara wrote: > >>>>> On Sun, 2011-10-09 at 05:12 -0600, Tim Gardner wrote: > >>>>>> From 27ed7cb2b00512e81016419715c1d9b6794b06ae Mon Sep 17 00:00:00 > >>>>>> 2001 > >>>>>> From: Tyler Hicks<tyhicks@canonical.com> > >>>>>> Date: Fri, 7 Oct 2011 15:54:26 -0500 > >>>>>> Subject: [PATCH] eCryptfs: Clear ECRYPTFS_NEW_FILE flag during > >>>>>> truncate > >>>>>> > >>>>>> BugLink: http://bugs.launchpad.net/bugs/745836 > >>>>>> > >>>>>> The ECRYPTFS_NEW_FILE crypt_stat flag is set upon creation of a new > >>>>>> eCryptfs file. When the flag is set, eCryptfs reads directly from the > >>>>>> lower filesystem when bringing a page up to date. This means that no > >>>>>> offset translation (for the eCryptfs file metadata in the lower file) > >>>>>> and no decryption is performed. The flag is cleared just before the > >>>>>> first write is completed (at the beginning of > >>>>>> ecryptfs_write_begin()). > >>>>>> > >>>>>> It was discovered that if a new file was created and then extended > >>>>>> with > >>>>>> truncate, the ECRYPTFS_NEW_FILE flag was not cleared. If pages > >>>>>> corresponding to this file are ever reclaimed, any subsequent reads > >>>>>> would result in userspace seeing eCryptfs file metadata and encrypted > >>>>>> file contents instead of the expected decrypted file contents. > >>>>>> > >>>>>> Data corruption is possible if the file is written to before the > >>>>>> eCryptfs directory is unmounted. The data written will be copied into > >>>>>> pages which have been read directly from the lower file rather than > >>>>>> zeroed pages, as would be expected after extending the file with > >>>>>> truncate. > >>>>>> > >>>>>> This flag, and the functionality that used it, was removed in > >>>>>> upstream > >>>>>> kernels in 2.6.39 with the following commits: > >>>>>> > >>>>>> bd4f0fe8bb7c73c738e1e11bc90d6e2cf9c6e20e > >>>>>> fed8859b3ab94274c986cbdf7d27130e0545f02c > >>>>> > >>>>> Is there a reason we're not just cherry-picking the upstream patches? > >>>>> And so I would assume this patch should be marked as SAUCE? > >>>>> > >>>>> Thanks, > >>>>> Leann > >>>>> > >>>> > >>>> Yeah, 'UBUNTU: SAUCE:' for sure. Tyler said in the LP report that > >>>> backporting those 2 commits was getting too involved and complicated. > >>>> Given the simplicity of his ultimate solution I thought the backport > >>>> seemed better. > >>> > >>> Hrm, those two patches appear to cherry-pick cleanly for me, although I > >>> could be missing other external factors. Reading the bug report it > >>> sounds like Tyler originally thought this was fixed with upstream commit > >>> 3b06b3ebf44170c90c893c6c80916db6e922b9f2 and it was that commit which > >>> was problematic to backport (see comment #85). > >> > >> Yep, I was wrong about 3b06b3eb being the fix. Bad assumption on my > >> part. > >> > >>> It's in the following > >>> comment #86 that he identifies the actual fix being commits bd4f0fe8 and > >>> fed8859b. > >>> > >>> Regardless, the SAUCE patch looks fine to me. It's straightforward and > >>> tested. I was just more curious as to why we don't just cherry-pick the > >>> upstream patches. I've CC'd Tyler to get his reasoning. > >> > >> While bd4f0fe8 and fed8859b will cherry-pick cleanly and get rid of the > >> buggy code, they weren't intended to be bug fixes when I wrote them. > >> They were just intended to remove some functionality in order to make > >> the file creation process a bit faster. To me, it just didn't feel like > >> something that should be backported consider how simple the real fix > >> was. > >> > >> Tyler > > > > Tyler - these 2 patches are simple enough that I'd prefer the clean > > cherry-picks (which we try to use as a matter of policy). I'll retest > > and send out the results... > > > > rtg > > The attached patches are clean cherry-picks and produce the same result > without regression. > > rtg
On 10/11/2011 03:38 PM, Tim Gardner wrote: > On 10/11/2011 01:53 PM, Tim Gardner wrote: >> On 10/10/2011 04:26 PM, Tyler Hicks wrote: >>> On 2011-10-10 07:16:59, Leann Ogasawara wrote: >>>> On Mon, 2011-10-10 at 14:46 +0100, Tim Gardner wrote: >>>>> On 10/10/2011 02:42 PM, Leann Ogasawara wrote: >>>>>> On Sun, 2011-10-09 at 05:12 -0600, Tim Gardner wrote: >>>>>>> From 27ed7cb2b00512e81016419715c1d9b6794b06ae Mon Sep 17 00:00:00 >>>>>>> 2001 >>>>>>> From: Tyler Hicks<tyhicks@canonical.com> >>>>>>> Date: Fri, 7 Oct 2011 15:54:26 -0500 >>>>>>> Subject: [PATCH] eCryptfs: Clear ECRYPTFS_NEW_FILE flag during >>>>>>> truncate >>>>>>> >>>>>>> BugLink: http://bugs.launchpad.net/bugs/745836 >>>>>>> >>>>>>> The ECRYPTFS_NEW_FILE crypt_stat flag is set upon creation of a new >>>>>>> eCryptfs file. When the flag is set, eCryptfs reads directly from >>>>>>> the >>>>>>> lower filesystem when bringing a page up to date. This means that no >>>>>>> offset translation (for the eCryptfs file metadata in the lower >>>>>>> file) >>>>>>> and no decryption is performed. The flag is cleared just before the >>>>>>> first write is completed (at the beginning of >>>>>>> ecryptfs_write_begin()). >>>>>>> >>>>>>> It was discovered that if a new file was created and then extended >>>>>>> with >>>>>>> truncate, the ECRYPTFS_NEW_FILE flag was not cleared. If pages >>>>>>> corresponding to this file are ever reclaimed, any subsequent reads >>>>>>> would result in userspace seeing eCryptfs file metadata and >>>>>>> encrypted >>>>>>> file contents instead of the expected decrypted file contents. >>>>>>> >>>>>>> Data corruption is possible if the file is written to before the >>>>>>> eCryptfs directory is unmounted. The data written will be copied >>>>>>> into >>>>>>> pages which have been read directly from the lower file rather than >>>>>>> zeroed pages, as would be expected after extending the file with >>>>>>> truncate. >>>>>>> >>>>>>> This flag, and the functionality that used it, was removed in >>>>>>> upstream >>>>>>> kernels in 2.6.39 with the following commits: >>>>>>> >>>>>>> bd4f0fe8bb7c73c738e1e11bc90d6e2cf9c6e20e >>>>>>> fed8859b3ab94274c986cbdf7d27130e0545f02c >>>>>> >>>>>> Is there a reason we're not just cherry-picking the upstream patches? >>>>>> And so I would assume this patch should be marked as SAUCE? >>>>>> >>>>>> Thanks, >>>>>> Leann >>>>>> >>>>> >>>>> Yeah, 'UBUNTU: SAUCE:' for sure. Tyler said in the LP report that >>>>> backporting those 2 commits was getting too involved and complicated. >>>>> Given the simplicity of his ultimate solution I thought the backport >>>>> seemed better. >>>> >>>> Hrm, those two patches appear to cherry-pick cleanly for me, although I >>>> could be missing other external factors. Reading the bug report it >>>> sounds like Tyler originally thought this was fixed with upstream >>>> commit >>>> 3b06b3ebf44170c90c893c6c80916db6e922b9f2 and it was that commit which >>>> was problematic to backport (see comment #85). >>> >>> Yep, I was wrong about 3b06b3eb being the fix. Bad assumption on my >>> part. >>> >>>> It's in the following >>>> comment #86 that he identifies the actual fix being commits bd4f0fe8 >>>> and >>>> fed8859b. >>>> >>>> Regardless, the SAUCE patch looks fine to me. It's straightforward and >>>> tested. I was just more curious as to why we don't just cherry-pick the >>>> upstream patches. I've CC'd Tyler to get his reasoning. >>> >>> While bd4f0fe8 and fed8859b will cherry-pick cleanly and get rid of the >>> buggy code, they weren't intended to be bug fixes when I wrote them. >>> They were just intended to remove some functionality in order to make >>> the file creation process a bit faster. To me, it just didn't feel like >>> something that should be backported consider how simple the real fix >>> was. >>> >>> Tyler >> >> Tyler - these 2 patches are simple enough that I'd prefer the clean >> cherry-picks (which we try to use as a matter of policy). I'll retest >> and send out the results... >> >> rtg > > The attached patches are clean cherry-picks and produce the same result > without regression. > > rtg >
From ebcee101904bc38b029db54af4f56d0570787b5e Mon Sep 17 00:00:00 2001 From: Tyler Hicks <tyhicks@linux.vnet.ibm.com> Date: Wed, 23 Feb 2011 00:54:20 -0600 Subject: [PATCH 2/2] eCryptfs: Remove ECRYPTFS_NEW_FILE crypt stat flag Now that grow_file() is not called in the ecryptfs_create() path, the ECRYPTFS_NEW_FILE flag is no longer needed. It helped ecryptfs_readpage() know not to decrypt zeroes that were read from the lower file in the grow_file() path. Signed-off-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com> (cherry picked from commit fed8859b3ab94274c986cbdf7d27130e0545f02c) Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- fs/ecryptfs/ecryptfs_kernel.h | 25 ++++++++++++------------- fs/ecryptfs/inode.c | 1 - fs/ecryptfs/mmap.c | 15 ++------------- 3 files changed, 14 insertions(+), 27 deletions(-) diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h index e007534..427478e 100644 --- a/fs/ecryptfs/ecryptfs_kernel.h +++ b/fs/ecryptfs/ecryptfs_kernel.h @@ -257,19 +257,18 @@ struct ecryptfs_filename { struct ecryptfs_crypt_stat { #define ECRYPTFS_STRUCT_INITIALIZED 0x00000001 #define ECRYPTFS_POLICY_APPLIED 0x00000002 -#define ECRYPTFS_NEW_FILE 0x00000004 -#define ECRYPTFS_ENCRYPTED 0x00000008 -#define ECRYPTFS_SECURITY_WARNING 0x00000010 -#define ECRYPTFS_ENABLE_HMAC 0x00000020 -#define ECRYPTFS_ENCRYPT_IV_PAGES 0x00000040 -#define ECRYPTFS_KEY_VALID 0x00000080 -#define ECRYPTFS_METADATA_IN_XATTR 0x00000100 -#define ECRYPTFS_VIEW_AS_ENCRYPTED 0x00000200 -#define ECRYPTFS_KEY_SET 0x00000400 -#define ECRYPTFS_ENCRYPT_FILENAMES 0x00000800 -#define ECRYPTFS_ENCFN_USE_MOUNT_FNEK 0x00001000 -#define ECRYPTFS_ENCFN_USE_FEK 0x00002000 -#define ECRYPTFS_UNLINK_SIGS 0x00004000 +#define ECRYPTFS_ENCRYPTED 0x00000004 +#define ECRYPTFS_SECURITY_WARNING 0x00000008 +#define ECRYPTFS_ENABLE_HMAC 0x00000010 +#define ECRYPTFS_ENCRYPT_IV_PAGES 0x00000020 +#define ECRYPTFS_KEY_VALID 0x00000040 +#define ECRYPTFS_METADATA_IN_XATTR 0x00000080 +#define ECRYPTFS_VIEW_AS_ENCRYPTED 0x00000100 +#define ECRYPTFS_KEY_SET 0x00000200 +#define ECRYPTFS_ENCRYPT_FILENAMES 0x00000400 +#define ECRYPTFS_ENCFN_USE_MOUNT_FNEK 0x00000800 +#define ECRYPTFS_ENCFN_USE_FEK 0x00001000 +#define ECRYPTFS_UNLINK_SIGS 0x00002000 u32 flags; unsigned int file_version; size_t iv_bytes; diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 8fa365a..f99051b 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -161,7 +161,6 @@ static int ecryptfs_initialize_file(struct dentry *ecryptfs_dentry) crypt_stat->flags &= ~(ECRYPTFS_ENCRYPTED); goto out; } - crypt_stat->flags |= ECRYPTFS_NEW_FILE; ecryptfs_printk(KERN_DEBUG, "Initializing crypto context\n"); rc = ecryptfs_new_file_context(ecryptfs_dentry); if (rc) { diff --git a/fs/ecryptfs/mmap.c b/fs/ecryptfs/mmap.c index eb9d967..c52925c 100644 --- a/fs/ecryptfs/mmap.c +++ b/fs/ecryptfs/mmap.c @@ -193,11 +193,7 @@ static int ecryptfs_readpage(struct file *file, struct page *page) &ecryptfs_inode_to_private(page->mapping->host)->crypt_stat; int rc = 0; - if (!crypt_stat - || !(crypt_stat->flags & ECRYPTFS_ENCRYPTED) - || (crypt_stat->flags & ECRYPTFS_NEW_FILE)) { - ecryptfs_printk(KERN_DEBUG, - "Passing through unencrypted page\n"); + if (!crypt_stat || !(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) { rc = ecryptfs_read_lower_page_segment(page, page->index, 0, PAGE_CACHE_SIZE, page->mapping->host); @@ -295,8 +291,7 @@ static int ecryptfs_write_begin(struct file *file, struct ecryptfs_crypt_stat *crypt_stat = &ecryptfs_inode_to_private(mapping->host)->crypt_stat; - if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED) - || (crypt_stat->flags & ECRYPTFS_NEW_FILE)) { + if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) { rc = ecryptfs_read_lower_page_segment( page, index, 0, PAGE_CACHE_SIZE, mapping->host); if (rc) { @@ -492,12 +487,6 @@ static int ecryptfs_write_end(struct file *file, &ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat; int rc; - if (crypt_stat->flags & ECRYPTFS_NEW_FILE) { - ecryptfs_printk(KERN_DEBUG, "ECRYPTFS_NEW_FILE flag set in " - "crypt_stat at memory location [%p]\n", crypt_stat); - crypt_stat->flags &= ~(ECRYPTFS_NEW_FILE); - } else - ecryptfs_printk(KERN_DEBUG, "Not a new file\n"); ecryptfs_printk(KERN_DEBUG, "Calling fill_zeros_to_end_of_page" "(page w/ index = [0x%.16lx], to = [%d])\n", index, to); if (!(crypt_stat->flags & ECRYPTFS_ENCRYPTED)) { -- 1.7.1