Message ID | 1331889411-29401-2-git-send-email-colin.king@canonical.com |
---|---|
State | New |
Headers | show |
On Fri, Mar 16, 2012 at 09:16:51AM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > 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 Confirmed these two are in Natty as cherry-picks. > Signed-off-by: Tyler Hicks <tyhicks@canonical.com> > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > fs/ecryptfs/inode.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index 645da17..3c1dbc0 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -777,6 +777,9 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia, > 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); > + > /* Set up a fake ecryptfs file, this is used to interface with > * the file in the underlying filesystem so that the > * truncation has an effect there as well. */ Seems to do what it says. Acked-by: Andy Whitcroft <apw@canonical.com> BTW as we are wrapping up Maverick, is this serious enough to warrent including in any final upload. Data corruption sounds like a critical issue. -apw
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 16.03.2012 10:16, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > 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> Signed-off-by: Colin Ian > King <colin.king@canonical.com> --- fs/ecryptfs/inode.c | 3 +++ 1 files > changed, 3 insertions(+), 0 deletions(-) > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index > 645da17..3c1dbc0 100644 --- a/fs/ecryptfs/inode.c +++ > b/fs/ecryptfs/inode.c @@ -777,6 +777,9 @@ static int truncate_upper(struct > dentry *dentry, struct iattr *ia, 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); + /* Set up a fake ecryptfs file, this is used to > interface with * the file in the underlying filesystem so that the * > truncation has an effect there as well. */ Trusting your testing... -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBCgAGBQJPYx1cAAoJEOhnXe7L7s6jVPsP/2DangfDp+G8f8Oqg9wo5Q1+ tsfwPCnwvNch4H44Yh+h57uouG+ODkUra19DtpsWSavD+Ge6OTibf4BFBU5NYBtN zNttGE28xiEHM8CL6s3YewUxfrVO9PtSpBo4+wvMk8Zs4/4jL+A24aukHhGjHKR+ abMYTMxH/NjyytDAAtMsMq1mVtCfeN+dZOTzAMTLhbMcnsdeJ5Qe7lRCqfmnCgBI xN+8i/5qT0iVjgfkZnxYIsd0606CYzvE5ZoY/TVAjXMd7pZG2esrxWQAfOrL9kYG AXbd3vcpT9b2IO9DmkU6wxAuhfV7gOwpC8MvtD+8qtO6vV085mbyD+uXB2wXjBgN FVTcho+VwKi0ZzJeQoCdsFkPVmXPmtTFqx6VQhsxlnBLAQODJmnRZIvQjGl6b2IP +0puf/ILuFA2tfhlCI4ZcljyM9RI1gchbxebJ+jLm3j6ajZizWhl3EqQ9YMSYEZm R9xN+DP2U4Uz5kMuIOvWTG4i4vBmeJMtQB259JKKA2W+4TTiCwO81MjlLt+uhpO9 w5GJ80RSEjwtUDBMnbPgJV8Y6s4u0vhwZjrTI9+GmhF8jNBO87gqZLW/W6zRH0Xc GzrzEVK0w9k67RmjsXUe8xOVRIfyrVnKSMhpuARRUwefad85gQ4NNFsxUjBFtG+K +6aPbPQxn9nx4cy8kMwH =e+JG -----END PGP SIGNATURE-----
On 16/03/12 11:00, Andy Whitcroft wrote: > On Fri, Mar 16, 2012 at 09:16:51AM +0000, Colin King wrote: >> From: Colin Ian King<colin.king@canonical.com> >> >> 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 > > Confirmed these two are in Natty as cherry-picks. > >> Signed-off-by: Tyler Hicks<tyhicks@canonical.com> >> Signed-off-by: Colin Ian King<colin.king@canonical.com> >> --- >> fs/ecryptfs/inode.c | 3 +++ >> 1 files changed, 3 insertions(+), 0 deletions(-) >> >> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c >> index 645da17..3c1dbc0 100644 >> --- a/fs/ecryptfs/inode.c >> +++ b/fs/ecryptfs/inode.c >> @@ -777,6 +777,9 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia, >> 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); >> + >> /* Set up a fake ecryptfs file, this is used to interface with >> * the file in the underlying filesystem so that the >> * truncation has an effect there as well. */ > > Seems to do what it says. > > Acked-by: Andy Whitcroft<apw@canonical.com> > > BTW as we are wrapping up Maverick, is this serious enough to warrent > including in any final upload. Data corruption sounds like a critical > issue. Yep, I will configure up a test machine and check out the patch on this sometime today. Colin > > -apw >
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index 645da17..3c1dbc0 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -777,6 +777,9 @@ static int truncate_upper(struct dentry *dentry, struct iattr *ia, 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); + /* Set up a fake ecryptfs file, this is used to interface with * the file in the underlying filesystem so that the * truncation has an effect there as well. */