[RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Michael Halcrow]

Several file systems either have already implemented encryption or are
in the process of doing so.  This addresses usability and storage
isolation requirements on mobile devices and in multi-tenant
environments.

While distinct keys locked down to user accounts protect the names and
contents of individual files, a volume-level encryption key should
protect the file system metadata.  When using dm-crypt to do this, the
blocks that the file system encrypts undergo another round of
encryption.  In many contexts this isn't necessary, and it results in
decreased performance and increased power consumption.  This
discourages users and vendors from taking steps to protect file system
metadata in addition to file contents.

This patchset provides a new I/O request flag that suggests to lower
layers that encryption may not be necessary because the file system
has already done it.  The first patch targets the block subsystem and
adds the REQ_NOENCRYPT flag.  The second patch targets dm-crypt and
adds logic to observe the flag only when the user has opted-in by
passing allow_encrypt_override as one of the optional parameters to
the dm-crypt target.  The third patch targets ext4 and sets the
REQ_NOENCRYPT flag for blocks it encrypts and decrypts.  The fourth
patch does the same for f2fs.

In this patchset I'm proposing that we make dm-crypt's observation of
the file system "don't encrypt" request optional, but I'm not sure
that's a good tradeoff.  Under the assumption that encryption in
upstream file systems is sound, the most common way I expect things
can go awry is if the file system keys don't meet security
requirements while dm-crypt keys do.  However I'm not convinced that
will end up being a widespread problem in practice, and there's a real
data corruption concern with making dm-crypt's observation of the flag
optional.

The problem is that once dm-crypt observes the flag, it must always
continue to do so or dm-crypt might decrypt content that it didn't
encrypt.  This can lead to scenarios where a vendor sets the dm-crypt
option to observe the "don't encrypt" flag, and then down the road a
user follows one of the many online guides to manually recover a
dm-crypt partition without setting this new option.

Should there be an encryption disable flag?  I'm interested in
considering other solutions.

Michael Halcrow (4):
  block: Add bio req flag to disable encryption in block
  dm-crypt: Skip encryption of file system-encrypted blocks
  ext4: Set the bio REQ_NOENCRYPT flag
  f2fs: Set the bio REQ_NOENCRYPT flag

 drivers/md/dm-crypt.c     | 17 +++++++++++++----
 fs/crypto/bio.c           |  2 +-
 fs/ext4/ext4.h            |  3 +++
 fs/ext4/inode.c           | 13 ++++++++-----
 fs/ext4/page-io.c         |  5 +++++
 fs/ext4/readpage.c        |  3 ++-
 fs/f2fs/data.c            | 10 ++++++++--
 include/linux/blk_types.h |  2 ++
 8 files changed, 42 insertions(+), 13 deletions(-)

-- 
2.13.1.508.gb3defc5cc-goog

[RFC PATCH 1/4] block: Add bio req flag to disable encryption in block

[Michael Halcrow]

When both the file system and a lower layer such as dm-crypt encrypt
the same file contents, it impacts performance and power utilization.
Depending on how the operating environment manages the encryption
keys, there is often no significant security benefit to redundantly
encrypting.

File systems that encrypt some of their blocks can set the
REQ_NOENCRYPT flag as a directive to lower layers to not encrypt.

Lower layers may optionally observe the flag, but once thay do, they
must continue to observe it on subsequent I/O on the device.
Otherwise they will decrypt content that they didn't previously
encrypt, resulting in data corruption.

Signed-off-by: Michael Halcrow <mhalcrow@google.com>
---
 include/linux/blk_types.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 61339bc44400..89da8f5f7be1 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -205,6 +205,7 @@ enum req_flag_bits {
 	/* command specific flags for REQ_OP_WRITE_ZEROES: */
 	__REQ_NOUNMAP,		/* do not free blocks when zeroing */
 
+	__REQ_NOENCRYPT,	/* ok to not encrypt */
 	__REQ_NR_BITS,		/* stops here */
 };
 
@@ -223,6 +224,7 @@ enum req_flag_bits {
 #define REQ_BACKGROUND		(1ULL << __REQ_BACKGROUND)
 
 #define REQ_NOUNMAP		(1ULL << __REQ_NOUNMAP)
+#define REQ_NOENCRYPT		(1ULL << __REQ_NOENCRYPT)
 
 #define REQ_FAILFAST_MASK \
 	(REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT | REQ_FAILFAST_DRIVER)
-- 
2.13.1.508.gb3defc5cc-goog

[RFC PATCH 2/4] dm-crypt: Skip encryption of file system-encrypted blocks

[Michael Halcrow]

File systems can encrypt some of their data blocks with their own
encryption keys, and for those blocks another round of encryption at
the dm-crypt layer may be redundant, depending on the keys being used.

This patch enables dm-crypt to observe the REQ_NOENCRYPT flag as an
indicator that a bio request should bypass the dm-crypt encryption
queue.

By default dm-crypt will ignore this request flag from the file
system.  The user must set the allow_encrypt_override option to enable
this functionality.  Once the dm-crypt has been used with the
allow_encrypt_override option for any given block device, it must
continue to be used with the option to avoid the possibility of data
corruption.

Signed-off-by: Michael Halcrow <mhalcrow@google.com>
---
 drivers/md/dm-crypt.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index ebf9e72d479b..14ca8a6de3d9 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -125,7 +125,8 @@ struct iv_tcw_private {
  * and encrypts / decrypts at the same time.
  */
 enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID,
-	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD };
+	     DM_CRYPT_SAME_CPU, DM_CRYPT_NO_OFFLOAD,
+	     DM_CRYPT_ENCRYPT_OVERRIDE };
 
 enum cipher_flags {
 	CRYPT_MODE_INTEGRITY_AEAD,	/* Use authenticated mode for cihper */
@@ -2572,6 +2573,8 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar
 			cc->sector_shift = __ffs(cc->sector_size) - SECTOR_SHIFT;
 		} else if (!strcasecmp(opt_string, "iv_large_sectors"))
 			set_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags);
