Natty SRU: eCryptfs: Clear ECRYPTFS_NEW_FILE flag during truncate

[Tyler Hicks]

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 at 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
-------------- 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/20111010/7b8a4591/attachment.sig>