Patchwork Natty SRU: eCryptfs: Clear ECRYPTFS_NEW_FILE flag during truncate

login
register
mail settings
Submitter Tim Gardner
Date Oct. 9, 2011, 11:12 a.m.
Message ID <20111009111202.E5225F88CF@lochsa.rtg.net>
Download mbox | patch
Permalink /patch/118584/
State New
Headers show

Comments

Tim Gardner - Oct. 9, 2011, 11:12 a.m.
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

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 fs/ecryptfs/inode.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Leann Ogasawara - Oct. 10, 2011, 1:42 p.m.
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

> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  fs/ecryptfs/inode.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index b592938..e47b5a4 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -785,7 +785,11 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia,
>  		lower_ia->ia_valid &= ~ATTR_SIZE;
>  		goto out;
>  	}
> +
>  	crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
> +	if (crypt_stat->flags & ECRYPTFS_NEW_FILE)
> +		crypt_stat->flags &= ~(ECRYPTFS_NEW_FILE);
> +
>  	/* Switch on growing or shrinking file */
>  	if (ia->ia_size > i_size) {
>  		char zero[] = { 0x00 };
> -- 
> 1.7.5.4
> 
>
Tim Gardner - Oct. 10, 2011, 1:46 p.m.
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.

rtg
Leann Ogasawara - Oct. 10, 2011, 2:16 p.m.
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).  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.

Thanks,
Leann
Tyler Hicks - Oct. 10, 2011, 3:26 p.m.
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
Tim Gardner - Oct. 11, 2011, 12:53 p.m.
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

Patch

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index b592938..e47b5a4 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -785,7 +785,11 @@  static int truncate_upper(struct dentry *dentry, struct iattr *ia,
 		lower_ia->ia_valid &= ~ATTR_SIZE;
 		goto out;
 	}
+
 	crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
+	if (crypt_stat->flags & ECRYPTFS_NEW_FILE)
+		crypt_stat->flags &= ~(ECRYPTFS_NEW_FILE);
+
 	/* Switch on growing or shrinking file */
 	if (ia->ia_size > i_size) {
 		char zero[] = { 0x00 };