+		else if (!strcasecmp(opt_string, "allow_encrypt_override"))
+			set_bit(DM_CRYPT_ENCRYPT_OVERRIDE, &cc->flags);
 		else {
 			ti->error = "Invalid feature arguments";
 			return -EINVAL;
@@ -2770,12 +2773,15 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
 	struct crypt_config *cc = ti->private;
 
 	/*
-	 * If bio is REQ_PREFLUSH or REQ_OP_DISCARD, just bypass crypt queues.
+	 * If bio is REQ_PREFLUSH, REQ_NOENCRYPT, or REQ_OP_DISCARD,
+	 * just bypass crypt queues.
 	 * - for REQ_PREFLUSH device-mapper core ensures that no IO is in-flight
 	 * - for REQ_OP_DISCARD caller must use flush if IO ordering matters
 	 */
-	if (unlikely(bio->bi_opf & REQ_PREFLUSH ||
-	    bio_op(bio) == REQ_OP_DISCARD)) {
+	if (unlikely(bio->bi_opf & REQ_PREFLUSH) ||
+	    (unlikely(bio->bi_opf & REQ_NOENCRYPT) &&
+	     test_bit(DM_CRYPT_ENCRYPT_OVERRIDE, &cc->flags)) ||
+	    bio_op(bio) == REQ_OP_DISCARD) {
 		bio->bi_bdev = cc->dev->bdev;
 		if (bio_sectors(bio))
 			bio->bi_iter.bi_sector = cc->start +
@@ -2862,6 +2868,7 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 		num_feature_args += test_bit(DM_CRYPT_NO_OFFLOAD, &cc->flags);
 		num_feature_args += cc->sector_size != (1 << SECTOR_SHIFT);
 		num_feature_args += test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags);
+		num_feature_args += test_bit(DM_CRYPT_ENCRYPT_OVERRIDE, &cc->flags);
 		if (cc->on_disk_tag_size)
 			num_feature_args++;
 		if (num_feature_args) {
@@ -2878,6 +2885,8 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 				DMEMIT(" sector_size:%d", cc->sector_size);
 			if (test_bit(CRYPT_IV_LARGE_SECTORS, &cc->cipher_flags))
 				DMEMIT(" iv_large_sectors");
+			if (test_bit(DM_CRYPT_ENCRYPT_OVERRIDE, &cc->flags))
+				DMEMIT(" allow_encrypt_override");
 		}
 
 		break;
-- 
2.13.1.508.gb3defc5cc-goog

[RFC PATCH 3/4] ext4: Set the bio REQ_NOENCRYPT flag

[Michael Halcrow]

When lower layers such as dm-crypt observe the REQ_NOENCRYPT flag, it
helps the I/O stack avoid redundant encryption, improving performance
and power utilization.

Note that lower layers must be consistent in their observation of this
flag in order to avoid the possibility of data corruption.

Signed-off-by: Michael Halcrow <mhalcrow@google.com>
---
 fs/crypto/bio.c    |  2 +-
 fs/ext4/ext4.h     |  3 +++
 fs/ext4/inode.c    | 13 ++++++++-----
 fs/ext4/page-io.c  |  5 +++++
 fs/ext4/readpage.c |  3 ++-
 5 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
index a409a84f1bca..9093a715d2be 100644
--- a/fs/crypto/bio.c
+++ b/fs/crypto/bio.c
@@ -118,7 +118,7 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 		bio->bi_bdev = inode->i_sb->s_bdev;
 		bio->bi_iter.bi_sector =
 			pblk << (inode->i_sb->s_blocksize_bits - 9);
-		bio_set_op_attrs(bio, REQ_OP_WRITE, 0);
+		bio_set_op_attrs(bio, REQ_OP_WRITE, REQ_NOENCRYPT);
 		ret = bio_add_page(bio, ciphertext_page,
 					inode->i_sb->s_blocksize, 0);
 		if (ret != inode->i_sb->s_blocksize) {
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8e8046104f4d..48c2bc9f8688 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -206,7 +206,10 @@ typedef struct ext4_io_end {
 	ssize_t			size;		/* size of the extent */
 } ext4_io_end_t;
 
+#define EXT4_IO_ENCRYPTED	1
+
 struct ext4_io_submit {
+	unsigned int		io_flags;
 	struct writeback_control *io_wbc;
 	struct bio		*io_bio;
 	ext4_io_end_t		*io_end;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1bd0bfa547f6..25a9b7265692 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1154,10 +1154,11 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
 		if (!buffer_uptodate(bh) && !buffer_delay(bh) &&
 		    !buffer_unwritten(bh) &&
 		    (block_start < from || block_end > to)) {
-			ll_rw_block(REQ_OP_READ, 0, 1, &bh);
-			*wait_bh++ = bh;
 			decrypt = ext4_encrypted_inode(inode) &&
 				S_ISREG(inode->i_mode);
+			ll_rw_block(REQ_OP_READ, (decrypt ? REQ_NOENCRYPT : 0),
+				    1, &bh);
+			*wait_bh++ = bh;
 		}
 	}
 	/*
@@ -3863,6 +3864,7 @@ static int __ext4_block_zero_page_range(handle_t *handle,
 	struct inode *inode = mapping->host;
 	struct buffer_head *bh;
 	struct page *page;
+	bool decrypt;
 	int err = 0;
 
 	page = find_or_create_page(mapping, from >> PAGE_SHIFT,
@@ -3905,13 +3907,14 @@ static int __ext4_block_zero_page_range(handle_t *handle,
 
 	if (!buffer_uptodate(bh)) {
 		err = -EIO;
-		ll_rw_block(REQ_OP_READ, 0, 1, &bh);
+		decrypt = S_ISREG(inode->i_mode) &&
+			ext4_encrypted_inode(inode);
+		ll_rw_block(REQ_OP_READ, (decrypt ? REQ_NOENCRYPT : 0), 1, &bh);
 		wait_on_buffer(bh);
 		/* Uhhuh. Read error. Complain and punt. */
 		if (!buffer_uptodate(bh))
 			goto unlock;
-		if (S_ISREG(inode->i_mode) &&
-		    ext4_encrypted_inode(inode)) {
+		if (decrypt) {
 			/* We expect the key to be set. */
 			BUG_ON(!fscrypt_has_encryption_key(inode));
 			BUG_ON(blocksize != PAGE_SIZE);
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 1a82138ba739..e25bf6cb216a 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -349,6 +349,8 @@ void ext4_io_submit(struct ext4_io_submit *io)
 	if (bio) {
 		int io_op_flags = io->io_wbc->sync_mode == WB_SYNC_ALL ?
 				  REQ_SYNC : 0;
+		if (io->io_flags & EXT4_IO_ENCRYPTED)
+			io_op_flags |= REQ_NOENCRYPT;
 		bio_set_op_attrs(io->io_bio, REQ_OP_WRITE, io_op_flags);
 		submit_bio(io->io_bio);
 	}
@@ -358,6 +360,7 @@ void ext4_io_submit(struct ext4_io_submit *io)
 void ext4_io_submit_init(struct ext4_io_submit *io,
 			 struct writeback_control *wbc)
 {
+	io->io_flags = 0;
 	io->io_wbc = wbc;
 	io->io_bio = NULL;
 	io->io_end = NULL;
@@ -499,6 +502,8 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	do {
 		if (!buffer_async_write(bh))
 			continue;
+		if (data_page)
+			io->io_flags |= EXT4_IO_ENCRYPTED;
 		ret = io_submit_add_bh(io, inode,
 				       data_page ? data_page : page, bh);
 		if (ret) {
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index a81b829d56de..008d14d74f33 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -258,7 +258,8 @@ int ext4_mpage_readpages(struct address_space *mapping,
 			bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9);
 			bio->bi_end_io = mpage_end_io;
 			bio->bi_private = ctx;
-			bio_set_op_attrs(bio, REQ_OP_READ, 0);
+			bio_set_op_attrs(bio, REQ_OP_READ,
+					 ctx ? REQ_NOENCRYPT : 0);
 		}
 
 		length = first_hole << blkbits;
-- 
2.13.1.508.gb3defc5cc-goog

[RFC PATCH 4/4] f2fs: Set the bio REQ_NOENCRYPT flag

[Michael Halcrow]

When lower layers such as dm-crypt observe the REQ_NOENCRYPT flag, it
helps the I/O stack avoid redundant encryption, improving performance
and power utilization.

Note that lower layers must be consistent in their observation of this
flag in order to avoid the possibility of data corruption.

Signed-off-by: Michael Halcrow <mhalcrow@google.com>
---
 fs/f2fs/data.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 7c0f6bdf817d..2a000c0ec7e1 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -359,6 +359,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
 		bio_put(bio);
 		return -EFAULT;
 	}
+	fio->op_flags |= fio->encrypted_page ? REQ_NOENCRYPT : 0;
 	bio_set_op_attrs(bio, fio->op, fio->op_flags);
 
 	__submit_bio(fio->sbi, bio, fio->type);
@@ -384,6 +385,7 @@ int f2fs_submit_page_mbio(struct f2fs_io_info *fio)
 	verify_block_addr(sbi, fio->new_blkaddr);
 
 	bio_page = fio->encrypted_page ? fio->encrypted_page : fio->page;
+	fio->op_flags |= fio->encrypted_page ? REQ_NOENCRYPT : 0;
 
 	/* set submitted = 1 as a return value */
 	fio->submitted = 1;
@@ -1242,7 +1244,10 @@ static int f2fs_mpage_readpages(struct address_space *mapping,
 				bio = NULL;
 				goto set_error_page;
 			}
-			bio_set_op_attrs(bio, REQ_OP_READ, 0);
+			bio_set_op_attrs(bio, REQ_OP_READ,
+					 (f2fs_encrypted_inode(inode) ?
+					  REQ_NOENCRYPT :
+					  0));
 		}
 
 		if (bio_add_page(bio, page, blocksize, 0) < blocksize)
@@ -1914,7 +1919,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
 			err = PTR_ERR(bio);
 			goto fail;
 		}
-		bio->bi_opf = REQ_OP_READ;
+		bio->bi_opf = REQ_OP_READ |
+			(f2fs_encrypted_inode(inode) ? REQ_NOENCRYPT : 0);
 		if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
 			bio_put(bio);
 			err = -EFAULT;
-- 
2.13.1.508.gb3defc5cc-goog

Re: [RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Milan Broz]

On 06/15/2017 01:40 AM, Michael Halcrow wrote:
> Several file systems either have already implemented encryption or are
> in the process of doing so.  This addresses usability and storage
> isolation requirements on mobile devices and in multi-tenant
> environments.
> 
> While distinct keys locked down to user accounts protect the names and
> contents of individual files, a volume-level encryption key should
> protect the file system metadata.  When using dm-crypt to do this, the
> blocks that the file system encrypts undergo another round of
> encryption.  In many contexts this isn't necessary, and it results in
> decreased performance and increased power consumption.  This
> discourages users and vendors from taking steps to protect file system
> metadata in addition to file contents.
> 
> This patchset provides a new I/O request flag that suggests to lower
> layers that encryption may not be necessary because the file system
> has already done it.  The first patch targets the block subsystem and
> adds the REQ_NOENCRYPT flag.  The second patch targets dm-crypt and
> adds logic to observe the flag only when the user has opted-in by
> passing allow_encrypt_override as one of the optional parameters to
> the dm-crypt target.  The third patch targets ext4 and sets the
> REQ_NOENCRYPT flag for blocks it encrypts and decrypts.  The fourth
> patch does the same for f2fs.
> 
> In this patchset I'm proposing that we make dm-crypt's observation of
> the file system "don't encrypt" request optional, but I'm not sure
> that's a good tradeoff.  Under the assumption that encryption in
> upstream file systems is sound, the most common way I expect things
> can go awry is if the file system keys don't meet security
> requirements while dm-crypt keys do.  However I'm not convinced that
> will end up being a widespread problem in practice, and there's a real
> data corruption concern with making dm-crypt's observation of the flag
> optional.
> 
> The problem is that once dm-crypt observes the flag, it must always
> continue to do so or dm-crypt might decrypt content that it didn't
> encrypt.  This can lead to scenarios where a vendor sets the dm-crypt
> option to observe the "don't encrypt" flag, and then down the road a
> user follows one of the many online guides to manually recover a
> dm-crypt partition without setting this new option.
> 
> Should there be an encryption disable flag?  I'm interested in
> considering other solutions.

The whole reason for full disk encryption (FDE) is the it is FULL disk
encryption.

If you do not need encryption on dmcrypt level, just do not use it
by properly configuring storage stack!

The file-level encryption and dm-crypt encryption can have completely different
purpose, even one can be authenticated one (providing integrity)
and second just providing confidentiality.

It is not "redundant" encryption, it is the encryption for different purpose
on different layer.

IMO what you are proposing is incredible security hack and very ugly
layer violation.

If this is accepted, we basically allow attacker to trick system to write
plaintext to media just by setting this flag. This must never ever happen
with FDE - BY DESIGN.

For me, ABSOLUTE NACK to this approach.

(cc dmcrypt list as well)

Milan


> 
> Michael Halcrow (4):
>   block: Add bio req flag to disable encryption in block
>   dm-crypt: Skip encryption of file system-encrypted blocks
>   ext4: Set the bio REQ_NOENCRYPT flag
>   f2fs: Set the bio REQ_NOENCRYPT flag
> 
>  drivers/md/dm-crypt.c     | 17 +++++++++++++----
>  fs/crypto/bio.c           |  2 +-
>  fs/ext4/ext4.h            |  3 +++
>  fs/ext4/inode.c           | 13 ++++++++-----
>  fs/ext4/page-io.c         |  5 +++++
>  fs/ext4/readpage.c        |  3 ++-
>  fs/f2fs/data.c            | 10 ++++++++--
>  include/linux/blk_types.h |  2 ++
>  8 files changed, 42 insertions(+), 13 deletions(-)
> 

Re: [RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Michael Halcrow]

On Thu, Jun 15, 2017 at 09:33:39AM +0200, Milan Broz wrote:
> On 06/15/2017 01:40 AM, Michael Halcrow wrote:
> > Several file systems either have already implemented encryption or are
> > in the process of doing so.  This addresses usability and storage
> > isolation requirements on mobile devices and in multi-tenant
> > environments.
> > 
> > While distinct keys locked down to user accounts protect the names and
> > contents of individual files, a volume-level encryption key should
> > protect the file system metadata.  When using dm-crypt to do this, the
> > blocks that the file system encrypts undergo another round of
> > encryption.  In many contexts this isn't necessary, and it results in
> > decreased performance and increased power consumption.  This
> > discourages users and vendors from taking steps to protect file system
> > metadata in addition to file contents.
> > 
> > This patchset provides a new I/O request flag that suggests to lower
> > layers that encryption may not be necessary because the file system
> > has already done it.  The first patch targets the block subsystem and
> > adds the REQ_NOENCRYPT flag.  The second patch targets dm-crypt and
> > adds logic to observe the flag only when the user has opted-in by
> > passing allow_encrypt_override as one of the optional parameters to
> > the dm-crypt target.  The third patch targets ext4 and sets the
> > REQ_NOENCRYPT flag for blocks it encrypts and decrypts.  The fourth
> > patch does the same for f2fs.
> > 
> > In this patchset I'm proposing that we make dm-crypt's observation of
> > the file system "don't encrypt" request optional, but I'm not sure
> > that's a good tradeoff.  Under the assumption that encryption in
> > upstream file systems is sound, the most common way I expect things
> > can go awry is if the file system keys don't meet security
> > requirements while dm-crypt keys do.  However I'm not convinced that
> > will end up being a widespread problem in practice, and there's a real
> > data corruption concern with making dm-crypt's observation of the flag
> > optional.
> > 
> > The problem is that once dm-crypt observes the flag, it must always
> > continue to do so or dm-crypt might decrypt content that it didn't
> > encrypt.  This can lead to scenarios where a vendor sets the dm-crypt
> > option to observe the "don't encrypt" flag, and then down the road a
> > user follows one of the many online guides to manually recover a
> > dm-crypt partition without setting this new option.
> > 
> > Should there be an encryption disable flag?  I'm interested in
> > considering other solutions.
>
> The whole reason for full disk encryption (FDE) is the it is FULL disk
> encryption.

Milan, I appreciate what dmcrypt is designed to do, and I would
prefer that everyone use it all the time!  I'm just concerned that
vendors, particularly in the mobile space, aren't enabling it to
protect file system metadata because of performance and power
utilization concerns.

> If you do not need encryption on dmcrypt level, just do not use it
> by properly configuring storage stack!

Unless we implement file system metadata encryption in all the file
systems that support file-based encryption, we're going to need
encryption somewhere in the block layer.  How can we do this on mobile
platforms where encrypting twice incurs too high a cost?

> The file-level encryption and dm-crypt encryption can have completely different
> purpose, even one can be authenticated one (providing integrity)
> and second just providing confidentiality.
> 
> It is not "redundant" encryption, it is the encryption for different purpose
> on different layer.
> 
> IMO what you are proposing is incredible security hack and very ugly
> layer violation.

With the advent of inline cryptographic engine (ICE) capability in
more recent hardware, we have a challenging layering problem to
address.  The file system needs to control encryption happening at the
block layer, such as by providing encryption keys for certain sets of
blocks along with initialization vectors.

> If this is accepted, we basically allow attacker to trick system to
> write plaintext to media just by setting this flag. This must never
> ever happen with FDE - BY DESIGN.

That's an important point.  This expands the attack surface to include
the file system, so if an adversary can provide a bad encryption key
or policy at the file system layer or if there's a bug in the file
system that an adversary can exploit, then users setting the
allow_encrypt_override option on dmcrypt would be vulnerable.

> For me, ABSOLUTE NACK to this approach.

I can understand where you're coming from.  Given our challenges on
mobile, do you have any suggestions for an alternative approach?

> (cc dmcrypt list as well)
> 
> Milan
> 
> 
> > 
> > Michael Halcrow (4):
> >   block: Add bio req flag to disable encryption in block
> >   dm-crypt: Skip encryption of file system-encrypted blocks
> >   ext4: Set the bio REQ_NOENCRYPT flag
> >   f2fs: Set the bio REQ_NOENCRYPT flag
> > 
> >  drivers/md/dm-crypt.c     | 17 +++++++++++++----
> >  fs/crypto/bio.c           |  2 +-
> >  fs/ext4/ext4.h            |  3 +++
> >  fs/ext4/inode.c           | 13 ++++++++-----
> >  fs/ext4/page-io.c         |  5 +++++
> >  fs/ext4/readpage.c        |  3 ++-
> >  fs/f2fs/data.c            | 10 ++++++++--
> >  include/linux/blk_types.h |  2 ++
> >  8 files changed, 42 insertions(+), 13 deletions(-)
> > 
> 

Re: [RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Milan Broz]

On 06/15/2017 07:24 PM, Michael Halcrow wrote:
...
>> If this is accepted, we basically allow attacker to trick system to
>> write plaintext to media just by setting this flag. This must never
>> ever happen with FDE - BY DESIGN.
> 
> That's an important point.  This expands the attack surface to include
> the file system, so if an adversary can provide a bad encryption key
> or policy at the file system layer or if there's a bug in the file
> system that an adversary can exploit, then users setting the
> allow_encrypt_override option on dmcrypt would be vulnerable.
> 
>> For me, ABSOLUTE NACK to this approach.
> 
> I can understand where you're coming from.  Given our challenges on
> mobile, do you have any suggestions for an alternative approach?

Well, what I really do not like here that you are solving problem
of missing properly designed cryptographic filesystem by hacking
some layers together that were not designed to it.

Obvious solution is to implement encryption in fs properly and encrypt
fs metadata there. I guess this is your long term goal?

Is the double encryption really such a big problem?
With some hw support it should not cost much time and energy.
Do you have some numbers to show that is it real problem?

What I am missing here is the definition of your threat model.

If the encryption of metadata in block layer is ok for you, why it is not
ok for the data?

What are you solving there? Is it that one user must not see other users data?
(And who is an user on a mobile device - an application in own sandbox?)

Because the confidentiality in the case of stolen device is there with
encryption on any layer. And ext4 encryption uses the same algorithms
as dmcrypt IIRC.

It would better to go with some model that actually increases security.

For example, if your patch marks IO operation that comes from fs as REQ_NOENCRYPT,
could fs send some metadata information in bio of owner (user, translated to key id)
instead and encrypt the sector with a different key?

I am not sure how complicated this would be to implement but we have already
concept of multiple keys in dmcrypt (there is 64 keys for loopAES mapping
and the used key id is calculated as sector# modulo 64).

Maybe the keys can be taken from kernel keyring and loaded dynamically,
according to optional table parameters that defines policy for it.
The IV could be shared (in XTS mode it is not a problem).
If the key is not available, the bio should simply fail early.

Dunno, just an idea - it really depends what are you trying to solve.

Milan

Re: [dm-crypt] [RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Daniel P. Berrange]

On Thu, Jun 15, 2017 at 09:33:39AM +0200, Milan Broz wrote:
> On 06/15/2017 01:40 AM, Michael Halcrow wrote:
> > Several file systems either have already implemented encryption or are
> > in the process of doing so.  This addresses usability and storage
> > isolation requirements on mobile devices and in multi-tenant
> > environments.
> > 
> > While distinct keys locked down to user accounts protect the names and
> > contents of individual files, a volume-level encryption key should
> > protect the file system metadata.  When using dm-crypt to do this, the
> > blocks that the file system encrypts undergo another round of
> > encryption.  In many contexts this isn't necessary, and it results in
> > decreased performance and increased power consumption.  This
> > discourages users and vendors from taking steps to protect file system
> > metadata in addition to file contents.
> > 
> > This patchset provides a new I/O request flag that suggests to lower
> > layers that encryption may not be necessary because the file system
> > has already done it.  The first patch targets the block subsystem and
> > adds the REQ_NOENCRYPT flag.  The second patch targets dm-crypt and
> > adds logic to observe the flag only when the user has opted-in by
> > passing allow_encrypt_override as one of the optional parameters to
> > the dm-crypt target.  The third patch targets ext4 and sets the
> > REQ_NOENCRYPT flag for blocks it encrypts and decrypts.  The fourth
> > patch does the same for f2fs.
> > 
> > In this patchset I'm proposing that we make dm-crypt's observation of
> > the file system "don't encrypt" request optional, but I'm not sure
> > that's a good tradeoff.  Under the assumption that encryption in
> > upstream file systems is sound, the most common way I expect things
> > can go awry is if the file system keys don't meet security
> > requirements while dm-crypt keys do.  However I'm not convinced that
> > will end up being a widespread problem in practice, and there's a real
> > data corruption concern with making dm-crypt's observation of the flag
> > optional.
> > 
> > The problem is that once dm-crypt observes the flag, it must always
> > continue to do so or dm-crypt might decrypt content that it didn't
> > encrypt.  This can lead to scenarios where a vendor sets the dm-crypt
> > option to observe the "don't encrypt" flag, and then down the road a
> > user follows one of the many online guides to manually recover a
> > dm-crypt partition without setting this new option.
> > 
> > Should there be an encryption disable flag?  I'm interested in
> > considering other solutions.
> 
> The whole reason for full disk encryption (FDE) is the it is FULL disk
> encryption.
> 
> If you do not need encryption on dmcrypt level, just do not use it
> by properly configuring storage stack!
> 
> The file-level encryption and dm-crypt encryption can have completely different
> purpose, even one can be authenticated one (providing integrity)
> and second just providing confidentiality.
> 
> It is not "redundant" encryption, it is the encryption for different purpose
> on different layer.
> 
> IMO what you are proposing is incredible security hack and very ugly
> layer violation.

This layering violation will make it very easy for administrators to
accidentally corrupt their data. If they open the LUKS volume, and
then want to do a block device level backup / copy plain text content
to another volume, without involving the filesystem layer. dmcrypt will
be missing the info on which sectors should not be decrypted - it'll
just decrypt everything and thus scramble the data. Worse this data
corruption will be totally invisible until an arbitrary future time
when someone tries to use the backup.

It would also harm portability to other LUKS implementations may not
directly integrate with filesystem drivers in the same way. For
example you can take an existing LUKS volume and export it as a disk
to a virtual machine with QEMU's built-in LUKS support, so the guest
sees plain text directly. You still have a filesystem driver involved
but its impossible to have that communicate with the LUKS driver on
the QEMU host side.

IMHO for this to be viable the LUKS metadata would need to include a
way to maintain a bitmap of which sectors were encrypted & which were
not, so impls can guarantee to do the right thing upon read without
needing help from an external source (which may not exist at time of
read).

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [dm-crypt] [RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Arno Wagner]

On Thu, Jun 15, 2017 at 09:33:39 CEST, Milan Broz wrote:
> On 06/15/2017 01:40 AM, Michael Halcrow wrote:
> > Several file systems either have already implemented encryption or are
> > in the process of doing so.  This addresses usability and storage
> > isolation requirements on mobile devices and in multi-tenant
> > environments.
> >
[...]
> > 
> > Should there be an encryption disable flag?  I'm interested in
> > considering other solutions.
> 
> The whole reason for full disk encryption (FDE) is the it is FULL disk
> encryption.
> 
> If you do not need encryption on dmcrypt level, just do not use it
> by properly configuring storage stack!
> 
> The file-level encryption and dm-crypt encryption can have completely different
> purpose, even one can be authenticated one (providing integrity)
> and second just providing confidentiality.
>
> It is not "redundant" encryption, it is the encryption for different purpose
> on different layer.

I fully agree. And FDE does protect unused space as well and swap.
FDE is for when you need to be sure that nothing unencrypted goes
to the disk (or specific area of the disk, loop-file, etc.) under 
any circumstances. 
 
> IMO what you are proposing is incredible security hack and very ugly
> layer violation.

Indeed. Do _NOT_ do this. It would have to be disabled or patched 
out by anybody that really cares about security. Performance must
not trump security or you lose security. And performance is really
good enough in most cases anyways.

This would also mean that you lose the ability to move a LUKS 
container transparently because it is not self-contained anymore.
That iwould be extremely bad for backups.

> If this is accepted, we basically allow attacker to trick system to write
> plaintext to media just by setting this flag. This must never ever happen
> with FDE - BY DESIGN.

Indeed.
 
> For me, ABSOLUTE NACK to this approach.

Same here. And I do not want to maintain documentation telling
users how to reliably get rid of this "feature". 

Regards,
Arno


> (cc dmcrypt list as well)
> 
> Milan
> 
> 
> > 
> > Michael Halcrow (4):
> >   block: Add bio req flag to disable encryption in block
> >   dm-crypt: Skip encryption of file system-encrypted blocks
> >   ext4: Set the bio REQ_NOENCRYPT flag
> >   f2fs: Set the bio REQ_NOENCRYPT flag
> > 
> >  drivers/md/dm-crypt.c     | 17 +++++++++++++----
> >  fs/crypto/bio.c           |  2 +-
> >  fs/ext4/ext4.h            |  3 +++
> >  fs/ext4/inode.c           | 13 ++++++++-----
> >  fs/ext4/page-io.c         |  5 +++++
> >  fs/ext4/readpage.c        |  3 ++-
> >  fs/f2fs/data.c            | 10 ++++++++--
> >  include/linux/blk_types.h |  2 ++
> >  8 files changed, 42 insertions(+), 13 deletions(-)
> > 
> 
> _______________________________________________
> dm-crypt mailing list
> dm-crypt@saout.de
> http://www.saout.de/mailman/listinfo/dm-crypt

-- 
Arno Wagner,     Dr. sc. techn., Dipl. Inform.,    Email: arno@wagner.name
GnuPG: ID: CB5D9718  FP: 12D6 C03B 1B30 33BB 13CF  B774 E35C 5FA1 CB5D 9718
----
A good decision is based on knowledge and not on numbers. -- Plato

If it's in the news, don't worry about it.  The very definition of 
"news" is "something that hardly ever happens." -- Bruce Schneier

Re: [RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Michael Halcrow]

On Thu, Jun 15, 2017 at 08:17:02PM +0200, Milan Broz wrote:
> On 06/15/2017 07:24 PM, Michael Halcrow wrote:
> ...
> >> If this is accepted, we basically allow attacker to trick system to
> >> write plaintext to media just by setting this flag. This must never
> >> ever happen with FDE - BY DESIGN.
> > 
> > That's an important point.  This expands the attack surface to include
> > the file system, so if an adversary can provide a bad encryption key
> > or policy at the file system layer or if there's a bug in the file
> > system that an adversary can exploit, then users setting the
> > allow_encrypt_override option on dmcrypt would be vulnerable.
> > 
> >> For me, ABSOLUTE NACK to this approach.
> > 
> > I can understand where you're coming from.  Given our challenges on
> > mobile, do you have any suggestions for an alternative approach?
> 
> Well, what I really do not like here that you are solving problem
> of missing properly designed cryptographic filesystem by hacking
> some layers together that were not designed to it.

Agreed.  I enthusiastically withdraw this proposal.

I think I'm instead going to look into proposing something new in the
block layer to address performance concerns with file system
encryption and inline cryptographic engine support.

> Obvious solution is to implement encryption in fs properly and encrypt
> fs metadata there. I guess this is your long term goal?

My long-term goal is to delegate all encryption to the block layer,
with the file system indicating which keys should encrypt which
blocks.  Given your concerns along with the backup/restore scenario
that Daniel Barrange pointed out, I'm convinced dmcrypt is not the
right place to do that.  At least not without more substantial
modifications to dmcrypt, and I would rather leave dmcrypt's behavior
as boring and predictable as possible.

> Is the double encryption really such a big problem?  With some hw
> support it should not cost much time and energy.  Do you have some
> numbers to show that is it real problem?

We have run numbers on several devices to measure the perf impact of
storage encryption, and it's a big enough on some of the lower-end
devices to threaten the ability to ship with storage encryption
enabled by default.

> What I am missing here is the definition of your threat model.

The original design document has some details regarding the threat
model for file system-level encryption, but there's a lot more the
story as you factor in how various approaches to storage encryption
impact device behavior.

https://docs.google.com/document/d/1ft26lUQyuSpiu6VleP70_npaWdRfXFoNnB8JYnykNTg/preview

> If the encryption of metadata in block layer is ok for you, why it
> is not ok for the data?

In short, because there are classes of data and classes of
adversaries.

The longer answer to that question also brings up how keys are managed
and protected across a spectrum of devices with a variety of hardware
capabilities.

Ideally, it would be impossible for an adversary to access any
plaintext at all without the user's credentials.  However, for the
vast majority of users a device needs to be able to boot and be
minimally functional without the user authenticating on the device --
think of a device rebooting in the middle of the night, and then its
alarm doesn't go off in the morning because it's stuck on a screen
asking for a pin.  Incoming phone calls don't go through.  That's a
recipe for encryption being disabled by default.

While a user might not care whether someone with enough resources to
mount a hardware attack can find out that that their alarm is set for
6:45am or that the directory structure reveals that they have a
particular app installed, they might care if a sophisticated and
well-funded adversary can read the contents of their email or their
location history without the user authenticating with "something they
know."

Meanwhile, if the key that protects metadata is locked in a secure
hardware element, a non-trivial number adversaries can be effectively
locked out from discovering anything at all simply because of the
costs associated with compromising the key.  This protection is
worthwhile.

> What are you solving there? Is it that one user must not see other users data?
> (And who is an user on a mobile device - an application in own sandbox?)
> 
> Because the confidentiality in the case of stolen device is there with
> encryption on any layer. And ext4 encryption uses the same algorithms
> as dmcrypt IIRC.
> 
> It would better to go with some model that actually increases security.
> 
> For example, if your patch marks IO operation that comes from fs as
> REQ_NOENCRYPT, could fs send some metadata information in bio of
> owner (user, translated to key id) instead and encrypt the sector
> with a different key?

I really like this idea.  However because users can access the dmcrypt
device without the file system, I wouldn't want to try to do it
without dmcrypt maintaining additional metadata about which keys
protect which blocks.  Even then, the user directly accessing the
dmcrypt device would be surprised when dmcrypt returns EIO because it
doesn't have a key for the blocks at offsets 42 and 128.

At this point I do think something new at the block layer for file
system managed encryption and inline cryptographic engine support will
be necessary.

> I am not sure how complicated this would be to implement but we have already
> concept of multiple keys in dmcrypt (there is 64 keys for loopAES mapping
> and the used key id is calculated as sector# modulo 64).
> 
> Maybe the keys can be taken from kernel keyring and loaded dynamically,
> according to optional table parameters that defines policy for it.
> The IV could be shared (in XTS mode it is not a problem).
> If the key is not available, the bio should simply fail early.
> 
> Dunno, just an idea - it really depends what are you trying to solve.
> 
> Milan

Re: [RFC PATCH 0/4] Allow file systems to selectively bypass dm-crypt

[Mike Snitzer]

On Fri, Jun 16 2017 at  2:42pm -0400,
Michael Halcrow <mhalcrow@google.com> wrote:

> On Thu, Jun 15, 2017 at 08:17:02PM +0200, Milan Broz wrote:
> > On 06/15/2017 07:24 PM, Michael Halcrow wrote:
> > ...
> > >> If this is accepted, we basically allow attacker to trick system to
> > >> write plaintext to media just by setting this flag. This must never
> > >> ever happen with FDE - BY DESIGN.
> > > 
> > > That's an important point.  This expands the attack surface to include
> > > the file system, so if an adversary can provide a bad encryption key
> > > or policy at the file system layer or if there's a bug in the file
> > > system that an adversary can exploit, then users setting the
> > > allow_encrypt_override option on dmcrypt would be vulnerable.
> > > 
> > >> For me, ABSOLUTE NACK to this approach.
> > > 
> > > I can understand where you're coming from.  Given our challenges on
> > > mobile, do you have any suggestions for an alternative approach?
> > 
> > Well, what I really do not like here that you are solving problem
> > of missing properly designed cryptographic filesystem by hacking
> > some layers together that were not designed to it.
> 
> Agreed.  I enthusiastically withdraw this proposal.
> 
> I think I'm instead going to look into proposing something new in the
> block layer to address performance concerns with file system
> encryption and inline cryptographic engine support.

As should have been done from the start.  The emergence of ICE support
for mobile/embedded platforms should result in a properly designed
solution to enable dm-crypt to leverage them (easier said than done!).
And if only certain files need to be encrypted then dm-crypt may or may
not be configured in the stack.

> > It would better to go with some model that actually increases security.
> > 
> > For example, if your patch marks IO operation that comes from fs as
> > REQ_NOENCRYPT, could fs send some metadata information in bio of
> > owner (user, translated to key id) instead and encrypt the sector
> > with a different key?
> 
> I really like this idea.  However because users can access the dmcrypt
> device without the file system, I wouldn't want to try to do it
> without dmcrypt maintaining additional metadata about which keys
> protect which blocks.  Even then, the user directly accessing the
> dmcrypt device would be surprised when dmcrypt returns EIO because it
> doesn't have a key for the blocks at offsets 42 and 128.
> 
> At this point I do think something new at the block layer for file
> system managed encryption and inline cryptographic engine support will
> be necessary.

Yes.