| Message ID | 1475258329-146528-1-git-send-email-ebiggers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric, On Fri, Sep 30, 2016 at 7:58 PM, Eric Biggers <ebiggers@google.com> wrote: > The XTS tweak (or IV) was initialized differently on little endian and > big endian systems. Because the ciphertext depends on the XTS tweak, it > was not possible to use an encrypted filesystem created by a little > endian system on a big endian system and vice versa, even if they shared > the same PAGE_SIZE. Fix this by always using little endian. > > This will break hypothetical big endian users of ext4 or f2fs > encryption. However, all users we are aware of are little endian, and > it's believed that "real" big endian users are unlikely to exist yet. > So this might as well be fixed now before it's too late. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > fs/crypto/crypto.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c > index 61057b7d..98f87fe 100644 > --- a/fs/crypto/crypto.c > +++ b/fs/crypto/crypto.c > @@ -151,7 +151,10 @@ static int do_page_crypto(struct inode *inode, > struct page *src_page, struct page *dest_page, > gfp_t gfp_flags) > { > - u8 xts_tweak[FS_XTS_TWEAK_SIZE]; > + struct { > + __le64 index; > + u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)]; > + } xts_tweak; While we are here, wouldn't it make sense to rename the variable to "iv"? In aes-xts mode the IV is used as tweak. But it is still an IV and passed as IV parameter to the crypto API. Especially when other cipher modes are used this is confusing.
On Sat, Oct 01, 2016 at 06:03:31PM +0200, Richard Weinberger wrote: > Eric, > > On Fri, Sep 30, 2016 at 7:58 PM, Eric Biggers <ebiggers@google.com> wrote: > > The XTS tweak (or IV) was initialized differently on little endian and > > big endian systems. Because the ciphertext depends on the XTS tweak, it > > was not possible to use an encrypted filesystem created by a little > > endian system on a big endian system and vice versa, even if they shared > > the same PAGE_SIZE. Fix this by always using little endian. > > > > This will break hypothetical big endian users of ext4 or f2fs > > encryption. However, all users we are aware of are little endian, and > > it's believed that "real" big endian users are unlikely to exist yet. > > So this might as well be fixed now before it's too late. > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > --- > > fs/crypto/crypto.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c > > index 61057b7d..98f87fe 100644 > > --- a/fs/crypto/crypto.c > > +++ b/fs/crypto/crypto.c > > @@ -151,7 +151,10 @@ static int do_page_crypto(struct inode *inode, > > struct page *src_page, struct page *dest_page, > > gfp_t gfp_flags) > > { > > - u8 xts_tweak[FS_XTS_TWEAK_SIZE]; > > + struct { > > + __le64 index; > > + u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)]; > > + } xts_tweak; > > While we are here, wouldn't it make sense to rename the variable to "iv"? > In aes-xts mode the IV is used as tweak. But it is still an IV and passed > as IV parameter to the crypto API. > > Especially when other cipher modes are used this is confusing. > Good idea --- I agree that "iv" is a better name, so as to not tie the code to XTS specifically. But I think the renaming should be a separate patch. Also, currently this code *is* only supposed to be used for XTS. There's a bug where a specially crafted filesystem can cause this code path to be entered with CTS, but I have a patch pending in the ext4 tree to fix that. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric, On 03.10.2016 20:03, Eric Biggers wrote: >>> { >>> - u8 xts_tweak[FS_XTS_TWEAK_SIZE]; >>> + struct { >>> + __le64 index; >>> + u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)]; >>> + } xts_tweak; >> >> While we are here, wouldn't it make sense to rename the variable to "iv"? >> In aes-xts mode the IV is used as tweak. But it is still an IV and passed >> as IV parameter to the crypto API. >> >> Especially when other cipher modes are used this is confusing. >> > > Good idea --- I agree that "iv" is a better name, so as to not tie the code to > XTS specifically. But I think the renaming should be a separate patch. Sure. I can do that. > Also, currently this code *is* only supposed to be used for XTS. There's a bug > where a specially crafted filesystem can cause this code path to be entered with > CTS, but I have a patch pending in the ext4 tree to fix that. David and I are currently working on UBIFS encryption and we have to support other cipher modes than XTS. So, keeping fscrypto as generic as possible would be nice. :-) Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 04, 2016 at 10:46:54AM +0200, Richard Weinberger wrote: > > Also, currently this code *is* only supposed to be used for XTS. There's a bug > > where a specially crafted filesystem can cause this code path to be entered with > > CTS, but I have a patch pending in the ext4 tree to fix that. > > David and I are currently working on UBIFS encryption and we have to support other cipher > modes than XTS. So, keeping fscrypto as generic as possible would be nice. :-) > The problem was that the kernel supported reading a file whose contents was encrypted with CTS, which is only supposed to be used for filenames. This was inconsistent with FS_IOC_SET_ENCRYPTION_POLICY which currently only allows XTS for contents and CTS for filenames. So in other words I wanted to eliminate a strange scenario that was not intended to happen and was almost certainly never tested. Either way, new modes can still be added if there is a good reason to do so. What new encryption modes are you thinking of adding, would they be for contents or for filenames, and are you thinking they would be offered by all filesystems (ext4 and f2fs too)? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric, > On 04.10.2016, at 18:38, Eric Biggers <ebiggers@google.com> wrote: > > On Tue, Oct 04, 2016 at 10:46:54AM +0200, Richard Weinberger wrote: >>> Also, currently this code *is* only supposed to be used for XTS. There's a bug >>> where a specially crafted filesystem can cause this code path to be entered with >>> CTS, but I have a patch pending in the ext4 tree to fix that. >> >> David and I are currently working on UBIFS encryption and we have to support other cipher >> modes than XTS. So, keeping fscrypto as generic as possible would be nice. :-) >> > > The problem was that the kernel supported reading a file whose contents was > encrypted with CTS, which is only supposed to be used for filenames. This was > inconsistent with FS_IOC_SET_ENCRYPTION_POLICY which currently only allows XTS > for contents and CTS for filenames. So in other words I wanted to eliminate a > strange scenario that was not intended to happen and was almost certainly never > tested. > > Either way, new modes can still be added if there is a good reason to do so. > What new encryption modes are you thinking of adding, would they be for contents > or for filenames, and are you thinking they would be offered by all filesystems > (ext4 and f2fs too)? We currently have one case where our embedded platform is only able to do AES-CBC in hardware, not AES-XTS. So switching to AES-CBC for file contents would yield far better performance while still being "secure enough". Generally speaking though, it would be great to have encryption _and_ authentication for file contents. AEAD modes like GCM or future finalists of the CAESAR competition come to mind. IIRC the ext4 encryption design document mentions this, but it's unclear to me why AES-GCM wasn't used for file contents from the beginning. I'd guess it has to do with where to store the authentication tag and performance. Does anybody have details on that? Thanks, David -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 30, 2016 at 7:58 PM, Eric Biggers <ebiggers@google.com> wrote: > The XTS tweak (or IV) was initialized differently on little endian and > big endian systems. Because the ciphertext depends on the XTS tweak, it > was not possible to use an encrypted filesystem created by a little > endian system on a big endian system and vice versa, even if they shared > the same PAGE_SIZE. Fix this by always using little endian. > > This will break hypothetical big endian users of ext4 or f2fs > encryption. However, all users we are aware of are little endian, and > it's believed that "real" big endian users are unlikely to exist yet. > So this might as well be fixed now before it's too late. > > Signed-off-by: Eric Biggers <ebiggers@google.com> Thanks, applied. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c index 61057b7d..98f87fe 100644 --- a/fs/crypto/crypto.c +++ b/fs/crypto/crypto.c @@ -151,7 +151,10 @@ static int do_page_crypto(struct inode *inode, struct page *src_page, struct page *dest_page, gfp_t gfp_flags) { - u8 xts_tweak[FS_XTS_TWEAK_SIZE]; + struct { + __le64 index; + u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)]; + } xts_tweak; struct skcipher_request *req = NULL; DECLARE_FS_COMPLETION_RESULT(ecr); struct scatterlist dst, src; @@ -171,17 +174,15 @@ static int do_page_crypto(struct inode *inode, req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP, page_crypt_complete, &ecr); - BUILD_BUG_ON(FS_XTS_TWEAK_SIZE < sizeof(index)); - memcpy(xts_tweak, &index, sizeof(index)); - memset(&xts_tweak[sizeof(index)], 0, - FS_XTS_TWEAK_SIZE - sizeof(index)); + BUILD_BUG_ON(sizeof(xts_tweak) != FS_XTS_TWEAK_SIZE); + xts_tweak.index = cpu_to_le64(index); + memset(xts_tweak.padding, 0, sizeof(xts_tweak.padding)); sg_init_table(&dst, 1); sg_set_page(&dst, dest_page, PAGE_SIZE, 0); sg_init_table(&src, 1); sg_set_page(&src, src_page, PAGE_SIZE, 0); - skcipher_request_set_crypt(req, &src, &dst, PAGE_SIZE, - xts_tweak); + skcipher_request_set_crypt(req, &src, &dst, PAGE_SIZE, &xts_tweak); if (rw == FS_DECRYPT) res = crypto_skcipher_decrypt(req); else
The XTS tweak (or IV) was initialized differently on little endian and big endian systems. Because the ciphertext depends on the XTS tweak, it was not possible to use an encrypted filesystem created by a little endian system on a big endian system and vice versa, even if they shared the same PAGE_SIZE. Fix this by always using little endian. This will break hypothetical big endian users of ext4 or f2fs encryption. However, all users we are aware of are little endian, and it's believed that "real" big endian users are unlikely to exist yet. So this might as well be fixed now before it's too late. Signed-off-by: Eric Biggers <ebiggers@google.com> --- fs/crypto/crypto.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)