All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-10-05  7:36 ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, Satya Tangirala

This patch series adds support for metadata encryption to F2FS using
blk-crypto.

Patch 1 replaces fscrypt_get_devices (which took an array of request_queues
and filled it up) with fscrypt_get_device, which takes a index of the
desired device and returns the device at that index (so the index passed
to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1)
inclusive). This allows callers to avoid having to allocate an array to
pass to fscrypt_get_devices() when they only need to iterate through
each element in the array (and have no use for the array itself).

Patch 2 introduces some functions to fscrypt that help filesystems perform
metadata encryption. Any filesystem that wants to use metadata encryption
can call fscrypt_setup_metadata_encryption() with the super_block of the
filesystem, the encryption algorithm and the descriptor of the encryption
key. The descriptor is looked up in the logon keyring of the current
session with "fscrypt:" as the prefix of the descriptor.

The patch also introduces fscrypt_metadata_crypt_bio() which an FS should
call on a bio that the FS wants metadata crypted. The function will add
an encryption context with the metadata encryption key set up by the call
to the above mentioned fscrypt_setup_metadata_encryption().

The patch also introduces fscrypt_metadata_crypt_prepare_all_devices().
Filesystems that use multiple devices should call this function once all
the underlying devices have been determined. An FS might only be able to
determine all the underlying devices after some initial processing that
might already require metadata en/decryption, which is why this function
is separate from fscrypt_setup_metadata_encryption().

Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
will encrypt every block (that's not being encrypted by some other
encryption key, e.g. a per-file key) with the metadata encryption key
except the superblock (and the redundant copy of the superblock). The DUN
of a block is the offset of the block from the start of the F2FS
filesystem.

Please refer to the commit message for why the superblock was excluded from
en/decryption, and other limitations. The superblock and its copy are
stored in plaintext on disk. The encryption algorithm used for metadata
encryption is stored within the superblock itself. Changes to the userspace
tools (that are required to test out metadata encryption with F2FS) are
also being sent out - I'll post a link as a reply to this mail once it's
out.

Satya Tangirala (3):
  fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
  fscrypt: Add metadata encryption support
  f2fs: Add metadata encryption support

 Documentation/filesystems/f2fs.rst |  12 ++
 fs/crypto/Kconfig                  |   6 +
 fs/crypto/Makefile                 |   1 +
 fs/crypto/fscrypt_private.h        |  19 +++
 fs/crypto/inline_crypt.c           |  37 +----
 fs/crypto/metadata_crypt.c         | 220 +++++++++++++++++++++++++++++
 fs/f2fs/data.c                     |  24 ++--
 fs/f2fs/f2fs.h                     |   2 +
 fs/f2fs/super.c                    |  83 +++++++++--
 include/linux/f2fs_fs.h            |   3 +-
 include/linux/fs.h                 |   3 +
 include/linux/fscrypt.h            |  51 ++++++-
 12 files changed, 410 insertions(+), 51 deletions(-)
 create mode 100644 fs/crypto/metadata_crypt.c

-- 
2.28.0.806.g8561365e88-goog


^ permalink raw reply	[flat|nested] 42+ messages in thread

* [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-10-05  7:36 ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-fscrypt, Satya Tangirala, linux-kernel, linux-f2fs-devel

This patch series adds support for metadata encryption to F2FS using
blk-crypto.

Patch 1 replaces fscrypt_get_devices (which took an array of request_queues
and filled it up) with fscrypt_get_device, which takes a index of the
desired device and returns the device at that index (so the index passed
to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1)
inclusive). This allows callers to avoid having to allocate an array to
pass to fscrypt_get_devices() when they only need to iterate through
each element in the array (and have no use for the array itself).

Patch 2 introduces some functions to fscrypt that help filesystems perform
metadata encryption. Any filesystem that wants to use metadata encryption
can call fscrypt_setup_metadata_encryption() with the super_block of the
filesystem, the encryption algorithm and the descriptor of the encryption
key. The descriptor is looked up in the logon keyring of the current
session with "fscrypt:" as the prefix of the descriptor.

The patch also introduces fscrypt_metadata_crypt_bio() which an FS should
call on a bio that the FS wants metadata crypted. The function will add
an encryption context with the metadata encryption key set up by the call
to the above mentioned fscrypt_setup_metadata_encryption().

The patch also introduces fscrypt_metadata_crypt_prepare_all_devices().
Filesystems that use multiple devices should call this function once all
the underlying devices have been determined. An FS might only be able to
determine all the underlying devices after some initial processing that
might already require metadata en/decryption, which is why this function
is separate from fscrypt_setup_metadata_encryption().

Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
will encrypt every block (that's not being encrypted by some other
encryption key, e.g. a per-file key) with the metadata encryption key
except the superblock (and the redundant copy of the superblock). The DUN
of a block is the offset of the block from the start of the F2FS
filesystem.

Please refer to the commit message for why the superblock was excluded from
en/decryption, and other limitations. The superblock and its copy are
stored in plaintext on disk. The encryption algorithm used for metadata
encryption is stored within the superblock itself. Changes to the userspace
tools (that are required to test out metadata encryption with F2FS) are
also being sent out - I'll post a link as a reply to this mail once it's
out.

Satya Tangirala (3):
  fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
  fscrypt: Add metadata encryption support
  f2fs: Add metadata encryption support

 Documentation/filesystems/f2fs.rst |  12 ++
 fs/crypto/Kconfig                  |   6 +
 fs/crypto/Makefile                 |   1 +
 fs/crypto/fscrypt_private.h        |  19 +++
 fs/crypto/inline_crypt.c           |  37 +----
 fs/crypto/metadata_crypt.c         | 220 +++++++++++++++++++++++++++++
 fs/f2fs/data.c                     |  24 ++--
 fs/f2fs/f2fs.h                     |   2 +
 fs/f2fs/super.c                    |  83 +++++++++--
 include/linux/f2fs_fs.h            |   3 +-
 include/linux/fs.h                 |   3 +
 include/linux/fscrypt.h            |  51 ++++++-
 12 files changed, 410 insertions(+), 51 deletions(-)
 create mode 100644 fs/crypto/metadata_crypt.c

-- 
2.28.0.806.g8561365e88-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [PATCH 1/3] fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
  2020-10-05  7:36 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-05  7:36   ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, Satya Tangirala

The new function takes the super_block and the index of a device, and
returns the request_queue of the device at that index (whereas the old
function would take a pointer to an array of request_queues and fill them
all up). This allows callers to avoid allocating an array of request_queues
in some cases (when they don't need the array for anything else).

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/crypto/inline_crypt.c | 33 ++++++++++++++-------------------
 fs/f2fs/super.c          | 16 ++++++++++------
 include/linux/fscrypt.h  |  4 ++--
 3 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index faa25541ccb6..5bbce79df638 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -33,13 +33,15 @@ static int fscrypt_get_num_devices(struct super_block *sb)
 	return 1;
 }
 
-static void fscrypt_get_devices(struct super_block *sb, int num_devs,
-				struct request_queue **devs)
+static struct request_queue *fscrypt_get_device(struct super_block *sb,
+						unsigned int device_index)
 {
-	if (num_devs == 1)
-		devs[0] = bdev_get_queue(sb->s_bdev);
+	if (sb->s_cop->get_device)
+		return sb->s_cop->get_device(sb, device_index);
+	else if (WARN_ON_ONCE(device_index != 0))
+		return NULL;
 	else
-		sb->s_cop->get_devices(sb, devs);
+		return bdev_get_queue(sb->s_bdev);
 }
 
 static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
@@ -70,7 +72,7 @@ int fscrypt_select_encryption_impl(struct fscrypt_info *ci)
 	struct super_block *sb = inode->i_sb;
 	struct blk_crypto_config crypto_cfg;
 	int num_devs;
-	struct request_queue **devs;
+	struct request_queue *dev;
 	int i;
 
 	/* The file must need contents encryption, not filenames encryption */
@@ -106,20 +108,14 @@ int fscrypt_select_encryption_impl(struct fscrypt_info *ci)
 	crypto_cfg.data_unit_size = sb->s_blocksize;
 	crypto_cfg.dun_bytes = fscrypt_get_dun_bytes(ci);
 	num_devs = fscrypt_get_num_devices(sb);
-	devs = kmalloc_array(num_devs, sizeof(*devs), GFP_NOFS);
-	if (!devs)
-		return -ENOMEM;
-	fscrypt_get_devices(sb, num_devs, devs);
 
 	for (i = 0; i < num_devs; i++) {
-		if (!blk_crypto_config_supported(devs[i], &crypto_cfg))
-			goto out_free_devs;
+		dev = fscrypt_get_device(sb, i);
+		if (!dev || !blk_crypto_config_supported(dev, &crypto_cfg))
+			return 0;
 	}
 
 	ci->ci_inlinecrypt = true;
-out_free_devs:
-	kfree(devs);
-
 	return 0;
 }
 
@@ -141,9 +137,6 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 	if (!blk_key)
 		return -ENOMEM;
 
-	blk_key->num_devs = num_devs;
-	fscrypt_get_devices(sb, num_devs, blk_key->devs);
-
 	err = blk_crypto_init_key(&blk_key->base, raw_key, crypto_mode,
 				  fscrypt_get_dun_bytes(ci), sb->s_blocksize);
 	if (err) {
@@ -158,8 +151,10 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 	 * aren't destroyed until after the filesystem was already unmounted
 	 * (namely, the per-mode keys in struct fscrypt_master_key).
 	 */
+	blk_key->num_devs = num_devs;
 	for (i = 0; i < num_devs; i++) {
-		if (!blk_get_queue(blk_key->devs[i])) {
+		blk_key->devs[i] = fscrypt_get_device(sb, i);
+		if (!blk_key->devs[i] || !blk_get_queue(blk_key->devs[i])) {
 			fscrypt_err(inode, "couldn't get request_queue");
 			err = -EAGAIN;
 			goto fail;
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index dfa072fa8081..9a6d375cbe4b 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2509,14 +2509,18 @@ static int f2fs_get_num_devices(struct super_block *sb)
 	return 1;
 }
 
-static void f2fs_get_devices(struct super_block *sb,
-			     struct request_queue **devs)
+static struct request_queue *f2fs_get_device(struct super_block *sb,
+					     unsigned int device_index)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
-	int i;
 
-	for (i = 0; i < sbi->s_ndevs; i++)
-		devs[i] = bdev_get_queue(FDEV(i).bdev);
+	if (WARN_ON_ONCE(device_index >= f2fs_get_num_devices(sb)))
+		return NULL;
+
+	if (!f2fs_is_multi_device(sbi))
+		return bdev_get_queue(sb->s_bdev);
+
+	return bdev_get_queue(FDEV(device_index).bdev);
 }
 
 static const struct fscrypt_operations f2fs_cryptops = {
@@ -2529,7 +2533,7 @@ static const struct fscrypt_operations f2fs_cryptops = {
 	.has_stable_inodes	= f2fs_has_stable_inodes,
 	.get_ino_and_lblk_bits	= f2fs_get_ino_and_lblk_bits,
 	.get_num_devices	= f2fs_get_num_devices,
-	.get_devices		= f2fs_get_devices,
+	.get_device		= f2fs_get_device,
 };
 #endif
 
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 991ff8575d0e..d835fd19a20a 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -70,8 +70,8 @@ struct fscrypt_operations {
 	void (*get_ino_and_lblk_bits)(struct super_block *sb,
 				      int *ino_bits_ret, int *lblk_bits_ret);
 	int (*get_num_devices)(struct super_block *sb);
-	void (*get_devices)(struct super_block *sb,
-			    struct request_queue **devs);
+	struct request_queue *(*get_device)(struct super_block *sb,
+					    unsigned int dev_index);
 };
 
 static inline struct fscrypt_info *fscrypt_get_info(const struct inode *inode)
-- 
2.28.0.806.g8561365e88-goog


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [f2fs-dev] [PATCH 1/3] fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
@ 2020-10-05  7:36   ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-fscrypt, Satya Tangirala, linux-kernel, linux-f2fs-devel

The new function takes the super_block and the index of a device, and
returns the request_queue of the device at that index (whereas the old
function would take a pointer to an array of request_queues and fill them
all up). This allows callers to avoid allocating an array of request_queues
in some cases (when they don't need the array for anything else).

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/crypto/inline_crypt.c | 33 ++++++++++++++-------------------
 fs/f2fs/super.c          | 16 ++++++++++------
 include/linux/fscrypt.h  |  4 ++--
 3 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index faa25541ccb6..5bbce79df638 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -33,13 +33,15 @@ static int fscrypt_get_num_devices(struct super_block *sb)
 	return 1;
 }
 
-static void fscrypt_get_devices(struct super_block *sb, int num_devs,
-				struct request_queue **devs)
+static struct request_queue *fscrypt_get_device(struct super_block *sb,
+						unsigned int device_index)
 {
-	if (num_devs == 1)
-		devs[0] = bdev_get_queue(sb->s_bdev);
+	if (sb->s_cop->get_device)
+		return sb->s_cop->get_device(sb, device_index);
+	else if (WARN_ON_ONCE(device_index != 0))
+		return NULL;
 	else
-		sb->s_cop->get_devices(sb, devs);
+		return bdev_get_queue(sb->s_bdev);
 }
 
 static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
@@ -70,7 +72,7 @@ int fscrypt_select_encryption_impl(struct fscrypt_info *ci)
 	struct super_block *sb = inode->i_sb;
 	struct blk_crypto_config crypto_cfg;
 	int num_devs;
-	struct request_queue **devs;
+	struct request_queue *dev;
 	int i;
 
 	/* The file must need contents encryption, not filenames encryption */
@@ -106,20 +108,14 @@ int fscrypt_select_encryption_impl(struct fscrypt_info *ci)
 	crypto_cfg.data_unit_size = sb->s_blocksize;
 	crypto_cfg.dun_bytes = fscrypt_get_dun_bytes(ci);
 	num_devs = fscrypt_get_num_devices(sb);
-	devs = kmalloc_array(num_devs, sizeof(*devs), GFP_NOFS);
-	if (!devs)
-		return -ENOMEM;
-	fscrypt_get_devices(sb, num_devs, devs);
 
 	for (i = 0; i < num_devs; i++) {
-		if (!blk_crypto_config_supported(devs[i], &crypto_cfg))
-			goto out_free_devs;
+		dev = fscrypt_get_device(sb, i);
+		if (!dev || !blk_crypto_config_supported(dev, &crypto_cfg))
+			return 0;
 	}
 
 	ci->ci_inlinecrypt = true;
-out_free_devs:
-	kfree(devs);
-
 	return 0;
 }
 
@@ -141,9 +137,6 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 	if (!blk_key)
 		return -ENOMEM;
 
-	blk_key->num_devs = num_devs;
-	fscrypt_get_devices(sb, num_devs, blk_key->devs);
-
 	err = blk_crypto_init_key(&blk_key->base, raw_key, crypto_mode,
 				  fscrypt_get_dun_bytes(ci), sb->s_blocksize);
 	if (err) {
@@ -158,8 +151,10 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key,
 	 * aren't destroyed until after the filesystem was already unmounted
 	 * (namely, the per-mode keys in struct fscrypt_master_key).
 	 */
+	blk_key->num_devs = num_devs;
 	for (i = 0; i < num_devs; i++) {
-		if (!blk_get_queue(blk_key->devs[i])) {
+		blk_key->devs[i] = fscrypt_get_device(sb, i);
+		if (!blk_key->devs[i] || !blk_get_queue(blk_key->devs[i])) {
 			fscrypt_err(inode, "couldn't get request_queue");
 			err = -EAGAIN;
 			goto fail;
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index dfa072fa8081..9a6d375cbe4b 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2509,14 +2509,18 @@ static int f2fs_get_num_devices(struct super_block *sb)
 	return 1;
 }
 
-static void f2fs_get_devices(struct super_block *sb,
-			     struct request_queue **devs)
+static struct request_queue *f2fs_get_device(struct super_block *sb,
+					     unsigned int device_index)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
-	int i;
 
-	for (i = 0; i < sbi->s_ndevs; i++)
-		devs[i] = bdev_get_queue(FDEV(i).bdev);
+	if (WARN_ON_ONCE(device_index >= f2fs_get_num_devices(sb)))
+		return NULL;
+
+	if (!f2fs_is_multi_device(sbi))
+		return bdev_get_queue(sb->s_bdev);
+
+	return bdev_get_queue(FDEV(device_index).bdev);
 }
 
 static const struct fscrypt_operations f2fs_cryptops = {
@@ -2529,7 +2533,7 @@ static const struct fscrypt_operations f2fs_cryptops = {
 	.has_stable_inodes	= f2fs_has_stable_inodes,
 	.get_ino_and_lblk_bits	= f2fs_get_ino_and_lblk_bits,
 	.get_num_devices	= f2fs_get_num_devices,
-	.get_devices		= f2fs_get_devices,
+	.get_device		= f2fs_get_device,
 };
 #endif
 
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 991ff8575d0e..d835fd19a20a 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -70,8 +70,8 @@ struct fscrypt_operations {
 	void (*get_ino_and_lblk_bits)(struct super_block *sb,
 				      int *ino_bits_ret, int *lblk_bits_ret);
 	int (*get_num_devices)(struct super_block *sb);
-	void (*get_devices)(struct super_block *sb,
-			    struct request_queue **devs);
+	struct request_queue *(*get_device)(struct super_block *sb,
+					    unsigned int dev_index);
 };
 
 static inline struct fscrypt_info *fscrypt_get_info(const struct inode *inode)
-- 
2.28.0.806.g8561365e88-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 2/3] fscrypt: Add metadata encryption support
  2020-10-05  7:36 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-05  7:36   ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, Satya Tangirala

Introduces functions that help with metadata encryption.

In particular, we introduce:

fscrypt_setup_metadata_encryption() - filesystems should call this function
to set up metadata encryption on a super block with the encryption
algorithm (the desired FSCRYPT_MODE_*) and the key descriptor of the
encryption key. The key descriptor is looked up in the logon keyring of the
current session using "fscrypt:" as the descriptor prefix.

fscrypt_metadata_crypt_bio() - filesystems should call this function on a
bio that it wants metadata crypted. This function will set a bio-crypt-ctx
on the bio if the metadata key was set up with
fscrypt_setup_metadata_encryption(). The DUN for the first block in the bio
is the offset of that block from the start of the filesystem.

fscrypt_free_metadata_encryption() - this function should be called when
the super block is being freed. It ensures that the metadata encryption key
is evicted, if necessary, from devices.

Note that the filesystem (rather than fscrypt) controls precisely which
blocks are encrypted with the metadata encryption key and which blocks are
encrypted with other keys/not encrypted at all. Fscrypt only provides some
convenience functions that ultimately help encrypt a bio with the metadata
encryption key when desired.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/crypto/Kconfig           |   6 +
 fs/crypto/Makefile          |   1 +
 fs/crypto/fscrypt_private.h |  19 ++++
 fs/crypto/inline_crypt.c    |  18 ---
 fs/crypto/metadata_crypt.c  | 220 ++++++++++++++++++++++++++++++++++++
 include/linux/fs.h          |   3 +
 include/linux/fscrypt.h     |  47 ++++++++
 7 files changed, 296 insertions(+), 18 deletions(-)
 create mode 100644 fs/crypto/metadata_crypt.c

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index a5f5c30368a2..3010e91f6659 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -30,3 +30,9 @@ config FS_ENCRYPTION_INLINE_CRYPT
 	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
 	help
 	  Enable fscrypt to use inline encryption hardware if available.
+
+config FS_ENCRYPTION_METADATA
+	bool "Enable metadata encryption with fscrypt"
+	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
+	help
+	  Enable fscrypt to encrypt metadata.
\ No newline at end of file
diff --git a/fs/crypto/Makefile b/fs/crypto/Makefile
index 652c7180ec6d..8403c7956983 100644
--- a/fs/crypto/Makefile
+++ b/fs/crypto/Makefile
@@ -12,3 +12,4 @@ fscrypto-y := crypto.o \
 
 fscrypto-$(CONFIG_BLOCK) += bio.o
 fscrypto-$(CONFIG_FS_ENCRYPTION_INLINE_CRYPT) += inline_crypt.o
+fscrypto-$(CONFIG_FS_ENCRYPTION_METADATA) += metadata_crypt.o
\ No newline at end of file
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 8117a61b6f55..dca254590a70 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -327,6 +327,25 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf);
 
 /* inline_crypt.c */
+
+static inline int fscrypt_get_num_devices(struct super_block *sb)
+{
+	if (sb->s_cop->get_num_devices)
+		return sb->s_cop->get_num_devices(sb);
+	return 1;
+}
+
+static inline struct request_queue *fscrypt_get_device(struct super_block *sb,
+						unsigned int device_index)
+{
+	if (sb->s_cop->get_device)
+		return sb->s_cop->get_device(sb, device_index);
+	else if (WARN_ON_ONCE(device_index != 0))
+		return NULL;
+	else
+		return bdev_get_queue(sb->s_bdev);
+}
+
 #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
 int fscrypt_select_encryption_impl(struct fscrypt_info *ci);
 
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 5bbce79df638..f8f7363ebcd0 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -26,24 +26,6 @@ struct fscrypt_blk_crypto_key {
 	struct request_queue *devs[];
 };
 
-static int fscrypt_get_num_devices(struct super_block *sb)
-{
-	if (sb->s_cop->get_num_devices)
-		return sb->s_cop->get_num_devices(sb);
-	return 1;
-}
-
-static struct request_queue *fscrypt_get_device(struct super_block *sb,
-						unsigned int device_index)
-{
-	if (sb->s_cop->get_device)
-		return sb->s_cop->get_device(sb, device_index);
-	else if (WARN_ON_ONCE(device_index != 0))
-		return NULL;
-	else
-		return bdev_get_queue(sb->s_bdev);
-}
-
 static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
 {
 	struct super_block *sb = ci->ci_inode->i_sb;
diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
new file mode 100644
index 000000000000..5e16df130509
--- /dev/null
+++ b/fs/crypto/metadata_crypt.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Metadata encryption support for fscrypt
+ *
+ * Copyright 2020 Google LLC
+ */
+
+#include <keys/user-type.h>
+#include <linux/blk-crypto.h>
+#include <linux/blkdev.h>
+#include <linux/buffer_head.h>
+#include <linux/sched/mm.h>
+
+#include "fscrypt_private.h"
+
+/* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
+static int fscrypt_metadata_get_key_from_id(const char *prefix,
+					    char *descriptor_hex,
+					    unsigned int min_keysize,
+					    char *raw_key)
+{
+	char *description;
+	struct key *key;
+	const struct user_key_payload *ukp;
+	const struct fscrypt_key *payload;
+	int err = -ENOKEY;
+
+	if (strlen(descriptor_hex) != FSCRYPT_KEY_DESCRIPTOR_SIZE * 2)
+		return -EINVAL;
+
+	description = kasprintf(GFP_NOFS, "%s%s", prefix, descriptor_hex);
+	if (!description)
+		return -ENOMEM;
+
+	key = request_key(&key_type_logon, description, NULL);
+	kfree(description);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	down_read(&key->sem);
+	ukp = user_key_payload_locked(key);
+
+	if (!ukp) /* was the key revoked before we acquired its semaphore? */
+		goto out;
+
+	payload = (const struct fscrypt_key *)ukp->data;
+
+	if (ukp->datalen != sizeof(struct fscrypt_key) ||
+	    payload->size < 1 || payload->size > FSCRYPT_MAX_KEY_SIZE) {
+		fscrypt_warn(NULL,
+			     "key with description '%s' has invalid payload",
+			     key->description);
+		goto out;
+	}
+
+	if (payload->size < min_keysize) {
+		fscrypt_warn(NULL,
+			     "key with description '%s' is too short (got %u bytes, need %u+ bytes)",
+			     key->description, payload->size, min_keysize);
+		goto out;
+	}
+
+	memcpy(raw_key, payload->raw, min_keysize);
+	err = 0;
+
+out:
+	up_read(&key->sem);
+	key_put(key);
+
+	return err;
+}
+
+/**
+ * fscrypt_setup_metadata_encryption() - prepare a super_block for metadata
+ *					 encryption
+ * @sb: The super_block to set up metadata encryption for
+ * @key_desc_hex: The key descriptor (in hex) to look for in the logon keyring.
+ * @fscrypt_mode_num: The FSCRYPT_MODE_* to use as the encryption algorithm.
+ *
+ * Return: 0 on success, negative number on error.
+ */
+int fscrypt_setup_metadata_encryption(struct super_block *sb,
+				      char *key_desc_hex,
+				      int fscrypt_mode_num)
+{
+	int err = 0;
+	enum blk_crypto_mode_num crypto_mode;
+	unsigned int lblk_bits = 64;
+	unsigned int dun_bytes;
+	unsigned int dummy;
+	char raw_key[FSCRYPT_MAX_KEY_SIZE];
+
+	if (fscrypt_mode_num > __FSCRYPT_MODE_MAX || fscrypt_mode_num < 0 ||
+	    !fscrypt_modes[fscrypt_mode_num].cipher_str) {
+		fscrypt_warn(NULL, "Invalid fscrypt mode %d specified for metadata encryption.",
+			     fscrypt_mode_num);
+		return -EOPNOTSUPP;
+	}
+
+	if (sb->s_cop->get_ino_and_lblk_bits)
+		sb->s_cop->get_ino_and_lblk_bits(sb, &dummy, &lblk_bits);
+	dun_bytes = DIV_ROUND_UP(lblk_bits, 8);
+
+	if (fscrypt_modes[fscrypt_mode_num].ivsize < dun_bytes) {
+		fscrypt_warn(NULL, "The fscrypt mode only supports %d DUN bytes, but FS requires support for %d DUN bytes.",
+			     fscrypt_modes[fscrypt_mode_num].ivsize, dun_bytes);
+		return -EOPNOTSUPP;
+	}
+
+	crypto_mode = fscrypt_modes[fscrypt_mode_num].blk_crypto_mode;
+
+	err = fscrypt_metadata_get_key_from_id(
+					FSCRYPT_KEY_DESC_PREFIX,
+					key_desc_hex,
+					fscrypt_modes[fscrypt_mode_num].keysize,
+					raw_key);
+	if (err)
+		goto out;
+
+	sb->s_metadata_key = kzalloc(sizeof(*sb->s_metadata_key), GFP_NOFS);
+	if (!sb->s_metadata_key) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = blk_crypto_init_key(sb->s_metadata_key, raw_key, crypto_mode,
+				  dun_bytes, sb->s_blocksize);
+	if (err)
+		goto out_free_key;
+
+	err = blk_crypto_start_using_key(sb->s_metadata_key,
+					 bdev_get_queue(sb->s_bdev));
+	if (err)
+		goto out_free_key;
+
+	goto out;
+out_free_key:
+	fscrypt_free_metadata_encryption(sb);
+out:
+	memzero_explicit(raw_key, sizeof(raw_key));
+	return err;
+}
+
+/**
+ * fscrypt_metadata_crypt_prepare_all_devices() - prepare all devices used by
+ *					the filesystem for metadata encryption.
+ * @sb: The super_block whose devices to prepare
+ *
+ * This function should be called when the filesystem has determined all its
+ * devices. This might happen only after some initial setup, which is why
+ * this is a separate function from fscrypt_setup_metadata_encryption().
+ *
+ * Return: 0 on success, negative on error.
+ */
+int fscrypt_metadata_crypt_prepare_all_devices(struct super_block *sb)
+{
+	int num_devices;
+	int i;
+	struct request_queue *q;
+
+	if (!sb->s_metadata_key)
+		return 0;
+
+	num_devices = fscrypt_get_num_devices(sb);
+	for (i = 0; i < num_devices; i++) {
+		q = fscrypt_get_device(sb, i);
+		if (!q || blk_crypto_start_using_key(sb->s_metadata_key, q))
+			return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+/**
+ * fscrypt_free_metadata_encryption() - free metadata encryption fields in
+ *					super_block.
+ * @sb: The super_block to free metatdata encryption fields from
+ */
+void fscrypt_free_metadata_encryption(struct super_block *sb)
+{
+	int num_devices;
+	int i;
+	struct request_queue *q;
+
+	if (!sb->s_metadata_key)
+		return;
+
+	num_devices = fscrypt_get_num_devices(sb);
+
+	for (i = 0; i < num_devices; i++) {
+		q = fscrypt_get_device(sb, i);
+		if (WARN_ON(!q))
+			continue;
+		blk_crypto_evict_key(q, sb->s_metadata_key);
+	}
+
+	memzero_explicit(sb->s_metadata_key, sizeof(*sb->s_metadata_key));
+	kzfree(sb->s_metadata_key);
+	sb->s_metadata_key = NULL;
+}
+
+/**
+ * fscrypt_metadata_crypt_bio() - Add metadata encryption context to bio.
+ *
+ * @bio: The bio to add the encryption context to
+ * @lblk: The logical block number within the filesystem at which this bio
+ *	  starts reading/writing data.
+ * @sb: The superblock of the filesystem
+ * @gfp_mask: gfp_mask for bio_crypt_context allocation
+ */
+void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
+				struct super_block *sb, gfp_t gfp_mask)
+{
+	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { 0 };
+
+	if (!sb->s_metadata_key)
+		return;
+
+	dun[0] = lblk;
+	bio_crypt_set_ctx(bio, sb->s_metadata_key, dun, gfp_mask);
+}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7519ae003a08..aba3b0e2d56f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1438,6 +1438,9 @@ struct super_block {
 	const struct fscrypt_operations	*s_cop;
 	struct key		*s_master_keys; /* master crypto keys in use */
 #endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+	struct blk_crypto_key	*s_metadata_key;
+#endif
 #ifdef CONFIG_FS_VERITY
 	const struct fsverity_operations *s_vop;
 #endif
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index d835fd19a20a..f7cdc8627984 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -594,6 +594,53 @@ static inline bool fscrypt_mergeable_bio_bh(struct bio *bio,
 }
 #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 
+/* metadata_crypt.c */
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+
+int fscrypt_setup_metadata_encryption(struct super_block *sb,
+				      char *key_desc_hex,
+				      int fscrypt_mode_num);
+
+int fscrypt_metadata_crypt_prepare_all_devices(struct super_block *sb);
+
+void fscrypt_free_metadata_encryption(struct super_block *sb);
+
+void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
+				struct super_block *sb, gfp_t gfp_mask);
+
+static inline bool fscrypt_metadata_crypted(struct super_block *sb)
+{
+	return sb->s_metadata_key;
+}
+
+#else /* CONFIG_FS_ENCRYPTION_METADATA */
+
+static inline int fscrypt_setup_metadata_encryption(struct super_block *sb,
+						    char *key_desc_hex,
+						    int fscrypt_mode_num)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int
+fscrypt_metadata_crypt_prepare_all_devices(struct super_block *sb)
+{
+	return 0;
+}
+
+static inline void fscrypt_free_metadata_encryption(struct super_block *sb) { }
+
+static inline void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
+					      struct super_block *sb,
+					      gfp_t gfp_mask) { }
+
+static inline bool fscrypt_metadata_crypted(struct super_block *sb)
+{
+	return false;
+}
+
+#endif /* CONFIG_FS_ENCRYPTION_METADATA */
+
 /**
  * fscrypt_inode_uses_inline_crypto() - test whether an inode uses inline
  *					encryption
-- 
2.28.0.806.g8561365e88-goog


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [f2fs-dev] [PATCH 2/3] fscrypt: Add metadata encryption support
@ 2020-10-05  7:36   ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-fscrypt, Satya Tangirala, linux-kernel, linux-f2fs-devel

Introduces functions that help with metadata encryption.

In particular, we introduce:

fscrypt_setup_metadata_encryption() - filesystems should call this function
to set up metadata encryption on a super block with the encryption
algorithm (the desired FSCRYPT_MODE_*) and the key descriptor of the
encryption key. The key descriptor is looked up in the logon keyring of the
current session using "fscrypt:" as the descriptor prefix.

fscrypt_metadata_crypt_bio() - filesystems should call this function on a
bio that it wants metadata crypted. This function will set a bio-crypt-ctx
on the bio if the metadata key was set up with
fscrypt_setup_metadata_encryption(). The DUN for the first block in the bio
is the offset of that block from the start of the filesystem.

fscrypt_free_metadata_encryption() - this function should be called when
the super block is being freed. It ensures that the metadata encryption key
is evicted, if necessary, from devices.

Note that the filesystem (rather than fscrypt) controls precisely which
blocks are encrypted with the metadata encryption key and which blocks are
encrypted with other keys/not encrypted at all. Fscrypt only provides some
convenience functions that ultimately help encrypt a bio with the metadata
encryption key when desired.

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 fs/crypto/Kconfig           |   6 +
 fs/crypto/Makefile          |   1 +
 fs/crypto/fscrypt_private.h |  19 ++++
 fs/crypto/inline_crypt.c    |  18 ---
 fs/crypto/metadata_crypt.c  | 220 ++++++++++++++++++++++++++++++++++++
 include/linux/fs.h          |   3 +
 include/linux/fscrypt.h     |  47 ++++++++
 7 files changed, 296 insertions(+), 18 deletions(-)
 create mode 100644 fs/crypto/metadata_crypt.c

diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index a5f5c30368a2..3010e91f6659 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -30,3 +30,9 @@ config FS_ENCRYPTION_INLINE_CRYPT
 	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
 	help
 	  Enable fscrypt to use inline encryption hardware if available.
+
+config FS_ENCRYPTION_METADATA
+	bool "Enable metadata encryption with fscrypt"
+	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
+	help
+	  Enable fscrypt to encrypt metadata.
\ No newline at end of file
diff --git a/fs/crypto/Makefile b/fs/crypto/Makefile
index 652c7180ec6d..8403c7956983 100644
--- a/fs/crypto/Makefile
+++ b/fs/crypto/Makefile
@@ -12,3 +12,4 @@ fscrypto-y := crypto.o \
 
 fscrypto-$(CONFIG_BLOCK) += bio.o
 fscrypto-$(CONFIG_FS_ENCRYPTION_INLINE_CRYPT) += inline_crypt.o
+fscrypto-$(CONFIG_FS_ENCRYPTION_METADATA) += metadata_crypt.o
\ No newline at end of file
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 8117a61b6f55..dca254590a70 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -327,6 +327,25 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf);
 
 /* inline_crypt.c */
+
+static inline int fscrypt_get_num_devices(struct super_block *sb)
+{
+	if (sb->s_cop->get_num_devices)
+		return sb->s_cop->get_num_devices(sb);
+	return 1;
+}
+
+static inline struct request_queue *fscrypt_get_device(struct super_block *sb,
+						unsigned int device_index)
+{
+	if (sb->s_cop->get_device)
+		return sb->s_cop->get_device(sb, device_index);
+	else if (WARN_ON_ONCE(device_index != 0))
+		return NULL;
+	else
+		return bdev_get_queue(sb->s_bdev);
+}
+
 #ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
 int fscrypt_select_encryption_impl(struct fscrypt_info *ci);
 
diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c
index 5bbce79df638..f8f7363ebcd0 100644
--- a/fs/crypto/inline_crypt.c
+++ b/fs/crypto/inline_crypt.c
@@ -26,24 +26,6 @@ struct fscrypt_blk_crypto_key {
 	struct request_queue *devs[];
 };
 
-static int fscrypt_get_num_devices(struct super_block *sb)
-{
-	if (sb->s_cop->get_num_devices)
-		return sb->s_cop->get_num_devices(sb);
-	return 1;
-}
-
-static struct request_queue *fscrypt_get_device(struct super_block *sb,
-						unsigned int device_index)
-{
-	if (sb->s_cop->get_device)
-		return sb->s_cop->get_device(sb, device_index);
-	else if (WARN_ON_ONCE(device_index != 0))
-		return NULL;
-	else
-		return bdev_get_queue(sb->s_bdev);
-}
-
 static unsigned int fscrypt_get_dun_bytes(const struct fscrypt_info *ci)
 {
 	struct super_block *sb = ci->ci_inode->i_sb;
diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
new file mode 100644
index 000000000000..5e16df130509
--- /dev/null
+++ b/fs/crypto/metadata_crypt.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Metadata encryption support for fscrypt
+ *
+ * Copyright 2020 Google LLC
+ */
+
+#include <keys/user-type.h>
+#include <linux/blk-crypto.h>
+#include <linux/blkdev.h>
+#include <linux/buffer_head.h>
+#include <linux/sched/mm.h>
+
+#include "fscrypt_private.h"
+
+/* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
+static int fscrypt_metadata_get_key_from_id(const char *prefix,
+					    char *descriptor_hex,
+					    unsigned int min_keysize,
+					    char *raw_key)
+{
+	char *description;
+	struct key *key;
+	const struct user_key_payload *ukp;
+	const struct fscrypt_key *payload;
+	int err = -ENOKEY;
+
+	if (strlen(descriptor_hex) != FSCRYPT_KEY_DESCRIPTOR_SIZE * 2)
+		return -EINVAL;
+
+	description = kasprintf(GFP_NOFS, "%s%s", prefix, descriptor_hex);
+	if (!description)
+		return -ENOMEM;
+
+	key = request_key(&key_type_logon, description, NULL);
+	kfree(description);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	down_read(&key->sem);
+	ukp = user_key_payload_locked(key);
+
+	if (!ukp) /* was the key revoked before we acquired its semaphore? */
+		goto out;
+
+	payload = (const struct fscrypt_key *)ukp->data;
+
+	if (ukp->datalen != sizeof(struct fscrypt_key) ||
+	    payload->size < 1 || payload->size > FSCRYPT_MAX_KEY_SIZE) {
+		fscrypt_warn(NULL,
+			     "key with description '%s' has invalid payload",
+			     key->description);
+		goto out;
+	}
+
+	if (payload->size < min_keysize) {
+		fscrypt_warn(NULL,
+			     "key with description '%s' is too short (got %u bytes, need %u+ bytes)",
+			     key->description, payload->size, min_keysize);
+		goto out;
+	}
+
+	memcpy(raw_key, payload->raw, min_keysize);
+	err = 0;
+
+out:
+	up_read(&key->sem);
+	key_put(key);
+
+	return err;
+}
+
+/**
+ * fscrypt_setup_metadata_encryption() - prepare a super_block for metadata
+ *					 encryption
+ * @sb: The super_block to set up metadata encryption for
+ * @key_desc_hex: The key descriptor (in hex) to look for in the logon keyring.
+ * @fscrypt_mode_num: The FSCRYPT_MODE_* to use as the encryption algorithm.
+ *
+ * Return: 0 on success, negative number on error.
+ */
+int fscrypt_setup_metadata_encryption(struct super_block *sb,
+				      char *key_desc_hex,
+				      int fscrypt_mode_num)
+{
+	int err = 0;
+	enum blk_crypto_mode_num crypto_mode;
+	unsigned int lblk_bits = 64;
+	unsigned int dun_bytes;
+	unsigned int dummy;
+	char raw_key[FSCRYPT_MAX_KEY_SIZE];
+
+	if (fscrypt_mode_num > __FSCRYPT_MODE_MAX || fscrypt_mode_num < 0 ||
+	    !fscrypt_modes[fscrypt_mode_num].cipher_str) {
+		fscrypt_warn(NULL, "Invalid fscrypt mode %d specified for metadata encryption.",
+			     fscrypt_mode_num);
+		return -EOPNOTSUPP;
+	}
+
+	if (sb->s_cop->get_ino_and_lblk_bits)
+		sb->s_cop->get_ino_and_lblk_bits(sb, &dummy, &lblk_bits);
+	dun_bytes = DIV_ROUND_UP(lblk_bits, 8);
+
+	if (fscrypt_modes[fscrypt_mode_num].ivsize < dun_bytes) {
+		fscrypt_warn(NULL, "The fscrypt mode only supports %d DUN bytes, but FS requires support for %d DUN bytes.",
+			     fscrypt_modes[fscrypt_mode_num].ivsize, dun_bytes);
+		return -EOPNOTSUPP;
+	}
+
+	crypto_mode = fscrypt_modes[fscrypt_mode_num].blk_crypto_mode;
+
+	err = fscrypt_metadata_get_key_from_id(
+					FSCRYPT_KEY_DESC_PREFIX,
+					key_desc_hex,
+					fscrypt_modes[fscrypt_mode_num].keysize,
+					raw_key);
+	if (err)
+		goto out;
+
+	sb->s_metadata_key = kzalloc(sizeof(*sb->s_metadata_key), GFP_NOFS);
+	if (!sb->s_metadata_key) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	err = blk_crypto_init_key(sb->s_metadata_key, raw_key, crypto_mode,
+				  dun_bytes, sb->s_blocksize);
+	if (err)
+		goto out_free_key;
+
+	err = blk_crypto_start_using_key(sb->s_metadata_key,
+					 bdev_get_queue(sb->s_bdev));
+	if (err)
+		goto out_free_key;
+
+	goto out;
+out_free_key:
+	fscrypt_free_metadata_encryption(sb);
+out:
+	memzero_explicit(raw_key, sizeof(raw_key));
+	return err;
+}
+
+/**
+ * fscrypt_metadata_crypt_prepare_all_devices() - prepare all devices used by
+ *					the filesystem for metadata encryption.
+ * @sb: The super_block whose devices to prepare
+ *
+ * This function should be called when the filesystem has determined all its
+ * devices. This might happen only after some initial setup, which is why
+ * this is a separate function from fscrypt_setup_metadata_encryption().
+ *
+ * Return: 0 on success, negative on error.
+ */
+int fscrypt_metadata_crypt_prepare_all_devices(struct super_block *sb)
+{
+	int num_devices;
+	int i;
+	struct request_queue *q;
+
+	if (!sb->s_metadata_key)
+		return 0;
+
+	num_devices = fscrypt_get_num_devices(sb);
+	for (i = 0; i < num_devices; i++) {
+		q = fscrypt_get_device(sb, i);
+		if (!q || blk_crypto_start_using_key(sb->s_metadata_key, q))
+			return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+/**
+ * fscrypt_free_metadata_encryption() - free metadata encryption fields in
+ *					super_block.
+ * @sb: The super_block to free metatdata encryption fields from
+ */
+void fscrypt_free_metadata_encryption(struct super_block *sb)
+{
+	int num_devices;
+	int i;
+	struct request_queue *q;
+
+	if (!sb->s_metadata_key)
+		return;
+
+	num_devices = fscrypt_get_num_devices(sb);
+
+	for (i = 0; i < num_devices; i++) {
+		q = fscrypt_get_device(sb, i);
+		if (WARN_ON(!q))
+			continue;
+		blk_crypto_evict_key(q, sb->s_metadata_key);
+	}
+
+	memzero_explicit(sb->s_metadata_key, sizeof(*sb->s_metadata_key));
+	kzfree(sb->s_metadata_key);
+	sb->s_metadata_key = NULL;
+}
+
+/**
+ * fscrypt_metadata_crypt_bio() - Add metadata encryption context to bio.
+ *
+ * @bio: The bio to add the encryption context to
+ * @lblk: The logical block number within the filesystem at which this bio
+ *	  starts reading/writing data.
+ * @sb: The superblock of the filesystem
+ * @gfp_mask: gfp_mask for bio_crypt_context allocation
+ */
+void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
+				struct super_block *sb, gfp_t gfp_mask)
+{
+	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { 0 };
+
+	if (!sb->s_metadata_key)
+		return;
+
+	dun[0] = lblk;
+	bio_crypt_set_ctx(bio, sb->s_metadata_key, dun, gfp_mask);
+}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7519ae003a08..aba3b0e2d56f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1438,6 +1438,9 @@ struct super_block {
 	const struct fscrypt_operations	*s_cop;
 	struct key		*s_master_keys; /* master crypto keys in use */
 #endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+	struct blk_crypto_key	*s_metadata_key;
+#endif
 #ifdef CONFIG_FS_VERITY
 	const struct fsverity_operations *s_vop;
 #endif
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index d835fd19a20a..f7cdc8627984 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -594,6 +594,53 @@ static inline bool fscrypt_mergeable_bio_bh(struct bio *bio,
 }
 #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */
 
+/* metadata_crypt.c */
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+
+int fscrypt_setup_metadata_encryption(struct super_block *sb,
+				      char *key_desc_hex,
+				      int fscrypt_mode_num);
+
+int fscrypt_metadata_crypt_prepare_all_devices(struct super_block *sb);
+
+void fscrypt_free_metadata_encryption(struct super_block *sb);
+
+void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
+				struct super_block *sb, gfp_t gfp_mask);
+
+static inline bool fscrypt_metadata_crypted(struct super_block *sb)
+{
+	return sb->s_metadata_key;
+}
+
+#else /* CONFIG_FS_ENCRYPTION_METADATA */
+
+static inline int fscrypt_setup_metadata_encryption(struct super_block *sb,
+						    char *key_desc_hex,
+						    int fscrypt_mode_num)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int
+fscrypt_metadata_crypt_prepare_all_devices(struct super_block *sb)
+{
+	return 0;
+}
+
+static inline void fscrypt_free_metadata_encryption(struct super_block *sb) { }
+
+static inline void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
+					      struct super_block *sb,
+					      gfp_t gfp_mask) { }
+
+static inline bool fscrypt_metadata_crypted(struct super_block *sb)
+{
+	return false;
+}
+
+#endif /* CONFIG_FS_ENCRYPTION_METADATA */
+
 /**
  * fscrypt_inode_uses_inline_crypto() - test whether an inode uses inline
  *					encryption
-- 
2.28.0.806.g8561365e88-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [PATCH 3/3] f2fs: Add metadata encryption support
  2020-10-05  7:36 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-05  7:36   ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel, Satya Tangirala

Wire up metadata encryption support with the fscrypt metadata crypt
additions.

Introduces a new mount option for metadata encryption -
metadata_crypt_key=%s. The argument to this option is the key descriptor of
the metadata encryption key in hex. This key descriptor will be looked up
in the logon keyring with the "fscrypt:" prefix.

E.g. one might pass "-o metadata_crypt_key=ababcdcdefef0101" as the f2fs
mount option to the kernel, when the logon keyring has a key with the
descriptor "fscrypt:ababcdcdefef0101".

Right now, the superblock of the filesystem is itself not encrypted. F2FS
reads the superblock using sb_bread, which uses the bd_inode of the block
device as the address space for any data it reads from the block device -
the data read under the bd_inode address space must be what is physically
present on disk (i.e. if the superblock is encrypted, then the ciphertext
of the superblock must be present in the page cache in the bd_inode's
address space), but f2fs requires that the superblock is decrypted by
blk-crypto, which would put the decrypted page contents into the page cache
instead. We could make f2fs read the superblock by submitting bios directly
with a separate address space, but we choose to just not encrypt the
superblock for now.

Not encrypting the superblock allows us to store the encryption algorithm
used for metadata encryption within the superblock itself, which simplifies
a few things. The userspace tools will store the encryption algorithm in
the superblock when formatting the FS.

Direct I/O with metadata encryption is also not supported for now.
Attempts to do direct I/O on a metadata encrypted F2FS filesystem will fall
back to using buffered I/O (just as attempts to do direct I/O on fscrypt
encrypted files also fall back to buffered I/O).

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 Documentation/filesystems/f2fs.rst | 12 ++++++
 fs/f2fs/data.c                     | 24 +++++++----
 fs/f2fs/f2fs.h                     |  2 +
 fs/f2fs/super.c                    | 67 ++++++++++++++++++++++++++++--
 include/linux/f2fs_fs.h            |  3 +-
 5 files changed, 95 insertions(+), 13 deletions(-)

diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst
index ec8d99703ecb..94a294874707 100644
--- a/Documentation/filesystems/f2fs.rst
+++ b/Documentation/filesystems/f2fs.rst
@@ -266,6 +266,18 @@ inlinecrypt		 When possible, encrypt/decrypt the contents of encrypted
 			 inline encryption hardware. The on-disk format is
 			 unaffected. For more details, see
 			 Documentation/block/inline-encryption.rst.
+metadata_crypt_key=%s	 Specify the metadata encryption key for the filesystem.
+			 The argument to this option is the key descriptor of
+			 the metadata encryption key in hex. This key descriptor
+			 will be looked up in the logon keyring with the
+			 "fscrypt:" prefix. So e.g. one might pass "-o
+			 metadata_crypt_key=ababcdcdefef0101" as the f2fs mount
+			 option to the kernel, when the logon keyring has a key
+			 with the descriptor "fscrypt:ababcdcdefef0101".
+			 When an F2FS filesystem has metadata encryption enabled,
+			 all blocks in the FS other than the superblock are
+			 encrypted with the metadata encryption key. The
+			 superblock itself is stored in plaintext.
 ======================== ============================================================
 
 Debugfs Entries
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 73683e58a08d..1b65313b57c8 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -460,8 +460,8 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages)
 	return bio;
 }
 
-static void f2fs_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
-				  pgoff_t first_idx,
+static void f2fs_set_bio_crypt_ctx(struct bio *bio, block_t blk_addr,
+				  const struct inode *inode, pgoff_t first_idx,
 				  const struct f2fs_io_info *fio,
 				  gfp_t gfp_mask)
 {
@@ -469,8 +469,13 @@ static void f2fs_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
 	 * The f2fs garbage collector sets ->encrypted_page when it wants to
 	 * read/write raw data without encryption.
 	 */
-	if (!fio || !fio->encrypted_page)
-		fscrypt_set_bio_crypt_ctx(bio, inode, first_idx, gfp_mask);
+	if (!fio || !fio->encrypted_page) {
+		if (fscrypt_needs_contents_encryption(inode))
+			fscrypt_set_bio_crypt_ctx(bio, inode, first_idx, gfp_mask);
+		else
+			fscrypt_metadata_crypt_bio(bio, blk_addr, inode->i_sb,
+						   gfp_mask);
+	}
 }
 
 static bool f2fs_crypt_mergeable_bio(struct bio *bio, const struct inode *inode,
@@ -712,7 +717,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
 	/* Allocate a new bio */
 	bio = __bio_alloc(fio, 1);
 
-	f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
+	f2fs_set_bio_crypt_ctx(bio, fio->new_blkaddr, fio->page->mapping->host,
 			       fio->page->index, fio, GFP_NOIO);
 
 	if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
@@ -918,7 +923,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
 	if (!bio) {
 		bio = __bio_alloc(fio, BIO_MAX_PAGES);
 		__attach_io_flag(fio);
-		f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
+		f2fs_set_bio_crypt_ctx(bio, fio->new_blkaddr,
+				       fio->page->mapping->host,
 				       fio->page->index, fio, GFP_NOIO);
 		bio_set_op_attrs(bio, fio->op, fio->op_flags);
 
@@ -992,7 +998,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
 			goto skip;
 		}
 		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
-		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
+		f2fs_set_bio_crypt_ctx(io->bio, fio->new_blkaddr,
+				       fio->page->mapping->host,
 				       bio_page->index, fio, GFP_NOIO);
 		io->fio = *fio;
 	}
@@ -1039,9 +1046,8 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
 	if (!bio)
 		return ERR_PTR(-ENOMEM);
 
-	f2fs_set_bio_crypt_ctx(bio, inode, first_idx, NULL, GFP_NOFS);
-
 	f2fs_target_device(sbi, blkaddr, bio);
+	f2fs_set_bio_crypt_ctx(bio, blkaddr, inode, first_idx, NULL, GFP_NOFS);
 	bio->bi_end_io = f2fs_read_end_io;
 	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d9e52a7f3702..8c5626a6f684 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4095,6 +4095,8 @@ static inline bool f2fs_force_buffered_io(struct inode *inode,
 
 	if (f2fs_post_read_required(inode))
 		return true;
+	if (fscrypt_metadata_crypted(sbi->sb))
+		return true;
 	if (f2fs_is_multi_device(sbi))
 		return true;
 	/*
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 9a6d375cbe4b..1c14c823a4e9 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -146,6 +146,7 @@ enum {
 	Opt_compress_algorithm,
 	Opt_compress_log_size,
 	Opt_compress_extension,
+	Opt_metadata_crypt_key,
 	Opt_err,
 };
 
@@ -213,6 +214,7 @@ static match_table_t f2fs_tokens = {
 	{Opt_compress_algorithm, "compress_algorithm=%s"},
 	{Opt_compress_log_size, "compress_log_size=%u"},
 	{Opt_compress_extension, "compress_extension=%s"},
+	{Opt_metadata_crypt_key, "metadata_crypt_key=%s"},
 	{Opt_err, NULL},
 };
 
@@ -465,6 +467,10 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 #ifdef CONFIG_F2FS_FS_COMPRESSION
 	unsigned char (*ext)[F2FS_EXTENSION_LEN];
 	int ext_cnt;
+#endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+	char *key_desc_hex = NULL;
+	int metadata_crypt_alg = le32_to_cpu(sbi->raw_super->metadata_crypt_alg);
 #endif
 	char *p, *name;
 	int arg = 0;
@@ -937,6 +943,35 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 		case Opt_compress_extension:
 			f2fs_info(sbi, "compression options not supported");
 			break;
+#endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+		case Opt_metadata_crypt_key:
+			if (!metadata_crypt_alg) {
+				f2fs_err(sbi, "Filesystem doesn't have metadata encryption enabled, but a metadata encryption key was provided");
+				return -EINVAL;
+			}
+			if (is_remount) {
+				f2fs_warn(sbi, "Ignoring metadata crypt key specified for remount");
+				break;
+			}
+
+			if (fscrypt_metadata_crypted(sb)) {
+				f2fs_err(sbi, "Multiple metadata crypt key options specified");
+				return -EINVAL;
+			}
+
+			key_desc_hex = match_strdup(&args[0]);
+			if (!key_desc_hex)
+				return -ENOMEM;
+
+			if (fscrypt_setup_metadata_encryption(sb, key_desc_hex,
+							metadata_crypt_alg)) {
+				f2fs_err(sbi, "Could not setup metadata encryption");
+				kfree(key_desc_hex);
+				return -EINVAL;
+			}
+			kfree(key_desc_hex);
+			break;
 #endif
 		default:
 			f2fs_err(sbi, "Unrecognized mount option \"%s\" or missing value",
@@ -964,6 +999,13 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 		return -EINVAL;
 	}
 #endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+	if (metadata_crypt_alg &&
+	    !fscrypt_metadata_crypted(sb)) {
+		f2fs_err(sbi, "Filesystem has metadata encryption. Please provide metadata encryption key to mount filesystem");
+		return -EINVAL;
+	}
+#endif
 
 	if (F2FS_IO_SIZE_BITS(sbi) && !f2fs_lfs_mode(sbi)) {
 		f2fs_err(sbi, "Should set mode=lfs with %uKB-sized IO",
@@ -1249,6 +1291,8 @@ static void f2fs_put_super(struct super_block *sb)
 	iput(sbi->meta_inode);
 	sbi->meta_inode = NULL;
 
+	fscrypt_free_metadata_encryption(sb);
+
 	/*
 	 * iput() can update stat information, if f2fs_write_checkpoint()
 	 * above failed with error.
@@ -2504,6 +2548,9 @@ static int f2fs_get_num_devices(struct super_block *sb)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 
+	if (!sbi)
+		return 0;
+
 	if (f2fs_is_multi_device(sbi))
 		return sbi->s_ndevs;
 	return 1;
@@ -2873,6 +2920,13 @@ static int sanity_check_raw_super(struct f2fs_sb_info *sbi,
 		return -EFSCORRUPTED;
 	}
 
+	/* Check if FS has metadata encryption if kernel doesn't support it */
+#ifndef CONFIG_FS_ENCRYPTION_METADATA
+	if (raw_super->metadata_crypt_alg) {
+		f2fs_err(sbi, "Filesystem has metadata encryption but kernel support for it wasn't enabled");
+		return -EINVAL;
+	}
+#endif
 	/* check CP/SIT/NAT/SSA/MAIN_AREA area boundary */
 	if (sanity_check_area_boundary(sbi, bh))
 		return -EFSCORRUPTED;
@@ -3464,6 +3518,9 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_sb_buf;
 	}
 
+#ifdef CONFIG_FS_ENCRYPTION
+	sb->s_cop = &f2fs_cryptops;
+#endif
 	err = parse_options(sb, options, false);
 	if (err)
 		goto free_options;
@@ -3491,9 +3548,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 #endif
 
 	sb->s_op = &f2fs_sops;
-#ifdef CONFIG_FS_ENCRYPTION
-	sb->s_cop = &f2fs_cryptops;
-#endif
 #ifdef CONFIG_FS_VERITY
 	sb->s_vop = &f2fs_verityops;
 #endif
@@ -3602,6 +3656,12 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_devices;
 	}
 
+	err = fscrypt_metadata_crypt_prepare_all_devices(sb);
+	if (err) {
+		f2fs_err(sbi, "Failed to initialize metadata crypt on all devices");
+		goto free_devices;
+	}
+
 	err = f2fs_init_post_read_wq(sbi);
 	if (err) {
 		f2fs_err(sbi, "Failed to initialize post read workqueue");
@@ -3864,6 +3924,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	utf8_unload(sbi->s_encoding);
 #endif
 free_options:
+	fscrypt_free_metadata_encryption(sb);
 #ifdef CONFIG_QUOTA
 	for (i = 0; i < MAXQUOTAS; i++)
 		kfree(F2FS_OPTION(sbi).s_qf_names[i]);
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index 3c383ddd92dd..34cf0031dc8a 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -118,7 +118,8 @@ struct f2fs_super_block {
 	__u8 hot_ext_count;		/* # of hot file extension */
 	__le16  s_encoding;		/* Filename charset encoding */
 	__le16  s_encoding_flags;	/* Filename charset encoding flags */
-	__u8 reserved[306];		/* valid reserved region */
+	__le32	metadata_crypt_alg;	/* The metadata encryption algorithm (FSCRYPT_MODE_*) */
+	__u8 reserved[302];		/* valid reserved region */
 	__le32 crc;			/* checksum of superblock */
 } __packed;
 
-- 
2.28.0.806.g8561365e88-goog


^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [f2fs-dev] [PATCH 3/3] f2fs: Add metadata encryption support
@ 2020-10-05  7:36   ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-05  7:36 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-fscrypt, Satya Tangirala, linux-kernel, linux-f2fs-devel

Wire up metadata encryption support with the fscrypt metadata crypt
additions.

Introduces a new mount option for metadata encryption -
metadata_crypt_key=%s. The argument to this option is the key descriptor of
the metadata encryption key in hex. This key descriptor will be looked up
in the logon keyring with the "fscrypt:" prefix.

E.g. one might pass "-o metadata_crypt_key=ababcdcdefef0101" as the f2fs
mount option to the kernel, when the logon keyring has a key with the
descriptor "fscrypt:ababcdcdefef0101".

Right now, the superblock of the filesystem is itself not encrypted. F2FS
reads the superblock using sb_bread, which uses the bd_inode of the block
device as the address space for any data it reads from the block device -
the data read under the bd_inode address space must be what is physically
present on disk (i.e. if the superblock is encrypted, then the ciphertext
of the superblock must be present in the page cache in the bd_inode's
address space), but f2fs requires that the superblock is decrypted by
blk-crypto, which would put the decrypted page contents into the page cache
instead. We could make f2fs read the superblock by submitting bios directly
with a separate address space, but we choose to just not encrypt the
superblock for now.

Not encrypting the superblock allows us to store the encryption algorithm
used for metadata encryption within the superblock itself, which simplifies
a few things. The userspace tools will store the encryption algorithm in
the superblock when formatting the FS.

Direct I/O with metadata encryption is also not supported for now.
Attempts to do direct I/O on a metadata encrypted F2FS filesystem will fall
back to using buffered I/O (just as attempts to do direct I/O on fscrypt
encrypted files also fall back to buffered I/O).

Signed-off-by: Satya Tangirala <satyat@google.com>
---
 Documentation/filesystems/f2fs.rst | 12 ++++++
 fs/f2fs/data.c                     | 24 +++++++----
 fs/f2fs/f2fs.h                     |  2 +
 fs/f2fs/super.c                    | 67 ++++++++++++++++++++++++++++--
 include/linux/f2fs_fs.h            |  3 +-
 5 files changed, 95 insertions(+), 13 deletions(-)

diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst
index ec8d99703ecb..94a294874707 100644
--- a/Documentation/filesystems/f2fs.rst
+++ b/Documentation/filesystems/f2fs.rst
@@ -266,6 +266,18 @@ inlinecrypt		 When possible, encrypt/decrypt the contents of encrypted
 			 inline encryption hardware. The on-disk format is
 			 unaffected. For more details, see
 			 Documentation/block/inline-encryption.rst.
+metadata_crypt_key=%s	 Specify the metadata encryption key for the filesystem.
+			 The argument to this option is the key descriptor of
+			 the metadata encryption key in hex. This key descriptor
+			 will be looked up in the logon keyring with the
+			 "fscrypt:" prefix. So e.g. one might pass "-o
+			 metadata_crypt_key=ababcdcdefef0101" as the f2fs mount
+			 option to the kernel, when the logon keyring has a key
+			 with the descriptor "fscrypt:ababcdcdefef0101".
+			 When an F2FS filesystem has metadata encryption enabled,
+			 all blocks in the FS other than the superblock are
+			 encrypted with the metadata encryption key. The
+			 superblock itself is stored in plaintext.
 ======================== ============================================================
 
 Debugfs Entries
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 73683e58a08d..1b65313b57c8 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -460,8 +460,8 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages)
 	return bio;
 }
 
-static void f2fs_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
-				  pgoff_t first_idx,
+static void f2fs_set_bio_crypt_ctx(struct bio *bio, block_t blk_addr,
+				  const struct inode *inode, pgoff_t first_idx,
 				  const struct f2fs_io_info *fio,
 				  gfp_t gfp_mask)
 {
@@ -469,8 +469,13 @@ static void f2fs_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
 	 * The f2fs garbage collector sets ->encrypted_page when it wants to
 	 * read/write raw data without encryption.
 	 */
-	if (!fio || !fio->encrypted_page)
-		fscrypt_set_bio_crypt_ctx(bio, inode, first_idx, gfp_mask);
+	if (!fio || !fio->encrypted_page) {
+		if (fscrypt_needs_contents_encryption(inode))
+			fscrypt_set_bio_crypt_ctx(bio, inode, first_idx, gfp_mask);
+		else
+			fscrypt_metadata_crypt_bio(bio, blk_addr, inode->i_sb,
+						   gfp_mask);
+	}
 }
 
 static bool f2fs_crypt_mergeable_bio(struct bio *bio, const struct inode *inode,
@@ -712,7 +717,7 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
 	/* Allocate a new bio */
 	bio = __bio_alloc(fio, 1);
 
-	f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
+	f2fs_set_bio_crypt_ctx(bio, fio->new_blkaddr, fio->page->mapping->host,
 			       fio->page->index, fio, GFP_NOIO);
 
 	if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
@@ -918,7 +923,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
 	if (!bio) {
 		bio = __bio_alloc(fio, BIO_MAX_PAGES);
 		__attach_io_flag(fio);
-		f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
+		f2fs_set_bio_crypt_ctx(bio, fio->new_blkaddr,
+				       fio->page->mapping->host,
 				       fio->page->index, fio, GFP_NOIO);
 		bio_set_op_attrs(bio, fio->op, fio->op_flags);
 
@@ -992,7 +998,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
 			goto skip;
 		}
 		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
-		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
+		f2fs_set_bio_crypt_ctx(io->bio, fio->new_blkaddr,
+				       fio->page->mapping->host,
 				       bio_page->index, fio, GFP_NOIO);
 		io->fio = *fio;
 	}
@@ -1039,9 +1046,8 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
 	if (!bio)
 		return ERR_PTR(-ENOMEM);
 
-	f2fs_set_bio_crypt_ctx(bio, inode, first_idx, NULL, GFP_NOFS);
-
 	f2fs_target_device(sbi, blkaddr, bio);
+	f2fs_set_bio_crypt_ctx(bio, blkaddr, inode, first_idx, NULL, GFP_NOFS);
 	bio->bi_end_io = f2fs_read_end_io;
 	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
 
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d9e52a7f3702..8c5626a6f684 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4095,6 +4095,8 @@ static inline bool f2fs_force_buffered_io(struct inode *inode,
 
 	if (f2fs_post_read_required(inode))
 		return true;
+	if (fscrypt_metadata_crypted(sbi->sb))
+		return true;
 	if (f2fs_is_multi_device(sbi))
 		return true;
 	/*
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 9a6d375cbe4b..1c14c823a4e9 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -146,6 +146,7 @@ enum {
 	Opt_compress_algorithm,
 	Opt_compress_log_size,
 	Opt_compress_extension,
+	Opt_metadata_crypt_key,
 	Opt_err,
 };
 
@@ -213,6 +214,7 @@ static match_table_t f2fs_tokens = {
 	{Opt_compress_algorithm, "compress_algorithm=%s"},
 	{Opt_compress_log_size, "compress_log_size=%u"},
 	{Opt_compress_extension, "compress_extension=%s"},
+	{Opt_metadata_crypt_key, "metadata_crypt_key=%s"},
 	{Opt_err, NULL},
 };
 
@@ -465,6 +467,10 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 #ifdef CONFIG_F2FS_FS_COMPRESSION
 	unsigned char (*ext)[F2FS_EXTENSION_LEN];
 	int ext_cnt;
+#endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+	char *key_desc_hex = NULL;
+	int metadata_crypt_alg = le32_to_cpu(sbi->raw_super->metadata_crypt_alg);
 #endif
 	char *p, *name;
 	int arg = 0;
@@ -937,6 +943,35 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 		case Opt_compress_extension:
 			f2fs_info(sbi, "compression options not supported");
 			break;
+#endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+		case Opt_metadata_crypt_key:
+			if (!metadata_crypt_alg) {
+				f2fs_err(sbi, "Filesystem doesn't have metadata encryption enabled, but a metadata encryption key was provided");
+				return -EINVAL;
+			}
+			if (is_remount) {
+				f2fs_warn(sbi, "Ignoring metadata crypt key specified for remount");
+				break;
+			}
+
+			if (fscrypt_metadata_crypted(sb)) {
+				f2fs_err(sbi, "Multiple metadata crypt key options specified");
+				return -EINVAL;
+			}
+
+			key_desc_hex = match_strdup(&args[0]);
+			if (!key_desc_hex)
+				return -ENOMEM;
+
+			if (fscrypt_setup_metadata_encryption(sb, key_desc_hex,
+							metadata_crypt_alg)) {
+				f2fs_err(sbi, "Could not setup metadata encryption");
+				kfree(key_desc_hex);
+				return -EINVAL;
+			}
+			kfree(key_desc_hex);
+			break;
 #endif
 		default:
 			f2fs_err(sbi, "Unrecognized mount option \"%s\" or missing value",
@@ -964,6 +999,13 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 		return -EINVAL;
 	}
 #endif
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+	if (metadata_crypt_alg &&
+	    !fscrypt_metadata_crypted(sb)) {
+		f2fs_err(sbi, "Filesystem has metadata encryption. Please provide metadata encryption key to mount filesystem");
+		return -EINVAL;
+	}
+#endif
 
 	if (F2FS_IO_SIZE_BITS(sbi) && !f2fs_lfs_mode(sbi)) {
 		f2fs_err(sbi, "Should set mode=lfs with %uKB-sized IO",
@@ -1249,6 +1291,8 @@ static void f2fs_put_super(struct super_block *sb)
 	iput(sbi->meta_inode);
 	sbi->meta_inode = NULL;
 
+	fscrypt_free_metadata_encryption(sb);
+
 	/*
 	 * iput() can update stat information, if f2fs_write_checkpoint()
 	 * above failed with error.
@@ -2504,6 +2548,9 @@ static int f2fs_get_num_devices(struct super_block *sb)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 
+	if (!sbi)
+		return 0;
+
 	if (f2fs_is_multi_device(sbi))
 		return sbi->s_ndevs;
 	return 1;
@@ -2873,6 +2920,13 @@ static int sanity_check_raw_super(struct f2fs_sb_info *sbi,
 		return -EFSCORRUPTED;
 	}
 
+	/* Check if FS has metadata encryption if kernel doesn't support it */
+#ifndef CONFIG_FS_ENCRYPTION_METADATA
+	if (raw_super->metadata_crypt_alg) {
+		f2fs_err(sbi, "Filesystem has metadata encryption but kernel support for it wasn't enabled");
+		return -EINVAL;
+	}
+#endif
 	/* check CP/SIT/NAT/SSA/MAIN_AREA area boundary */
 	if (sanity_check_area_boundary(sbi, bh))
 		return -EFSCORRUPTED;
@@ -3464,6 +3518,9 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_sb_buf;
 	}
 
+#ifdef CONFIG_FS_ENCRYPTION
+	sb->s_cop = &f2fs_cryptops;
+#endif
 	err = parse_options(sb, options, false);
 	if (err)
 		goto free_options;
@@ -3491,9 +3548,6 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 #endif
 
 	sb->s_op = &f2fs_sops;
-#ifdef CONFIG_FS_ENCRYPTION
-	sb->s_cop = &f2fs_cryptops;
-#endif
 #ifdef CONFIG_FS_VERITY
 	sb->s_vop = &f2fs_verityops;
 #endif
@@ -3602,6 +3656,12 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_devices;
 	}
 
+	err = fscrypt_metadata_crypt_prepare_all_devices(sb);
+	if (err) {
+		f2fs_err(sbi, "Failed to initialize metadata crypt on all devices");
+		goto free_devices;
+	}
+
 	err = f2fs_init_post_read_wq(sbi);
 	if (err) {
 		f2fs_err(sbi, "Failed to initialize post read workqueue");
@@ -3864,6 +3924,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	utf8_unload(sbi->s_encoding);
 #endif
 free_options:
+	fscrypt_free_metadata_encryption(sb);
 #ifdef CONFIG_QUOTA
 	for (i = 0; i < MAXQUOTAS; i++)
 		kfree(F2FS_OPTION(sbi).s_qf_names[i]);
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index 3c383ddd92dd..34cf0031dc8a 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -118,7 +118,8 @@ struct f2fs_super_block {
 	__u8 hot_ext_count;		/* # of hot file extension */
 	__le16  s_encoding;		/* Filename charset encoding */
 	__le16  s_encoding_flags;	/* Filename charset encoding flags */
-	__u8 reserved[306];		/* valid reserved region */
+	__le32	metadata_crypt_alg;	/* The metadata encryption algorithm (FSCRYPT_MODE_*) */
+	__u8 reserved[302];		/* valid reserved region */
 	__le32 crc;			/* checksum of superblock */
 } __packed;
 
-- 
2.28.0.806.g8561365e88-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-10-05  7:36 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-05  7:43   ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-05  7:43 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel

On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote:
> This patch series adds support for metadata encryption to F2FS using
> blk-crypto.
> 
> Patch 1 replaces fscrypt_get_devices (which took an array of request_queues
> and filled it up) with fscrypt_get_device, which takes a index of the
> desired device and returns the device at that index (so the index passed
> to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1)
> inclusive). This allows callers to avoid having to allocate an array to
> pass to fscrypt_get_devices() when they only need to iterate through
> each element in the array (and have no use for the array itself).
> 
> Patch 2 introduces some functions to fscrypt that help filesystems perform
> metadata encryption. Any filesystem that wants to use metadata encryption
> can call fscrypt_setup_metadata_encryption() with the super_block of the
> filesystem, the encryption algorithm and the descriptor of the encryption
> key. The descriptor is looked up in the logon keyring of the current
> session with "fscrypt:" as the prefix of the descriptor.
> 
> The patch also introduces fscrypt_metadata_crypt_bio() which an FS should
> call on a bio that the FS wants metadata crypted. The function will add
> an encryption context with the metadata encryption key set up by the call
> to the above mentioned fscrypt_setup_metadata_encryption().
> 
> The patch also introduces fscrypt_metadata_crypt_prepare_all_devices().
> Filesystems that use multiple devices should call this function once all
> the underlying devices have been determined. An FS might only be able to
> determine all the underlying devices after some initial processing that
> might already require metadata en/decryption, which is why this function
> is separate from fscrypt_setup_metadata_encryption().
> 
> Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
> will encrypt every block (that's not being encrypted by some other
> encryption key, e.g. a per-file key) with the metadata encryption key
> except the superblock (and the redundant copy of the superblock). The DUN
> of a block is the offset of the block from the start of the F2FS
> filesystem.
> 
> Please refer to the commit message for why the superblock was excluded from
> en/decryption, and other limitations. The superblock and its copy are
> stored in plaintext on disk. The encryption algorithm used for metadata
> encryption is stored within the superblock itself. Changes to the userspace
> tools (that are required to test out metadata encryption with F2FS) are
> also being sent out - I'll post a link as a reply to this mail once it's
> out.
The userspace patches are at

https://lore.kernel.org/linux-fscrypt/20201005074133.1958633-2-satyat@google.com/

> 
> Satya Tangirala (3):
>   fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
>   fscrypt: Add metadata encryption support
>   f2fs: Add metadata encryption support
> 
>  Documentation/filesystems/f2fs.rst |  12 ++
>  fs/crypto/Kconfig                  |   6 +
>  fs/crypto/Makefile                 |   1 +
>  fs/crypto/fscrypt_private.h        |  19 +++
>  fs/crypto/inline_crypt.c           |  37 +----
>  fs/crypto/metadata_crypt.c         | 220 +++++++++++++++++++++++++++++
>  fs/f2fs/data.c                     |  24 ++--
>  fs/f2fs/f2fs.h                     |   2 +
>  fs/f2fs/super.c                    |  83 +++++++++--
>  include/linux/f2fs_fs.h            |   3 +-
>  include/linux/fs.h                 |   3 +
>  include/linux/fscrypt.h            |  51 ++++++-
>  12 files changed, 410 insertions(+), 51 deletions(-)
>  create mode 100644 fs/crypto/metadata_crypt.c
> 
> -- 
> 2.28.0.806.g8561365e88-goog
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-10-05  7:43   ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-05  7:43 UTC (permalink / raw)
  To: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu
  Cc: linux-fscrypt, linux-kernel, linux-f2fs-devel

On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote:
> This patch series adds support for metadata encryption to F2FS using
> blk-crypto.
> 
> Patch 1 replaces fscrypt_get_devices (which took an array of request_queues
> and filled it up) with fscrypt_get_device, which takes a index of the
> desired device and returns the device at that index (so the index passed
> to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1)
> inclusive). This allows callers to avoid having to allocate an array to
> pass to fscrypt_get_devices() when they only need to iterate through
> each element in the array (and have no use for the array itself).
> 
> Patch 2 introduces some functions to fscrypt that help filesystems perform
> metadata encryption. Any filesystem that wants to use metadata encryption
> can call fscrypt_setup_metadata_encryption() with the super_block of the
> filesystem, the encryption algorithm and the descriptor of the encryption
> key. The descriptor is looked up in the logon keyring of the current
> session with "fscrypt:" as the prefix of the descriptor.
> 
> The patch also introduces fscrypt_metadata_crypt_bio() which an FS should
> call on a bio that the FS wants metadata crypted. The function will add
> an encryption context with the metadata encryption key set up by the call
> to the above mentioned fscrypt_setup_metadata_encryption().
> 
> The patch also introduces fscrypt_metadata_crypt_prepare_all_devices().
> Filesystems that use multiple devices should call this function once all
> the underlying devices have been determined. An FS might only be able to
> determine all the underlying devices after some initial processing that
> might already require metadata en/decryption, which is why this function
> is separate from fscrypt_setup_metadata_encryption().
> 
> Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
> will encrypt every block (that's not being encrypted by some other
> encryption key, e.g. a per-file key) with the metadata encryption key
> except the superblock (and the redundant copy of the superblock). The DUN
> of a block is the offset of the block from the start of the F2FS
> filesystem.
> 
> Please refer to the commit message for why the superblock was excluded from
> en/decryption, and other limitations. The superblock and its copy are
> stored in plaintext on disk. The encryption algorithm used for metadata
> encryption is stored within the superblock itself. Changes to the userspace
> tools (that are required to test out metadata encryption with F2FS) are
> also being sent out - I'll post a link as a reply to this mail once it's
> out.
The userspace patches are at

https://lore.kernel.org/linux-fscrypt/20201005074133.1958633-2-satyat@google.com/

> 
> Satya Tangirala (3):
>   fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
>   fscrypt: Add metadata encryption support
>   f2fs: Add metadata encryption support
> 
>  Documentation/filesystems/f2fs.rst |  12 ++
>  fs/crypto/Kconfig                  |   6 +
>  fs/crypto/Makefile                 |   1 +
>  fs/crypto/fscrypt_private.h        |  19 +++
>  fs/crypto/inline_crypt.c           |  37 +----
>  fs/crypto/metadata_crypt.c         | 220 +++++++++++++++++++++++++++++
>  fs/f2fs/data.c                     |  24 ++--
>  fs/f2fs/f2fs.h                     |   2 +
>  fs/f2fs/super.c                    |  83 +++++++++--
>  include/linux/f2fs_fs.h            |   3 +-
>  include/linux/fs.h                 |   3 +
>  include/linux/fscrypt.h            |  51 ++++++-
>  12 files changed, 410 insertions(+), 51 deletions(-)
>  create mode 100644 fs/crypto/metadata_crypt.c
> 
> -- 
> 2.28.0.806.g8561365e88-goog
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 3/3] f2fs: Add metadata encryption support
  2020-10-05  7:36   ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-05 10:19     ` kernel test robot
  -1 siblings, 0 replies; 42+ messages in thread
From: kernel test robot @ 2020-10-05 10:19 UTC (permalink / raw)
  To: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim,
	Eric Biggers, Chao Yu, Chao Yu
  Cc: kbuild-all, linux-kernel, linux-fscrypt, linux-f2fs-devel,
	Satya Tangirala

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

Hi Satya,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.9-rc8]
[cannot apply to f2fs/dev-test linux/master next-20201002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Satya-Tangirala/add-support-for-metadata-encryption-to-F2FS/20201005-153825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 549738f15da0e5a00275977623be199fbbf7df50
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/0dcac73b9c53e2f16c8c8ba3a9fc84084ddae25d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Satya-Tangirala/add-support-for-metadata-encryption-to-F2FS/20201005-153825
        git checkout 0dcac73b9c53e2f16c8c8ba3a9fc84084ddae25d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "__delay" [drivers/net/phy/mdio-cavium.ko] undefined!
>> ERROR: modpost: "fscrypt_setup_metadata_encryption" [fs/f2fs/f2fs.ko] undefined!
>> ERROR: modpost: "fscrypt_free_metadata_encryption" [fs/f2fs/f2fs.ko] undefined!
>> ERROR: modpost: "fscrypt_metadata_crypt_prepare_all_devices" [fs/f2fs/f2fs.ko] undefined!
>> ERROR: modpost: "fscrypt_metadata_crypt_bio" [fs/f2fs/f2fs.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52722 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 3/3] f2fs: Add metadata encryption support
@ 2020-10-05 10:19     ` kernel test robot
  0 siblings, 0 replies; 42+ messages in thread
From: kernel test robot @ 2020-10-05 10:19 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

Hi Satya,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.9-rc8]
[cannot apply to f2fs/dev-test linux/master next-20201002]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Satya-Tangirala/add-support-for-metadata-encryption-to-F2FS/20201005-153825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 549738f15da0e5a00275977623be199fbbf7df50
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/0dcac73b9c53e2f16c8c8ba3a9fc84084ddae25d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Satya-Tangirala/add-support-for-metadata-encryption-to-F2FS/20201005-153825
        git checkout 0dcac73b9c53e2f16c8c8ba3a9fc84084ddae25d
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

ERROR: modpost: "__delay" [drivers/net/phy/mdio-cavium.ko] undefined!
>> ERROR: modpost: "fscrypt_setup_metadata_encryption" [fs/f2fs/f2fs.ko] undefined!
>> ERROR: modpost: "fscrypt_free_metadata_encryption" [fs/f2fs/f2fs.ko] undefined!
>> ERROR: modpost: "fscrypt_metadata_crypt_prepare_all_devices" [fs/f2fs/f2fs.ko] undefined!
>> ERROR: modpost: "fscrypt_metadata_crypt_bio" [fs/f2fs/f2fs.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 52722 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/3] fscrypt: Add metadata encryption support
  2020-10-05  7:36   ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-07 20:52     ` Eric Biggers
  -1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-07 20:52 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Mon, Oct 05, 2020 at 07:36:05AM +0000, Satya Tangirala wrote:
> Introduces functions that help with metadata encryption.
> 
> In particular, we introduce:
> 
> fscrypt_setup_metadata_encryption() - filesystems should call this function
> to set up metadata encryption on a super block with the encryption
> algorithm (the desired FSCRYPT_MODE_*) and the key descriptor of the
> encryption key. The key descriptor is looked up in the logon keyring of the
> current session using "fscrypt:" as the descriptor prefix.
> 
> fscrypt_metadata_crypt_bio() - filesystems should call this function on a
> bio that it wants metadata crypted. This function will set a bio-crypt-ctx
> on the bio if the metadata key was set up with
> fscrypt_setup_metadata_encryption(). The DUN for the first block in the bio
> is the offset of that block from the start of the filesystem.
> 
> fscrypt_free_metadata_encryption() - this function should be called when
> the super block is being freed. It ensures that the metadata encryption key
> is evicted, if necessary, from devices.
> 
> Note that the filesystem (rather than fscrypt) controls precisely which
> blocks are encrypted with the metadata encryption key and which blocks are
> encrypted with other keys/not encrypted at all. Fscrypt only provides some
> convenience functions that ultimately help encrypt a bio with the metadata
> encryption key when desired.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  fs/crypto/Kconfig           |   6 +
>  fs/crypto/Makefile          |   1 +
>  fs/crypto/fscrypt_private.h |  19 ++++
>  fs/crypto/inline_crypt.c    |  18 ---
>  fs/crypto/metadata_crypt.c  | 220 ++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h          |   3 +
>  include/linux/fscrypt.h     |  47 ++++++++
>  7 files changed, 296 insertions(+), 18 deletions(-)
>  create mode 100644 fs/crypto/metadata_crypt.c
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index a5f5c30368a2..3010e91f6659 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -30,3 +30,9 @@ config FS_ENCRYPTION_INLINE_CRYPT
>  	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
>  	help
>  	  Enable fscrypt to use inline encryption hardware if available.
> +
> +config FS_ENCRYPTION_METADATA
> +	bool "Enable metadata encryption with fscrypt"
> +	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
> +	help
> +	  Enable fscrypt to encrypt metadata.

This needs Kconfig help text to describe what this feature is and why anyone
would want to enable it.  It also needs an update to
Documentation/filesystems/fscrypt.rst, and a test in xfstests that tests that
the encryption is being done correctly.

> diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
> new file mode 100644
> index 000000000000..5e16df130509
> --- /dev/null
> +++ b/fs/crypto/metadata_crypt.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Metadata encryption support for fscrypt
> + *
> + * Copyright 2020 Google LLC
> + */
> +
> +#include <keys/user-type.h>
> +#include <linux/blk-crypto.h>
> +#include <linux/blkdev.h>
> +#include <linux/buffer_head.h>
> +#include <linux/sched/mm.h>
> +
> +#include "fscrypt_private.h"
> +
> +/* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
> +static int fscrypt_metadata_get_key_from_id(const char *prefix,
> +					    char *descriptor_hex,
> +					    unsigned int min_keysize,
> +					    char *raw_key)
> +{
> +	char *description;
> +	struct key *key;
> +	const struct user_key_payload *ukp;
> +	const struct fscrypt_key *payload;
> +	int err = -ENOKEY;
> +
> +	if (strlen(descriptor_hex) != FSCRYPT_KEY_DESCRIPTOR_SIZE * 2)
> +		return -EINVAL;
> +
> +	description = kasprintf(GFP_NOFS, "%s%s", prefix, descriptor_hex);
> +	if (!description)
> +		return -ENOMEM;
> +
> +	key = request_key(&key_type_logon, description, NULL);
> +	kfree(description);
> +	if (IS_ERR(key))
> +		return PTR_ERR(key);
> +
> +	down_read(&key->sem);
> +	ukp = user_key_payload_locked(key);
> +
> +	if (!ukp) /* was the key revoked before we acquired its semaphore? */
> +		goto out;
> +
> +	payload = (const struct fscrypt_key *)ukp->data;

'struct fscrypt_key' was a mistake.  How about having the key payload just be
the raw key?

Or are you thinking that reserved fields will be needed?

> +/**
> + * fscrypt_setup_metadata_encryption() - prepare a super_block for metadata
> + *					 encryption
> + * @sb: The super_block to set up metadata encryption for
> + * @key_desc_hex: The key descriptor (in hex) to look for in the logon keyring.

There's no such thing as a "logon keyring".  I think you mean "look for a logon
key in the process-subscribed keyrings".

> + * @fscrypt_mode_num: The FSCRYPT_MODE_* to use as the encryption algorithm.
> + *
> + * Return: 0 on success, negative number on error.
> + */
> +int fscrypt_setup_metadata_encryption(struct super_block *sb,
> +				      char *key_desc_hex,
> +				      int fscrypt_mode_num)
> +{
> +	int err = 0;
> +	enum blk_crypto_mode_num crypto_mode;
> +	unsigned int lblk_bits = 64;
> +	unsigned int dun_bytes;
> +	unsigned int dummy;
> +	char raw_key[FSCRYPT_MAX_KEY_SIZE];

For binary data, prefer 'u8' to 'char'.

> +
> +	if (fscrypt_mode_num > __FSCRYPT_MODE_MAX || fscrypt_mode_num < 0 ||
> +	    !fscrypt_modes[fscrypt_mode_num].cipher_str) {
> +		fscrypt_warn(NULL, "Invalid fscrypt mode %d specified for metadata encryption.",
> +			     fscrypt_mode_num);
> +		return -EOPNOTSUPP;
> +	}

The filenames-only encryption modes (FSCRYPT_MODE_AES_256_CTS and
FSCRYPT_MODE_AES_128_CTS) will pass this check, which seems undesired.

> +
> +	if (sb->s_cop->get_ino_and_lblk_bits)
> +		sb->s_cop->get_ino_and_lblk_bits(sb, &dummy, &lblk_bits);
> +	dun_bytes = DIV_ROUND_UP(lblk_bits, 8);
> +
> +	if (fscrypt_modes[fscrypt_mode_num].ivsize < dun_bytes) {
> +		fscrypt_warn(NULL, "The fscrypt mode only supports %d DUN bytes, but FS requires support for %d DUN bytes.",
> +			     fscrypt_modes[fscrypt_mode_num].ivsize, dun_bytes);
> +		return -EOPNOTSUPP;
> +	}

lblk_bits is the number of bits used to represent file logical block numbers
(e.g. ext4_lblk_t).  That's different from the filesystem-wide block number
(e.g. ext4_fsblk_t), which is what metadata encryption will use.

> +	crypto_mode = fscrypt_modes[fscrypt_mode_num].blk_crypto_mode;
> +
> +	err = fscrypt_metadata_get_key_from_id(
> +					FSCRYPT_KEY_DESC_PREFIX,
> +					key_desc_hex,
> +					fscrypt_modes[fscrypt_mode_num].keysize,
> +					raw_key);
> +	if (err)
> +		goto out;

This is allowing for the key to be longer than the provided keysize, in which
case only a prefix of the key is used.

It should require the exact keysize instead.

> +
> +	sb->s_metadata_key = kzalloc(sizeof(*sb->s_metadata_key), GFP_NOFS);

No need for GFP_NOFS here.

> +/**
> + * fscrypt_free_metadata_encryption() - free metadata encryption fields in
> + *					super_block.
> + * @sb: The super_block to free metatdata encryption fields from
> + */
> +void fscrypt_free_metadata_encryption(struct super_block *sb)
> +{
> +	int num_devices;
> +	int i;
> +	struct request_queue *q;
> +
> +	if (!sb->s_metadata_key)
> +		return;
> +
> +	num_devices = fscrypt_get_num_devices(sb);
> +
> +	for (i = 0; i < num_devices; i++) {
> +		q = fscrypt_get_device(sb, i);
> +		if (WARN_ON(!q))
> +			continue;
> +		blk_crypto_evict_key(q, sb->s_metadata_key);
> +	}
> +
> +	memzero_explicit(sb->s_metadata_key, sizeof(*sb->s_metadata_key));
> +	kzfree(sb->s_metadata_key);
> +	sb->s_metadata_key = NULL;
> +}

kfree_sensitive(), not kzfree().

Also, memzero_explicit() is redundant.

> +/**
> + * fscrypt_metadata_crypt_bio() - Add metadata encryption context to bio.
> + *
> + * @bio: The bio to add the encryption context to
> + * @lblk: The logical block number within the filesystem at which this bio
> + *	  starts reading/writing data.

Should be:

   @fsblk: The block number within the filesystem ...

> + * @sb: The superblock of the filesystem
> + * @gfp_mask: gfp_mask for bio_crypt_context allocation
> + */
> +void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
> +				struct super_block *sb, gfp_t gfp_mask)
> +{
> +	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { 0 };
> +
> +	if (!sb->s_metadata_key)
> +		return;
> +
> +	dun[0] = lblk;
> +	bio_crypt_set_ctx(bio, sb->s_metadata_key, dun, gfp_mask);
> +}

Perhaps fscrypt_set_bio_crypt_ctx() should call this?  It seems there should be
a single function that filesystems can call that handles setting the
bio_crypt_ctx for both file contents and metadata encryption.

- Eric

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 2/3] fscrypt: Add metadata encryption support
@ 2020-10-07 20:52     ` Eric Biggers
  0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-07 20:52 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Mon, Oct 05, 2020 at 07:36:05AM +0000, Satya Tangirala wrote:
> Introduces functions that help with metadata encryption.
> 
> In particular, we introduce:
> 
> fscrypt_setup_metadata_encryption() - filesystems should call this function
> to set up metadata encryption on a super block with the encryption
> algorithm (the desired FSCRYPT_MODE_*) and the key descriptor of the
> encryption key. The key descriptor is looked up in the logon keyring of the
> current session using "fscrypt:" as the descriptor prefix.
> 
> fscrypt_metadata_crypt_bio() - filesystems should call this function on a
> bio that it wants metadata crypted. This function will set a bio-crypt-ctx
> on the bio if the metadata key was set up with
> fscrypt_setup_metadata_encryption(). The DUN for the first block in the bio
> is the offset of that block from the start of the filesystem.
> 
> fscrypt_free_metadata_encryption() - this function should be called when
> the super block is being freed. It ensures that the metadata encryption key
> is evicted, if necessary, from devices.
> 
> Note that the filesystem (rather than fscrypt) controls precisely which
> blocks are encrypted with the metadata encryption key and which blocks are
> encrypted with other keys/not encrypted at all. Fscrypt only provides some
> convenience functions that ultimately help encrypt a bio with the metadata
> encryption key when desired.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  fs/crypto/Kconfig           |   6 +
>  fs/crypto/Makefile          |   1 +
>  fs/crypto/fscrypt_private.h |  19 ++++
>  fs/crypto/inline_crypt.c    |  18 ---
>  fs/crypto/metadata_crypt.c  | 220 ++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h          |   3 +
>  include/linux/fscrypt.h     |  47 ++++++++
>  7 files changed, 296 insertions(+), 18 deletions(-)
>  create mode 100644 fs/crypto/metadata_crypt.c
> 
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index a5f5c30368a2..3010e91f6659 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -30,3 +30,9 @@ config FS_ENCRYPTION_INLINE_CRYPT
>  	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
>  	help
>  	  Enable fscrypt to use inline encryption hardware if available.
> +
> +config FS_ENCRYPTION_METADATA
> +	bool "Enable metadata encryption with fscrypt"
> +	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
> +	help
> +	  Enable fscrypt to encrypt metadata.

This needs Kconfig help text to describe what this feature is and why anyone
would want to enable it.  It also needs an update to
Documentation/filesystems/fscrypt.rst, and a test in xfstests that tests that
the encryption is being done correctly.

> diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
> new file mode 100644
> index 000000000000..5e16df130509
> --- /dev/null
> +++ b/fs/crypto/metadata_crypt.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Metadata encryption support for fscrypt
> + *
> + * Copyright 2020 Google LLC
> + */
> +
> +#include <keys/user-type.h>
> +#include <linux/blk-crypto.h>
> +#include <linux/blkdev.h>
> +#include <linux/buffer_head.h>
> +#include <linux/sched/mm.h>
> +
> +#include "fscrypt_private.h"
> +
> +/* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
> +static int fscrypt_metadata_get_key_from_id(const char *prefix,
> +					    char *descriptor_hex,
> +					    unsigned int min_keysize,
> +					    char *raw_key)
> +{
> +	char *description;
> +	struct key *key;
> +	const struct user_key_payload *ukp;
> +	const struct fscrypt_key *payload;
> +	int err = -ENOKEY;
> +
> +	if (strlen(descriptor_hex) != FSCRYPT_KEY_DESCRIPTOR_SIZE * 2)
> +		return -EINVAL;
> +
> +	description = kasprintf(GFP_NOFS, "%s%s", prefix, descriptor_hex);
> +	if (!description)
> +		return -ENOMEM;
> +
> +	key = request_key(&key_type_logon, description, NULL);
> +	kfree(description);
> +	if (IS_ERR(key))
> +		return PTR_ERR(key);
> +
> +	down_read(&key->sem);
> +	ukp = user_key_payload_locked(key);
> +
> +	if (!ukp) /* was the key revoked before we acquired its semaphore? */
> +		goto out;
> +
> +	payload = (const struct fscrypt_key *)ukp->data;

'struct fscrypt_key' was a mistake.  How about having the key payload just be
the raw key?

Or are you thinking that reserved fields will be needed?

> +/**
> + * fscrypt_setup_metadata_encryption() - prepare a super_block for metadata
> + *					 encryption
> + * @sb: The super_block to set up metadata encryption for
> + * @key_desc_hex: The key descriptor (in hex) to look for in the logon keyring.

There's no such thing as a "logon keyring".  I think you mean "look for a logon
key in the process-subscribed keyrings".

> + * @fscrypt_mode_num: The FSCRYPT_MODE_* to use as the encryption algorithm.
> + *
> + * Return: 0 on success, negative number on error.
> + */
> +int fscrypt_setup_metadata_encryption(struct super_block *sb,
> +				      char *key_desc_hex,
> +				      int fscrypt_mode_num)
> +{
> +	int err = 0;
> +	enum blk_crypto_mode_num crypto_mode;
> +	unsigned int lblk_bits = 64;
> +	unsigned int dun_bytes;
> +	unsigned int dummy;
> +	char raw_key[FSCRYPT_MAX_KEY_SIZE];

For binary data, prefer 'u8' to 'char'.

> +
> +	if (fscrypt_mode_num > __FSCRYPT_MODE_MAX || fscrypt_mode_num < 0 ||
> +	    !fscrypt_modes[fscrypt_mode_num].cipher_str) {
> +		fscrypt_warn(NULL, "Invalid fscrypt mode %d specified for metadata encryption.",
> +			     fscrypt_mode_num);
> +		return -EOPNOTSUPP;
> +	}

The filenames-only encryption modes (FSCRYPT_MODE_AES_256_CTS and
FSCRYPT_MODE_AES_128_CTS) will pass this check, which seems undesired.

> +
> +	if (sb->s_cop->get_ino_and_lblk_bits)
> +		sb->s_cop->get_ino_and_lblk_bits(sb, &dummy, &lblk_bits);
> +	dun_bytes = DIV_ROUND_UP(lblk_bits, 8);
> +
> +	if (fscrypt_modes[fscrypt_mode_num].ivsize < dun_bytes) {
> +		fscrypt_warn(NULL, "The fscrypt mode only supports %d DUN bytes, but FS requires support for %d DUN bytes.",
> +			     fscrypt_modes[fscrypt_mode_num].ivsize, dun_bytes);
> +		return -EOPNOTSUPP;
> +	}

lblk_bits is the number of bits used to represent file logical block numbers
(e.g. ext4_lblk_t).  That's different from the filesystem-wide block number
(e.g. ext4_fsblk_t), which is what metadata encryption will use.

> +	crypto_mode = fscrypt_modes[fscrypt_mode_num].blk_crypto_mode;
> +
> +	err = fscrypt_metadata_get_key_from_id(
> +					FSCRYPT_KEY_DESC_PREFIX,
> +					key_desc_hex,
> +					fscrypt_modes[fscrypt_mode_num].keysize,
> +					raw_key);
> +	if (err)
> +		goto out;

This is allowing for the key to be longer than the provided keysize, in which
case only a prefix of the key is used.

It should require the exact keysize instead.

> +
> +	sb->s_metadata_key = kzalloc(sizeof(*sb->s_metadata_key), GFP_NOFS);

No need for GFP_NOFS here.

> +/**
> + * fscrypt_free_metadata_encryption() - free metadata encryption fields in
> + *					super_block.
> + * @sb: The super_block to free metatdata encryption fields from
> + */
> +void fscrypt_free_metadata_encryption(struct super_block *sb)
> +{
> +	int num_devices;
> +	int i;
> +	struct request_queue *q;
> +
> +	if (!sb->s_metadata_key)
> +		return;
> +
> +	num_devices = fscrypt_get_num_devices(sb);
> +
> +	for (i = 0; i < num_devices; i++) {
> +		q = fscrypt_get_device(sb, i);
> +		if (WARN_ON(!q))
> +			continue;
> +		blk_crypto_evict_key(q, sb->s_metadata_key);
> +	}
> +
> +	memzero_explicit(sb->s_metadata_key, sizeof(*sb->s_metadata_key));
> +	kzfree(sb->s_metadata_key);
> +	sb->s_metadata_key = NULL;
> +}

kfree_sensitive(), not kzfree().

Also, memzero_explicit() is redundant.

> +/**
> + * fscrypt_metadata_crypt_bio() - Add metadata encryption context to bio.
> + *
> + * @bio: The bio to add the encryption context to
> + * @lblk: The logical block number within the filesystem at which this bio
> + *	  starts reading/writing data.

Should be:

   @fsblk: The block number within the filesystem ...

> + * @sb: The superblock of the filesystem
> + * @gfp_mask: gfp_mask for bio_crypt_context allocation
> + */
> +void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
> +				struct super_block *sb, gfp_t gfp_mask)
> +{
> +	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { 0 };
> +
> +	if (!sb->s_metadata_key)
> +		return;
> +
> +	dun[0] = lblk;
> +	bio_crypt_set_ctx(bio, sb->s_metadata_key, dun, gfp_mask);
> +}

Perhaps fscrypt_set_bio_crypt_ctx() should call this?  It seems there should be
a single function that filesystems can call that handles setting the
bio_crypt_ctx for both file contents and metadata encryption.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-10-05  7:36 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-07 21:00   ` Eric Biggers
  -1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-07 21:00 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote:
> This patch series adds support for metadata encryption to F2FS using
> blk-crypto.

This patch series needs more explanation about what "metadata encryption" is,
why people will want to use it (as opposed to either not using it, or using
fscrypt + dm-crypt instead), and why this is the best implementation of it.

> Patch 2 introduces some functions to fscrypt that help filesystems perform
> metadata encryption. Any filesystem that wants to use metadata encryption
> can call fscrypt_setup_metadata_encryption() with the super_block of the
> filesystem, the encryption algorithm and the descriptor of the encryption
> key. The descriptor is looked up in the logon keyring of the current
> session with "fscrypt:" as the prefix of the descriptor.

I notice this is missing the step I suggested to include the metadata encryption
key in the HKDF application-specific info string when deriving subkeys from the
fscrypt master keys.

The same effect could also be achieved by adding an additional level to the key
hierarchy: each HKDF key would be derived from a fscrypt master key and the
metadata encryption key.

We need one of those, to guarantee that the file contents encryption is at least
as strong as the "metadata encryption".

- Eric

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-10-07 21:00   ` Eric Biggers
  0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-07 21:00 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote:
> This patch series adds support for metadata encryption to F2FS using
> blk-crypto.

This patch series needs more explanation about what "metadata encryption" is,
why people will want to use it (as opposed to either not using it, or using
fscrypt + dm-crypt instead), and why this is the best implementation of it.

> Patch 2 introduces some functions to fscrypt that help filesystems perform
> metadata encryption. Any filesystem that wants to use metadata encryption
> can call fscrypt_setup_metadata_encryption() with the super_block of the
> filesystem, the encryption algorithm and the descriptor of the encryption
> key. The descriptor is looked up in the logon keyring of the current
> session with "fscrypt:" as the prefix of the descriptor.

I notice this is missing the step I suggested to include the metadata encryption
key in the HKDF application-specific info string when deriving subkeys from the
fscrypt master keys.

The same effect could also be achieved by adding an additional level to the key
hierarchy: each HKDF key would be derived from a fscrypt master key and the
metadata encryption key.

We need one of those, to guarantee that the file contents encryption is at least
as strong as the "metadata encryption".

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 3/3] f2fs: Add metadata encryption support
  2020-10-05  7:36   ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-07 21:20     ` Eric Biggers
  -1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-07 21:20 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Mon, Oct 05, 2020 at 07:36:06AM +0000, Satya Tangirala wrote:
> Wire up metadata encryption support with the fscrypt metadata crypt
> additions.
> 
> Introduces a new mount option for metadata encryption -
> metadata_crypt_key=%s. The argument to this option is the key descriptor of
> the metadata encryption key in hex. 

It's unclear what "key descriptor in hex" means in this context.  Keys in the
Linux keyrings subsystem can be specified either by an integer ID or by a string
"description".

fscrypt_policy_v1 has an 8-byte binary master_key_descriptor, which specifies a
keyring key with description "fscrypt:" + ToHex(master_key_descriptor).  So I'm
guessing that's where this terminology is coming from.

However, here the value passed to metadata_crypt_key is just a key description
that's passed directly to the Linux keyrings subsystem.  I don't see why it has
to be a hex string (and it fact, it seems it's not enforced?).

The current proposal is also missing any sort of key verification.  The
filesystem will use any key that is provided, even if a different key was used
at format time.

In "fscrypt v2", we solved the equivalent problem by making the keys be
specified by a HKDF-derived master_key_identifier.

How about doing something similar for the metadata encryption key?  I.e. the
metadata encryption key could be used as input to HKDF to derive two subkeys:
metadata_key_identifier and the real metadata encryption key.  Then
metadata_key_identifier could be stored in the superblock.  Then the filesystem
could request the keyring key "fscrypt:" + ToHex(metadata_key_identifier) at
mount time, which would eliminate the need for a mount option.

> Direct I/O with metadata encryption is also not supported for now.
> Attempts to do direct I/O on a metadata encrypted F2FS filesystem will fall
> back to using buffered I/O (just as attempts to do direct I/O on fscrypt
> encrypted files also fall back to buffered I/O).

What would it take to get direct I/O working?

> +#ifdef CONFIG_FS_ENCRYPTION_METADATA
> +	if (metadata_crypt_alg &&
> +	    !fscrypt_metadata_crypted(sb)) {
> +		f2fs_err(sbi, "Filesystem has metadata encryption. Please provide metadata encryption key to mount filesystem");
> +		return -EINVAL;
> +	}
> +#endif

Please try to avoid #ifdefs.  It looks like some of these could be replaced with
IS_ENABLED() or the use of stub functions.

- Eric

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 3/3] f2fs: Add metadata encryption support
@ 2020-10-07 21:20     ` Eric Biggers
  0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-07 21:20 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Mon, Oct 05, 2020 at 07:36:06AM +0000, Satya Tangirala wrote:
> Wire up metadata encryption support with the fscrypt metadata crypt
> additions.
> 
> Introduces a new mount option for metadata encryption -
> metadata_crypt_key=%s. The argument to this option is the key descriptor of
> the metadata encryption key in hex. 

It's unclear what "key descriptor in hex" means in this context.  Keys in the
Linux keyrings subsystem can be specified either by an integer ID or by a string
"description".

fscrypt_policy_v1 has an 8-byte binary master_key_descriptor, which specifies a
keyring key with description "fscrypt:" + ToHex(master_key_descriptor).  So I'm
guessing that's where this terminology is coming from.

However, here the value passed to metadata_crypt_key is just a key description
that's passed directly to the Linux keyrings subsystem.  I don't see why it has
to be a hex string (and it fact, it seems it's not enforced?).

The current proposal is also missing any sort of key verification.  The
filesystem will use any key that is provided, even if a different key was used
at format time.

In "fscrypt v2", we solved the equivalent problem by making the keys be
specified by a HKDF-derived master_key_identifier.

How about doing something similar for the metadata encryption key?  I.e. the
metadata encryption key could be used as input to HKDF to derive two subkeys:
metadata_key_identifier and the real metadata encryption key.  Then
metadata_key_identifier could be stored in the superblock.  Then the filesystem
could request the keyring key "fscrypt:" + ToHex(metadata_key_identifier) at
mount time, which would eliminate the need for a mount option.

> Direct I/O with metadata encryption is also not supported for now.
> Attempts to do direct I/O on a metadata encrypted F2FS filesystem will fall
> back to using buffered I/O (just as attempts to do direct I/O on fscrypt
> encrypted files also fall back to buffered I/O).

What would it take to get direct I/O working?

> +#ifdef CONFIG_FS_ENCRYPTION_METADATA
> +	if (metadata_crypt_alg &&
> +	    !fscrypt_metadata_crypted(sb)) {
> +		f2fs_err(sbi, "Filesystem has metadata encryption. Please provide metadata encryption key to mount filesystem");
> +		return -EINVAL;
> +	}
> +#endif

Please try to avoid #ifdefs.  It looks like some of these could be replaced with
IS_ENABLED() or the use of stub functions.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-10-07 21:00   ` [f2fs-dev] " Eric Biggers
@ 2020-10-07 22:05     ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-07 22:05 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Wed, Oct 07, 2020 at 02:00:40PM -0700, Eric Biggers wrote:
> On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote:
> > This patch series adds support for metadata encryption to F2FS using
> > blk-crypto.
> 
> This patch series needs more explanation about what "metadata encryption" is,
> why people will want to use it (as opposed to either not using it, or using
> fscrypt + dm-crypt instead), and why this is the best implementation of it.
> 
Sure, I'll add that in the next version
> > Patch 2 introduces some functions to fscrypt that help filesystems perform
> > metadata encryption. Any filesystem that wants to use metadata encryption
> > can call fscrypt_setup_metadata_encryption() with the super_block of the
> > filesystem, the encryption algorithm and the descriptor of the encryption
> > key. The descriptor is looked up in the logon keyring of the current
> > session with "fscrypt:" as the prefix of the descriptor.
> 
> I notice this is missing the step I suggested to include the metadata encryption
> key in the HKDF application-specific info string when deriving subkeys from the
> fscrypt master keys.
> 
> The same effect could also be achieved by adding an additional level to the key
> hierarchy: each HKDF key would be derived from a fscrypt master key and the
> metadata encryption key.
> 
> We need one of those, to guarantee that the file contents encryption is at least
> as strong as the "metadata encryption".
>
Yes - I didn't get around to that in the first version, but I'll add
that too in the next version. I was going to go with the first approach
before I saw your comment - is there one method you'd recommend going
with over the other?
> - Eric

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-10-07 22:05     ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-07 22:05 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Wed, Oct 07, 2020 at 02:00:40PM -0700, Eric Biggers wrote:
> On Mon, Oct 05, 2020 at 07:36:03AM +0000, Satya Tangirala wrote:
> > This patch series adds support for metadata encryption to F2FS using
> > blk-crypto.
> 
> This patch series needs more explanation about what "metadata encryption" is,
> why people will want to use it (as opposed to either not using it, or using
> fscrypt + dm-crypt instead), and why this is the best implementation of it.
> 
Sure, I'll add that in the next version
> > Patch 2 introduces some functions to fscrypt that help filesystems perform
> > metadata encryption. Any filesystem that wants to use metadata encryption
> > can call fscrypt_setup_metadata_encryption() with the super_block of the
> > filesystem, the encryption algorithm and the descriptor of the encryption
> > key. The descriptor is looked up in the logon keyring of the current
> > session with "fscrypt:" as the prefix of the descriptor.
> 
> I notice this is missing the step I suggested to include the metadata encryption
> key in the HKDF application-specific info string when deriving subkeys from the
> fscrypt master keys.
> 
> The same effect could also be achieved by adding an additional level to the key
> hierarchy: each HKDF key would be derived from a fscrypt master key and the
> metadata encryption key.
> 
> We need one of those, to guarantee that the file contents encryption is at least
> as strong as the "metadata encryption".
>
Yes - I didn't get around to that in the first version, but I'll add
that too in the next version. I was going to go with the first approach
before I saw your comment - is there one method you'd recommend going
with over the other?
> - Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/3] fscrypt: Add metadata encryption support
  2020-10-07 20:52     ` [f2fs-dev] " Eric Biggers
@ 2020-10-07 23:28       ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-07 23:28 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Wed, Oct 07, 2020 at 01:52:21PM -0700, Eric Biggers wrote:
> On Mon, Oct 05, 2020 at 07:36:05AM +0000, Satya Tangirala wrote:
> > Introduces functions that help with metadata encryption.
> > 
> > In particular, we introduce:
> > 
> > fscrypt_setup_metadata_encryption() - filesystems should call this function
> > to set up metadata encryption on a super block with the encryption
> > algorithm (the desired FSCRYPT_MODE_*) and the key descriptor of the
> > encryption key. The key descriptor is looked up in the logon keyring of the
> > current session using "fscrypt:" as the descriptor prefix.
> > 
> > fscrypt_metadata_crypt_bio() - filesystems should call this function on a
> > bio that it wants metadata crypted. This function will set a bio-crypt-ctx
> > on the bio if the metadata key was set up with
> > fscrypt_setup_metadata_encryption(). The DUN for the first block in the bio
> > is the offset of that block from the start of the filesystem.
> > 
> > fscrypt_free_metadata_encryption() - this function should be called when
> > the super block is being freed. It ensures that the metadata encryption key
> > is evicted, if necessary, from devices.
> > 
> > Note that the filesystem (rather than fscrypt) controls precisely which
> > blocks are encrypted with the metadata encryption key and which blocks are
> > encrypted with other keys/not encrypted at all. Fscrypt only provides some
> > convenience functions that ultimately help encrypt a bio with the metadata
> > encryption key when desired.
> > 
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> > ---
> >  fs/crypto/Kconfig           |   6 +
> >  fs/crypto/Makefile          |   1 +
> >  fs/crypto/fscrypt_private.h |  19 ++++
> >  fs/crypto/inline_crypt.c    |  18 ---
> >  fs/crypto/metadata_crypt.c  | 220 ++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h          |   3 +
> >  include/linux/fscrypt.h     |  47 ++++++++
> >  7 files changed, 296 insertions(+), 18 deletions(-)
> >  create mode 100644 fs/crypto/metadata_crypt.c
> > 
> > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> > index a5f5c30368a2..3010e91f6659 100644
> > --- a/fs/crypto/Kconfig
> > +++ b/fs/crypto/Kconfig
> > @@ -30,3 +30,9 @@ config FS_ENCRYPTION_INLINE_CRYPT
> >  	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
> >  	help
> >  	  Enable fscrypt to use inline encryption hardware if available.
> > +
> > +config FS_ENCRYPTION_METADATA
> > +	bool "Enable metadata encryption with fscrypt"
> > +	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
> > +	help
> > +	  Enable fscrypt to encrypt metadata.
> 
> This needs Kconfig help text to describe what this feature is and why anyone
> would want to enable it.  It also needs an update to
> Documentation/filesystems/fscrypt.rst, and a test in xfstests that tests that
> the encryption is being done correctly.
> 
Sure. I forgot to mention, fwiw I did hack xfstests to enable metadata
encryption on each device to try to test the code, and also some other
informal tests, but as you point out, I should send out actual xfstests
to test this.
> > diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
> > new file mode 100644
> > index 000000000000..5e16df130509
> > --- /dev/null
> > +++ b/fs/crypto/metadata_crypt.c
> > @@ -0,0 +1,220 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Metadata encryption support for fscrypt
> > + *
> > + * Copyright 2020 Google LLC
> > + */
> > +
> > +#include <keys/user-type.h>
> > +#include <linux/blk-crypto.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/buffer_head.h>
> > +#include <linux/sched/mm.h>
> > +
> > +#include "fscrypt_private.h"
> > +
> > +/* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
> > +static int fscrypt_metadata_get_key_from_id(const char *prefix,
> > +					    char *descriptor_hex,
> > +					    unsigned int min_keysize,
> > +					    char *raw_key)
> > +{
> > +	char *description;
> > +	struct key *key;
> > +	const struct user_key_payload *ukp;
> > +	const struct fscrypt_key *payload;
> > +	int err = -ENOKEY;
> > +
> > +	if (strlen(descriptor_hex) != FSCRYPT_KEY_DESCRIPTOR_SIZE * 2)
> > +		return -EINVAL;
> > +
> > +	description = kasprintf(GFP_NOFS, "%s%s", prefix, descriptor_hex);
> > +	if (!description)
> > +		return -ENOMEM;
> > +
> > +	key = request_key(&key_type_logon, description, NULL);
> > +	kfree(description);
> > +	if (IS_ERR(key))
> > +		return PTR_ERR(key);
> > +
> > +	down_read(&key->sem);
> > +	ukp = user_key_payload_locked(key);
> > +
> > +	if (!ukp) /* was the key revoked before we acquired its semaphore? */
> > +		goto out;
> > +
> > +	payload = (const struct fscrypt_key *)ukp->data;
> 
> 'struct fscrypt_key' was a mistake.  How about having the key payload just be
> the raw key?
> 
> Or are you thinking that reserved fields will be needed?
> 
Ah, I should've just made it the raw key to start with - I can't think
of any reserved fields we might need when specifying the key (I thought
we might need something like that when we try to support hardware
wrapped keys with metadata encryption, but we could use extra fields in
the superblock for that).

> > +/**
> > + * fscrypt_setup_metadata_encryption() - prepare a super_block for metadata
> > + *					 encryption
> > + * @sb: The super_block to set up metadata encryption for
> > + * @key_desc_hex: The key descriptor (in hex) to look for in the logon keyring.
> 
> There's no such thing as a "logon keyring".  I think you mean "look for a logon
> key in the process-subscribed keyrings".
> 
Ah, I see - thanks!
> > + * @fscrypt_mode_num: The FSCRYPT_MODE_* to use as the encryption algorithm.
> > + *
> > + * Return: 0 on success, negative number on error.
> > + */
> > +int fscrypt_setup_metadata_encryption(struct super_block *sb,
> > +				      char *key_desc_hex,
> > +				      int fscrypt_mode_num)
> > +{
> > +	int err = 0;
> > +	enum blk_crypto_mode_num crypto_mode;
> > +	unsigned int lblk_bits = 64;
> > +	unsigned int dun_bytes;
> > +	unsigned int dummy;
> > +	char raw_key[FSCRYPT_MAX_KEY_SIZE];
> 
> For binary data, prefer 'u8' to 'char'.
> 
> > +
> > +	if (fscrypt_mode_num > __FSCRYPT_MODE_MAX || fscrypt_mode_num < 0 ||
> > +	    !fscrypt_modes[fscrypt_mode_num].cipher_str) {
> > +		fscrypt_warn(NULL, "Invalid fscrypt mode %d specified for metadata encryption.",
> > +			     fscrypt_mode_num);
> > +		return -EOPNOTSUPP;
> > +	}
> 
> The filenames-only encryption modes (FSCRYPT_MODE_AES_256_CTS and
> FSCRYPT_MODE_AES_128_CTS) will pass this check, which seems undesired.
> 
> > +
> > +	if (sb->s_cop->get_ino_and_lblk_bits)
> > +		sb->s_cop->get_ino_and_lblk_bits(sb, &dummy, &lblk_bits);
> > +	dun_bytes = DIV_ROUND_UP(lblk_bits, 8);
> > +
> > +	if (fscrypt_modes[fscrypt_mode_num].ivsize < dun_bytes) {
> > +		fscrypt_warn(NULL, "The fscrypt mode only supports %d DUN bytes, but FS requires support for %d DUN bytes.",
> > +			     fscrypt_modes[fscrypt_mode_num].ivsize, dun_bytes);
> > +		return -EOPNOTSUPP;
> > +	}
> 
> lblk_bits is the number of bits used to represent file logical block numbers
> (e.g. ext4_lblk_t).  That's different from the filesystem-wide block number
> (e.g. ext4_fsblk_t), which is what metadata encryption will use.
> 
> > +	crypto_mode = fscrypt_modes[fscrypt_mode_num].blk_crypto_mode;
> > +
> > +	err = fscrypt_metadata_get_key_from_id(
> > +					FSCRYPT_KEY_DESC_PREFIX,
> > +					key_desc_hex,
> > +					fscrypt_modes[fscrypt_mode_num].keysize,
> > +					raw_key);
> > +	if (err)
> > +		goto out;
> 
> This is allowing for the key to be longer than the provided keysize, in which
> case only a prefix of the key is used.
> 
> It should require the exact keysize instead.
> 
> > +
> > +	sb->s_metadata_key = kzalloc(sizeof(*sb->s_metadata_key), GFP_NOFS);
> 
> No need for GFP_NOFS here.
> 
> > +/**
> > + * fscrypt_free_metadata_encryption() - free metadata encryption fields in
> > + *					super_block.
> > + * @sb: The super_block to free metatdata encryption fields from
> > + */
> > +void fscrypt_free_metadata_encryption(struct super_block *sb)
> > +{
> > +	int num_devices;
> > +	int i;
> > +	struct request_queue *q;
> > +
> > +	if (!sb->s_metadata_key)
> > +		return;
> > +
> > +	num_devices = fscrypt_get_num_devices(sb);
> > +
> > +	for (i = 0; i < num_devices; i++) {
> > +		q = fscrypt_get_device(sb, i);
> > +		if (WARN_ON(!q))
> > +			continue;
> > +		blk_crypto_evict_key(q, sb->s_metadata_key);
> > +	}
> > +
> > +	memzero_explicit(sb->s_metadata_key, sizeof(*sb->s_metadata_key));
> > +	kzfree(sb->s_metadata_key);
> > +	sb->s_metadata_key = NULL;
> > +}
> 
> kfree_sensitive(), not kzfree().
> 
> Also, memzero_explicit() is redundant.
> 
> > +/**
> > + * fscrypt_metadata_crypt_bio() - Add metadata encryption context to bio.
> > + *
> > + * @bio: The bio to add the encryption context to
> > + * @lblk: The logical block number within the filesystem at which this bio
> > + *	  starts reading/writing data.
> 
> Should be:
> 
>    @fsblk: The block number within the filesystem ...
> 
> > + * @sb: The superblock of the filesystem
> > + * @gfp_mask: gfp_mask for bio_crypt_context allocation
> > + */
> > +void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
> > +				struct super_block *sb, gfp_t gfp_mask)
> > +{
> > +	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { 0 };
> > +
> > +	if (!sb->s_metadata_key)
> > +		return;
> > +
> > +	dun[0] = lblk;
> > +	bio_crypt_set_ctx(bio, sb->s_metadata_key, dun, gfp_mask);
> > +}
> 
> Perhaps fscrypt_set_bio_crypt_ctx() should call this?  It seems there should be
> a single function that filesystems can call that handles setting the
> bio_crypt_ctx for both file contents and metadata encryption.
> 
I mistakenly dismissed this idea when I was coding this up :( - I'll do
this for the next version... I think it'll also make supporting direct I/O
easier in future :) . Also, I might require FS_ENCRYPTION_INLINE_CRYPT
when enabling FS_ENCRYPTION_METADATA to maybe make the code slightly
cleaner (unless there's a reason we want to support metadata encryption
without FS inline encryption being enabled?).
> - Eric

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 2/3] fscrypt: Add metadata encryption support
@ 2020-10-07 23:28       ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-07 23:28 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Wed, Oct 07, 2020 at 01:52:21PM -0700, Eric Biggers wrote:
> On Mon, Oct 05, 2020 at 07:36:05AM +0000, Satya Tangirala wrote:
> > Introduces functions that help with metadata encryption.
> > 
> > In particular, we introduce:
> > 
> > fscrypt_setup_metadata_encryption() - filesystems should call this function
> > to set up metadata encryption on a super block with the encryption
> > algorithm (the desired FSCRYPT_MODE_*) and the key descriptor of the
> > encryption key. The key descriptor is looked up in the logon keyring of the
> > current session using "fscrypt:" as the descriptor prefix.
> > 
> > fscrypt_metadata_crypt_bio() - filesystems should call this function on a
> > bio that it wants metadata crypted. This function will set a bio-crypt-ctx
> > on the bio if the metadata key was set up with
> > fscrypt_setup_metadata_encryption(). The DUN for the first block in the bio
> > is the offset of that block from the start of the filesystem.
> > 
> > fscrypt_free_metadata_encryption() - this function should be called when
> > the super block is being freed. It ensures that the metadata encryption key
> > is evicted, if necessary, from devices.
> > 
> > Note that the filesystem (rather than fscrypt) controls precisely which
> > blocks are encrypted with the metadata encryption key and which blocks are
> > encrypted with other keys/not encrypted at all. Fscrypt only provides some
> > convenience functions that ultimately help encrypt a bio with the metadata
> > encryption key when desired.
> > 
> > Signed-off-by: Satya Tangirala <satyat@google.com>
> > ---
> >  fs/crypto/Kconfig           |   6 +
> >  fs/crypto/Makefile          |   1 +
> >  fs/crypto/fscrypt_private.h |  19 ++++
> >  fs/crypto/inline_crypt.c    |  18 ---
> >  fs/crypto/metadata_crypt.c  | 220 ++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h          |   3 +
> >  include/linux/fscrypt.h     |  47 ++++++++
> >  7 files changed, 296 insertions(+), 18 deletions(-)
> >  create mode 100644 fs/crypto/metadata_crypt.c
> > 
> > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> > index a5f5c30368a2..3010e91f6659 100644
> > --- a/fs/crypto/Kconfig
> > +++ b/fs/crypto/Kconfig
> > @@ -30,3 +30,9 @@ config FS_ENCRYPTION_INLINE_CRYPT
> >  	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
> >  	help
> >  	  Enable fscrypt to use inline encryption hardware if available.
> > +
> > +config FS_ENCRYPTION_METADATA
> > +	bool "Enable metadata encryption with fscrypt"
> > +	depends on FS_ENCRYPTION && BLK_INLINE_ENCRYPTION
> > +	help
> > +	  Enable fscrypt to encrypt metadata.
> 
> This needs Kconfig help text to describe what this feature is and why anyone
> would want to enable it.  It also needs an update to
> Documentation/filesystems/fscrypt.rst, and a test in xfstests that tests that
> the encryption is being done correctly.
> 
Sure. I forgot to mention, fwiw I did hack xfstests to enable metadata
encryption on each device to try to test the code, and also some other
informal tests, but as you point out, I should send out actual xfstests
to test this.
> > diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
> > new file mode 100644
> > index 000000000000..5e16df130509
> > --- /dev/null
> > +++ b/fs/crypto/metadata_crypt.c
> > @@ -0,0 +1,220 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Metadata encryption support for fscrypt
> > + *
> > + * Copyright 2020 Google LLC
> > + */
> > +
> > +#include <keys/user-type.h>
> > +#include <linux/blk-crypto.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/buffer_head.h>
> > +#include <linux/sched/mm.h>
> > +
> > +#include "fscrypt_private.h"
> > +
> > +/* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
> > +static int fscrypt_metadata_get_key_from_id(const char *prefix,
> > +					    char *descriptor_hex,
> > +					    unsigned int min_keysize,
> > +					    char *raw_key)
> > +{
> > +	char *description;
> > +	struct key *key;
> > +	const struct user_key_payload *ukp;
> > +	const struct fscrypt_key *payload;
> > +	int err = -ENOKEY;
> > +
> > +	if (strlen(descriptor_hex) != FSCRYPT_KEY_DESCRIPTOR_SIZE * 2)
> > +		return -EINVAL;
> > +
> > +	description = kasprintf(GFP_NOFS, "%s%s", prefix, descriptor_hex);
> > +	if (!description)
> > +		return -ENOMEM;
> > +
> > +	key = request_key(&key_type_logon, description, NULL);
> > +	kfree(description);
> > +	if (IS_ERR(key))
> > +		return PTR_ERR(key);
> > +
> > +	down_read(&key->sem);
> > +	ukp = user_key_payload_locked(key);
> > +
> > +	if (!ukp) /* was the key revoked before we acquired its semaphore? */
> > +		goto out;
> > +
> > +	payload = (const struct fscrypt_key *)ukp->data;
> 
> 'struct fscrypt_key' was a mistake.  How about having the key payload just be
> the raw key?
> 
> Or are you thinking that reserved fields will be needed?
> 
Ah, I should've just made it the raw key to start with - I can't think
of any reserved fields we might need when specifying the key (I thought
we might need something like that when we try to support hardware
wrapped keys with metadata encryption, but we could use extra fields in
the superblock for that).

> > +/**
> > + * fscrypt_setup_metadata_encryption() - prepare a super_block for metadata
> > + *					 encryption
> > + * @sb: The super_block to set up metadata encryption for
> > + * @key_desc_hex: The key descriptor (in hex) to look for in the logon keyring.
> 
> There's no such thing as a "logon keyring".  I think you mean "look for a logon
> key in the process-subscribed keyrings".
> 
Ah, I see - thanks!
> > + * @fscrypt_mode_num: The FSCRYPT_MODE_* to use as the encryption algorithm.
> > + *
> > + * Return: 0 on success, negative number on error.
> > + */
> > +int fscrypt_setup_metadata_encryption(struct super_block *sb,
> > +				      char *key_desc_hex,
> > +				      int fscrypt_mode_num)
> > +{
> > +	int err = 0;
> > +	enum blk_crypto_mode_num crypto_mode;
> > +	unsigned int lblk_bits = 64;
> > +	unsigned int dun_bytes;
> > +	unsigned int dummy;
> > +	char raw_key[FSCRYPT_MAX_KEY_SIZE];
> 
> For binary data, prefer 'u8' to 'char'.
> 
> > +
> > +	if (fscrypt_mode_num > __FSCRYPT_MODE_MAX || fscrypt_mode_num < 0 ||
> > +	    !fscrypt_modes[fscrypt_mode_num].cipher_str) {
> > +		fscrypt_warn(NULL, "Invalid fscrypt mode %d specified for metadata encryption.",
> > +			     fscrypt_mode_num);
> > +		return -EOPNOTSUPP;
> > +	}
> 
> The filenames-only encryption modes (FSCRYPT_MODE_AES_256_CTS and
> FSCRYPT_MODE_AES_128_CTS) will pass this check, which seems undesired.
> 
> > +
> > +	if (sb->s_cop->get_ino_and_lblk_bits)
> > +		sb->s_cop->get_ino_and_lblk_bits(sb, &dummy, &lblk_bits);
> > +	dun_bytes = DIV_ROUND_UP(lblk_bits, 8);
> > +
> > +	if (fscrypt_modes[fscrypt_mode_num].ivsize < dun_bytes) {
> > +		fscrypt_warn(NULL, "The fscrypt mode only supports %d DUN bytes, but FS requires support for %d DUN bytes.",
> > +			     fscrypt_modes[fscrypt_mode_num].ivsize, dun_bytes);
> > +		return -EOPNOTSUPP;
> > +	}
> 
> lblk_bits is the number of bits used to represent file logical block numbers
> (e.g. ext4_lblk_t).  That's different from the filesystem-wide block number
> (e.g. ext4_fsblk_t), which is what metadata encryption will use.
> 
> > +	crypto_mode = fscrypt_modes[fscrypt_mode_num].blk_crypto_mode;
> > +
> > +	err = fscrypt_metadata_get_key_from_id(
> > +					FSCRYPT_KEY_DESC_PREFIX,
> > +					key_desc_hex,
> > +					fscrypt_modes[fscrypt_mode_num].keysize,
> > +					raw_key);
> > +	if (err)
> > +		goto out;
> 
> This is allowing for the key to be longer than the provided keysize, in which
> case only a prefix of the key is used.
> 
> It should require the exact keysize instead.
> 
> > +
> > +	sb->s_metadata_key = kzalloc(sizeof(*sb->s_metadata_key), GFP_NOFS);
> 
> No need for GFP_NOFS here.
> 
> > +/**
> > + * fscrypt_free_metadata_encryption() - free metadata encryption fields in
> > + *					super_block.
> > + * @sb: The super_block to free metatdata encryption fields from
> > + */
> > +void fscrypt_free_metadata_encryption(struct super_block *sb)
> > +{
> > +	int num_devices;
> > +	int i;
> > +	struct request_queue *q;
> > +
> > +	if (!sb->s_metadata_key)
> > +		return;
> > +
> > +	num_devices = fscrypt_get_num_devices(sb);
> > +
> > +	for (i = 0; i < num_devices; i++) {
> > +		q = fscrypt_get_device(sb, i);
> > +		if (WARN_ON(!q))
> > +			continue;
> > +		blk_crypto_evict_key(q, sb->s_metadata_key);
> > +	}
> > +
> > +	memzero_explicit(sb->s_metadata_key, sizeof(*sb->s_metadata_key));
> > +	kzfree(sb->s_metadata_key);
> > +	sb->s_metadata_key = NULL;
> > +}
> 
> kfree_sensitive(), not kzfree().
> 
> Also, memzero_explicit() is redundant.
> 
> > +/**
> > + * fscrypt_metadata_crypt_bio() - Add metadata encryption context to bio.
> > + *
> > + * @bio: The bio to add the encryption context to
> > + * @lblk: The logical block number within the filesystem at which this bio
> > + *	  starts reading/writing data.
> 
> Should be:
> 
>    @fsblk: The block number within the filesystem ...
> 
> > + * @sb: The superblock of the filesystem
> > + * @gfp_mask: gfp_mask for bio_crypt_context allocation
> > + */
> > +void fscrypt_metadata_crypt_bio(struct bio *bio, u64 lblk,
> > +				struct super_block *sb, gfp_t gfp_mask)
> > +{
> > +	u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { 0 };
> > +
> > +	if (!sb->s_metadata_key)
> > +		return;
> > +
> > +	dun[0] = lblk;
> > +	bio_crypt_set_ctx(bio, sb->s_metadata_key, dun, gfp_mask);
> > +}
> 
> Perhaps fscrypt_set_bio_crypt_ctx() should call this?  It seems there should be
> a single function that filesystems can call that handles setting the
> bio_crypt_ctx for both file contents and metadata encryption.
> 
I mistakenly dismissed this idea when I was coding this up :( - I'll do
this for the next version... I think it'll also make supporting direct I/O
easier in future :) . Also, I might require FS_ENCRYPTION_INLINE_CRYPT
when enabling FS_ENCRYPTION_METADATA to maybe make the code slightly
cleaner (unless there's a reason we want to support metadata encryption
without FS inline encryption being enabled?).
> - Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 3/3] f2fs: Add metadata encryption support
  2020-10-07 21:20     ` [f2fs-dev] " Eric Biggers
@ 2020-10-08  0:31       ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-10-08  0:31 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Wed, Oct 07, 2020 at 02:20:52PM -0700, Eric Biggers wrote:
> On Mon, Oct 05, 2020 at 07:36:06AM +0000, Satya Tangirala wrote:
> > Wire up metadata encryption support with the fscrypt metadata crypt
> > additions.
> > 
> > Introduces a new mount option for metadata encryption -
> > metadata_crypt_key=%s. The argument to this option is the key descriptor of
> > the metadata encryption key in hex. 
> 
> It's unclear what "key descriptor in hex" means in this context.  Keys in the
> Linux keyrings subsystem can be specified either by an integer ID or by a string
> "description".
> 
> fscrypt_policy_v1 has an 8-byte binary master_key_descriptor, which specifies a
> keyring key with description "fscrypt:" + ToHex(master_key_descriptor).  So I'm
> guessing that's where this terminology is coming from.
> 
> However, here the value passed to metadata_crypt_key is just a key description
> that's passed directly to the Linux keyrings subsystem.  I don't see why it has
> to be a hex string (and it fact, it seems it's not enforced?).
Yeah, I really meant "string description". Also I'll be putting the
key identifier in the superblock so this mount option will be going
away.
> 
> The current proposal is also missing any sort of key verification.  The
> filesystem will use any key that is provided, even if a different key was used
> at format time.
>
I was relying on the validate_checkpoint() to fail when it tries to
verify the checkpoint checksum if an incorrect key is provided, but that
does sound bad to rely on from a design perspective. I'll do what you
mentioned below.
> In "fscrypt v2", we solved the equivalent problem by making the keys be
> specified by a HKDF-derived master_key_identifier.
> 
> How about doing something similar for the metadata encryption key?  I.e. the
> metadata encryption key could be used as input to HKDF to derive two subkeys:
> metadata_key_identifier and the real metadata encryption key.  Then
> metadata_key_identifier could be stored in the superblock.  Then the filesystem
> could request the keyring key "fscrypt:" + ToHex(metadata_key_identifier) at
> mount time, which would eliminate the need for a mount option.
> 
> > Direct I/O with metadata encryption is also not supported for now.
> > Attempts to do direct I/O on a metadata encrypted F2FS filesystem will fall
> > back to using buffered I/O (just as attempts to do direct I/O on fscrypt
> > encrypted files also fall back to buffered I/O).
> 
> What would it take to get direct I/O working?
> 
I think we'd first need to get the direct I/O with fscrypt via
blk-crypto patches in (i.e. the patch series at

https://lore.kernel.org/linux-fscrypt/20200724184501.1651378-1-satyat@google.com/
)

At least for single device filesystems, it shouldn't be much extra work to
support metadata encryption with the above patch in, especially once I make
fscrypt_set_bio_crypt_ctx() handle setting both the metadata encryption
and file encryption keys as you suggested in the previous patch - For
multi device filesystems, we'd need the offset of the block from the
start of the FS rather than offset of the block from the start of the
device that block belongs to (through my cursory glance at
dio_bio_alloc() where the above patch calls fscrypt_set_bio_crypt_ctx(),
I can see that the latter is readily available as first_sector, but I'm
not sure about the former - I'd imagine we can get that from the
dio->inode or something like that, or maybe some extra plumbing is
required).

> > +#ifdef CONFIG_FS_ENCRYPTION_METADATA
> > +	if (metadata_crypt_alg &&
> > +	    !fscrypt_metadata_crypted(sb)) {
> > +		f2fs_err(sbi, "Filesystem has metadata encryption. Please provide metadata encryption key to mount filesystem");
> > +		return -EINVAL;
> > +	}
> > +#endif
> 
> Please try to avoid #ifdefs.  It looks like some of these could be replaced with
> IS_ENABLED() or the use of stub functions.
> 
> - Eric

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 3/3] f2fs: Add metadata encryption support
@ 2020-10-08  0:31       ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-10-08  0:31 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Wed, Oct 07, 2020 at 02:20:52PM -0700, Eric Biggers wrote:
> On Mon, Oct 05, 2020 at 07:36:06AM +0000, Satya Tangirala wrote:
> > Wire up metadata encryption support with the fscrypt metadata crypt
> > additions.
> > 
> > Introduces a new mount option for metadata encryption -
> > metadata_crypt_key=%s. The argument to this option is the key descriptor of
> > the metadata encryption key in hex. 
> 
> It's unclear what "key descriptor in hex" means in this context.  Keys in the
> Linux keyrings subsystem can be specified either by an integer ID or by a string
> "description".
> 
> fscrypt_policy_v1 has an 8-byte binary master_key_descriptor, which specifies a
> keyring key with description "fscrypt:" + ToHex(master_key_descriptor).  So I'm
> guessing that's where this terminology is coming from.
> 
> However, here the value passed to metadata_crypt_key is just a key description
> that's passed directly to the Linux keyrings subsystem.  I don't see why it has
> to be a hex string (and it fact, it seems it's not enforced?).
Yeah, I really meant "string description". Also I'll be putting the
key identifier in the superblock so this mount option will be going
away.
> 
> The current proposal is also missing any sort of key verification.  The
> filesystem will use any key that is provided, even if a different key was used
> at format time.
>
I was relying on the validate_checkpoint() to fail when it tries to
verify the checkpoint checksum if an incorrect key is provided, but that
does sound bad to rely on from a design perspective. I'll do what you
mentioned below.
> In "fscrypt v2", we solved the equivalent problem by making the keys be
> specified by a HKDF-derived master_key_identifier.
> 
> How about doing something similar for the metadata encryption key?  I.e. the
> metadata encryption key could be used as input to HKDF to derive two subkeys:
> metadata_key_identifier and the real metadata encryption key.  Then
> metadata_key_identifier could be stored in the superblock.  Then the filesystem
> could request the keyring key "fscrypt:" + ToHex(metadata_key_identifier) at
> mount time, which would eliminate the need for a mount option.
> 
> > Direct I/O with metadata encryption is also not supported for now.
> > Attempts to do direct I/O on a metadata encrypted F2FS filesystem will fall
> > back to using buffered I/O (just as attempts to do direct I/O on fscrypt
> > encrypted files also fall back to buffered I/O).
> 
> What would it take to get direct I/O working?
> 
I think we'd first need to get the direct I/O with fscrypt via
blk-crypto patches in (i.e. the patch series at

https://lore.kernel.org/linux-fscrypt/20200724184501.1651378-1-satyat@google.com/
)

At least for single device filesystems, it shouldn't be much extra work to
support metadata encryption with the above patch in, especially once I make
fscrypt_set_bio_crypt_ctx() handle setting both the metadata encryption
and file encryption keys as you suggested in the previous patch - For
multi device filesystems, we'd need the offset of the block from the
start of the FS rather than offset of the block from the start of the
device that block belongs to (through my cursory glance at
dio_bio_alloc() where the above patch calls fscrypt_set_bio_crypt_ctx(),
I can see that the latter is readily available as first_sector, but I'm
not sure about the former - I'd imagine we can get that from the
dio->inode or something like that, or maybe some extra plumbing is
required).

> > +#ifdef CONFIG_FS_ENCRYPTION_METADATA
> > +	if (metadata_crypt_alg &&
> > +	    !fscrypt_metadata_crypted(sb)) {
> > +		f2fs_err(sbi, "Filesystem has metadata encryption. Please provide metadata encryption key to mount filesystem");
> > +		return -EINVAL;
> > +	}
> > +#endif
> 
> Please try to avoid #ifdefs.  It looks like some of these could be replaced with
> IS_ENABLED() or the use of stub functions.
> 
> - Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-10-07 22:05     ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-08 17:01       ` Eric Biggers
  -1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-08 17:01 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Wed, Oct 07, 2020 at 10:05:00PM +0000, Satya Tangirala wrote:
> > I notice this is missing the step I suggested to include the metadata encryption
> > key in the HKDF application-specific info string when deriving subkeys from the
> > fscrypt master keys.
> > 
> > The same effect could also be achieved by adding an additional level to the key
> > hierarchy: each HKDF key would be derived from a fscrypt master key and the
> > metadata encryption key.
> > 
> > We need one of those, to guarantee that the file contents encryption is at least
> > as strong as the "metadata encryption".
> >
> Yes - I didn't get around to that in the first version, but I'll add
> that too in the next version. I was going to go with the first approach
> before I saw your comment - is there one method you'd recommend going
> with over the other?

I'm not entirely sure, but I'm now leaning towards the second approach because
it would avoid adding additional work (another SHA-512 block) to all later key
derivations.  Also it would avoid having to add a super_block argument to
fscrypt_hkdf_expand().  But please ask Paul Crowley for his suggestion too.

Here's a quick untested patch to consider:

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index dca254590a70..67f8ba3098d3 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -319,6 +319,7 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
 #define HKDF_CONTEXT_DIRHASH_KEY	5 /* info=file_nonce		*/
 #define HKDF_CONTEXT_IV_INO_LBLK_32_KEY	6 /* info=mode_num||fs_uuid	*/
 #define HKDF_CONTEXT_INODE_HASH_KEY	7 /* info=<empty>		*/
+#define HKDF_CONTEXT_MIX_METADATA_KEY	8 /* info=metadata_key		*/
 
 int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 			const u8 *info, unsigned int infolen,
@@ -600,6 +601,20 @@ int fscrypt_setup_v1_file_key(struct fscrypt_info *ci,
 
 int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci);
 
+/* metadata_crypt.c */
+
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+int fscrypt_mix_in_metadata_key(struct super_block *sb,
+				struct fscrypt_master_key_secret *secret);
+#else
+static inline int
+fscrypt_mix_in_metadata_key(struct super_block *sb,
+			    struct fscrypt_master_key_secret *secret)
+{
+	return 0;
+}
+#endif
+
 /* policy.c */
 
 bool fscrypt_policies_equal(const union fscrypt_policy *policy1,
diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
index 0cba7928446d..61d1f0aa802e 100644
--- a/fs/crypto/hkdf.c
+++ b/fs/crypto/hkdf.c
@@ -174,4 +174,5 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf)
 {
 	crypto_free_shash(hkdf->hmac_tfm);
+	hkdf->hmac_tfm = NULL;
 }
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index e74f239c4428..43453a7f77b1 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -494,6 +494,10 @@ static int add_master_key(struct super_block *sb,
 		 */
 		memzero_explicit(secret->raw, secret->size);
 
+		err = fscrypt_mix_in_metadata_key(sb, secret);
+		if (err)
+			return err;
+
 		/* Calculate the key identifier */
 		err = fscrypt_hkdf_expand(&secret->hkdf,
 					  HKDF_CONTEXT_KEY_IDENTIFIER, NULL, 0,
diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
index 5e16df130509..233e68c35504 100644
--- a/fs/crypto/metadata_crypt.c
+++ b/fs/crypto/metadata_crypt.c
@@ -13,6 +13,32 @@
 
 #include "fscrypt_private.h"
 
+/* TODO: add comment */
+int fscrypt_mix_in_metadata_key(struct super_block *sb,
+				struct fscrypt_master_key_secret *secret)
+{
+	u8 real_key[FSCRYPT_MAX_KEY_SIZE];
+	int err;
+
+	if (WARN_ON(secret->size > sizeof(real_key)))
+		return -EINVAL;
+
+	if (!sb->s_metadata_key)
+		return 0;
+
+	err = fscrypt_hkdf_expand(&secret->hkdf, HKDF_CONTEXT_MIX_METADATA_KEY,
+				  sb->s_metadata_key->raw,
+				  sb->s_metadata_key->size,
+				  real_key, secret->size);
+	if (err)
+		return err;
+
+	fscrypt_destroy_hkdf(&secret->hkdf);
+	err = fscrypt_init_hkdf(&secret->hkdf, real_key, secret->size);
+	memzero_explicit(real_key, secret->size);
+	return err;
+}
+
 /* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
 static int fscrypt_metadata_get_key_from_id(const char *prefix,
 					    char *descriptor_hex,

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-10-08 17:01       ` Eric Biggers
  0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-08 17:01 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Wed, Oct 07, 2020 at 10:05:00PM +0000, Satya Tangirala wrote:
> > I notice this is missing the step I suggested to include the metadata encryption
> > key in the HKDF application-specific info string when deriving subkeys from the
> > fscrypt master keys.
> > 
> > The same effect could also be achieved by adding an additional level to the key
> > hierarchy: each HKDF key would be derived from a fscrypt master key and the
> > metadata encryption key.
> > 
> > We need one of those, to guarantee that the file contents encryption is at least
> > as strong as the "metadata encryption".
> >
> Yes - I didn't get around to that in the first version, but I'll add
> that too in the next version. I was going to go with the first approach
> before I saw your comment - is there one method you'd recommend going
> with over the other?

I'm not entirely sure, but I'm now leaning towards the second approach because
it would avoid adding additional work (another SHA-512 block) to all later key
derivations.  Also it would avoid having to add a super_block argument to
fscrypt_hkdf_expand().  But please ask Paul Crowley for his suggestion too.

Here's a quick untested patch to consider:

diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index dca254590a70..67f8ba3098d3 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -319,6 +319,7 @@ int fscrypt_init_hkdf(struct fscrypt_hkdf *hkdf, const u8 *master_key,
 #define HKDF_CONTEXT_DIRHASH_KEY	5 /* info=file_nonce		*/
 #define HKDF_CONTEXT_IV_INO_LBLK_32_KEY	6 /* info=mode_num||fs_uuid	*/
 #define HKDF_CONTEXT_INODE_HASH_KEY	7 /* info=<empty>		*/
+#define HKDF_CONTEXT_MIX_METADATA_KEY	8 /* info=metadata_key		*/
 
 int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 			const u8 *info, unsigned int infolen,
@@ -600,6 +601,20 @@ int fscrypt_setup_v1_file_key(struct fscrypt_info *ci,
 
 int fscrypt_setup_v1_file_key_via_subscribed_keyrings(struct fscrypt_info *ci);
 
+/* metadata_crypt.c */
+
+#ifdef CONFIG_FS_ENCRYPTION_METADATA
+int fscrypt_mix_in_metadata_key(struct super_block *sb,
+				struct fscrypt_master_key_secret *secret);
+#else
+static inline int
+fscrypt_mix_in_metadata_key(struct super_block *sb,
+			    struct fscrypt_master_key_secret *secret)
+{
+	return 0;
+}
+#endif
+
 /* policy.c */
 
 bool fscrypt_policies_equal(const union fscrypt_policy *policy1,
diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
index 0cba7928446d..61d1f0aa802e 100644
--- a/fs/crypto/hkdf.c
+++ b/fs/crypto/hkdf.c
@@ -174,4 +174,5 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 void fscrypt_destroy_hkdf(struct fscrypt_hkdf *hkdf)
 {
 	crypto_free_shash(hkdf->hmac_tfm);
+	hkdf->hmac_tfm = NULL;
 }
diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c
index e74f239c4428..43453a7f77b1 100644
--- a/fs/crypto/keyring.c
+++ b/fs/crypto/keyring.c
@@ -494,6 +494,10 @@ static int add_master_key(struct super_block *sb,
 		 */
 		memzero_explicit(secret->raw, secret->size);
 
+		err = fscrypt_mix_in_metadata_key(sb, secret);
+		if (err)
+			return err;
+
 		/* Calculate the key identifier */
 		err = fscrypt_hkdf_expand(&secret->hkdf,
 					  HKDF_CONTEXT_KEY_IDENTIFIER, NULL, 0,
diff --git a/fs/crypto/metadata_crypt.c b/fs/crypto/metadata_crypt.c
index 5e16df130509..233e68c35504 100644
--- a/fs/crypto/metadata_crypt.c
+++ b/fs/crypto/metadata_crypt.c
@@ -13,6 +13,32 @@
 
 #include "fscrypt_private.h"
 
+/* TODO: add comment */
+int fscrypt_mix_in_metadata_key(struct super_block *sb,
+				struct fscrypt_master_key_secret *secret)
+{
+	u8 real_key[FSCRYPT_MAX_KEY_SIZE];
+	int err;
+
+	if (WARN_ON(secret->size > sizeof(real_key)))
+		return -EINVAL;
+
+	if (!sb->s_metadata_key)
+		return 0;
+
+	err = fscrypt_hkdf_expand(&secret->hkdf, HKDF_CONTEXT_MIX_METADATA_KEY,
+				  sb->s_metadata_key->raw,
+				  sb->s_metadata_key->size,
+				  real_key, secret->size);
+	if (err)
+		return err;
+
+	fscrypt_destroy_hkdf(&secret->hkdf);
+	err = fscrypt_init_hkdf(&secret->hkdf, real_key, secret->size);
+	memzero_explicit(real_key, secret->size);
+	return err;
+}
+
 /* TODO: mostly copied from keysetup_v1.c - maybe refactor this function */
 static int fscrypt_metadata_get_key_from_id(const char *prefix,
 					    char *descriptor_hex,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [PATCH 2/3] fscrypt: Add metadata encryption support
  2020-10-07 23:28       ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-08 17:05         ` Eric Biggers
  -1 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-08 17:05 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Chao Yu, linux-kernel,
	linux-fscrypt, linux-f2fs-devel

On Wed, Oct 07, 2020 at 11:28:06PM +0000, Satya Tangirala wrote:
> > This needs Kconfig help text to describe what this feature is and why anyone
> > would want to enable it.  It also needs an update to
> > Documentation/filesystems/fscrypt.rst, and a test in xfstests that tests that
> > the encryption is being done correctly.
> > 
> Sure. I forgot to mention, fwiw I did hack xfstests to enable metadata
> encryption on each device to try to test the code, and also some other
> informal tests, but as you point out, I should send out actual xfstests
> to test this.

To be clear, I'm asking for tests which verify the actual ciphertext written to
disk.  So similar to _verify_ciphertext_for_encryption_policy() in xfstests, or
to vts_kernel_encryption_test in Android's VTS.

> > Perhaps fscrypt_set_bio_crypt_ctx() should call this?  It seems there should be
> > a single function that filesystems can call that handles setting the
> > bio_crypt_ctx for both file contents and metadata encryption.
> > 
> I mistakenly dismissed this idea when I was coding this up :( - I'll do
> this for the next version... I think it'll also make supporting direct I/O
> easier in future :) . Also, I might require FS_ENCRYPTION_INLINE_CRYPT
> when enabling FS_ENCRYPTION_METADATA to maybe make the code slightly
> cleaner (unless there's a reason we want to support metadata encryption
> without FS inline encryption being enabled?).

Since metadata encryption would already depend on FS_ENCRYPTION and
BLK_INLINE_ENCRYPTION, I think it would be fine to require
FS_ENCRYPTION_INLINE_CRYPT too, in order to reduce the number of combinations.

- Eric

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 2/3] fscrypt: Add metadata encryption support
@ 2020-10-08 17:05         ` Eric Biggers
  0 siblings, 0 replies; 42+ messages in thread
From: Eric Biggers @ 2020-10-08 17:05 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	linux-fscrypt, Jaegeuk Kim

On Wed, Oct 07, 2020 at 11:28:06PM +0000, Satya Tangirala wrote:
> > This needs Kconfig help text to describe what this feature is and why anyone
> > would want to enable it.  It also needs an update to
> > Documentation/filesystems/fscrypt.rst, and a test in xfstests that tests that
> > the encryption is being done correctly.
> > 
> Sure. I forgot to mention, fwiw I did hack xfstests to enable metadata
> encryption on each device to try to test the code, and also some other
> informal tests, but as you point out, I should send out actual xfstests
> to test this.

To be clear, I'm asking for tests which verify the actual ciphertext written to
disk.  So similar to _verify_ciphertext_for_encryption_policy() in xfstests, or
to vts_kernel_encryption_test in Android's VTS.

> > Perhaps fscrypt_set_bio_crypt_ctx() should call this?  It seems there should be
> > a single function that filesystems can call that handles setting the
> > bio_crypt_ctx for both file contents and metadata encryption.
> > 
> I mistakenly dismissed this idea when I was coding this up :( - I'll do
> this for the next version... I think it'll also make supporting direct I/O
> easier in future :) . Also, I might require FS_ENCRYPTION_INLINE_CRYPT
> when enabling FS_ENCRYPTION_METADATA to maybe make the code slightly
> cleaner (unless there's a reason we want to support metadata encryption
> without FS inline encryption being enabled?).

Since metadata encryption would already depend on FS_ENCRYPTION and
BLK_INLINE_ENCRYPTION, I think it would be fine to require
FS_ENCRYPTION_INLINE_CRYPT too, in order to reduce the number of combinations.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-10-05  7:36 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-10-10  9:53   ` Chao Yu
  -1 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-10-10  9:53 UTC (permalink / raw)
  To: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim,
	Eric Biggers, Chao Yu
  Cc: linux-kernel, linux-fscrypt, linux-f2fs-devel

On 2020/10/5 15:36, Satya Tangirala wrote:
> This patch series adds support for metadata encryption to F2FS using
> blk-crypto.

It looks this implementation is based on hardware crypto engine, could you
please add this info into f2fs.rst as well like inlinecrypt...

> 
> Patch 1 replaces fscrypt_get_devices (which took an array of request_queues
> and filled it up) with fscrypt_get_device, which takes a index of the
> desired device and returns the device at that index (so the index passed
> to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1)
> inclusive). This allows callers to avoid having to allocate an array to
> pass to fscrypt_get_devices() when they only need to iterate through
> each element in the array (and have no use for the array itself).
> 
> Patch 2 introduces some functions to fscrypt that help filesystems perform
> metadata encryption. Any filesystem that wants to use metadata encryption
> can call fscrypt_setup_metadata_encryption() with the super_block of the
> filesystem, the encryption algorithm and the descriptor of the encryption
> key. The descriptor is looked up in the logon keyring of the current
> session with "fscrypt:" as the prefix of the descriptor.
> 
> The patch also introduces fscrypt_metadata_crypt_bio() which an FS should
> call on a bio that the FS wants metadata crypted. The function will add
> an encryption context with the metadata encryption key set up by the call
> to the above mentioned fscrypt_setup_metadata_encryption().
> 
> The patch also introduces fscrypt_metadata_crypt_prepare_all_devices().
> Filesystems that use multiple devices should call this function once all
> the underlying devices have been determined. An FS might only be able to
> determine all the underlying devices after some initial processing that
> might already require metadata en/decryption, which is why this function
> is separate from fscrypt_setup_metadata_encryption().
> 
> Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
> will encrypt every block (that's not being encrypted by some other
> encryption key, e.g. a per-file key) with the metadata encryption key
> except the superblock (and the redundant copy of the superblock). The DUN
> of a block is the offset of the block from the start of the F2FS
> filesystem.

Why not using nid as DUN, then GC could migrate encrypted node block directly via
meta inode's address space like we do for encrypted data block, rather than
decrypting node block to node page and then encrypting node page with DUN of new
blkaddr it migrates to.

Thanks,

> 
> Please refer to the commit message for why the superblock was excluded from
> en/decryption, and other limitations. The superblock and its copy are
> stored in plaintext on disk. The encryption algorithm used for metadata
> encryption is stored within the superblock itself. Changes to the userspace
> tools (that are required to test out metadata encryption with F2FS) are
> also being sent out - I'll post a link as a reply to this mail once it's
> out.
> 
> Satya Tangirala (3):
>    fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
>    fscrypt: Add metadata encryption support
>    f2fs: Add metadata encryption support
> 
>   Documentation/filesystems/f2fs.rst |  12 ++
>   fs/crypto/Kconfig                  |   6 +
>   fs/crypto/Makefile                 |   1 +
>   fs/crypto/fscrypt_private.h        |  19 +++
>   fs/crypto/inline_crypt.c           |  37 +----
>   fs/crypto/metadata_crypt.c         | 220 +++++++++++++++++++++++++++++
>   fs/f2fs/data.c                     |  24 ++--
>   fs/f2fs/f2fs.h                     |   2 +
>   fs/f2fs/super.c                    |  83 +++++++++--
>   include/linux/f2fs_fs.h            |   3 +-
>   include/linux/fs.h                 |   3 +
>   include/linux/fscrypt.h            |  51 ++++++-
>   12 files changed, 410 insertions(+), 51 deletions(-)
>   create mode 100644 fs/crypto/metadata_crypt.c
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-10-10  9:53   ` Chao Yu
  0 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-10-10  9:53 UTC (permalink / raw)
  To: Satya Tangirala, Theodore Y . Ts'o, Jaegeuk Kim,
	Eric Biggers, Chao Yu
  Cc: linux-fscrypt, linux-kernel, linux-f2fs-devel

On 2020/10/5 15:36, Satya Tangirala wrote:
> This patch series adds support for metadata encryption to F2FS using
> blk-crypto.

It looks this implementation is based on hardware crypto engine, could you
please add this info into f2fs.rst as well like inlinecrypt...

> 
> Patch 1 replaces fscrypt_get_devices (which took an array of request_queues
> and filled it up) with fscrypt_get_device, which takes a index of the
> desired device and returns the device at that index (so the index passed
> to fscrypt_get_device must be between 0 and (fscrypt_get_num_devices() - 1)
> inclusive). This allows callers to avoid having to allocate an array to
> pass to fscrypt_get_devices() when they only need to iterate through
> each element in the array (and have no use for the array itself).
> 
> Patch 2 introduces some functions to fscrypt that help filesystems perform
> metadata encryption. Any filesystem that wants to use metadata encryption
> can call fscrypt_setup_metadata_encryption() with the super_block of the
> filesystem, the encryption algorithm and the descriptor of the encryption
> key. The descriptor is looked up in the logon keyring of the current
> session with "fscrypt:" as the prefix of the descriptor.
> 
> The patch also introduces fscrypt_metadata_crypt_bio() which an FS should
> call on a bio that the FS wants metadata crypted. The function will add
> an encryption context with the metadata encryption key set up by the call
> to the above mentioned fscrypt_setup_metadata_encryption().
> 
> The patch also introduces fscrypt_metadata_crypt_prepare_all_devices().
> Filesystems that use multiple devices should call this function once all
> the underlying devices have been determined. An FS might only be able to
> determine all the underlying devices after some initial processing that
> might already require metadata en/decryption, which is why this function
> is separate from fscrypt_setup_metadata_encryption().
> 
> Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
> will encrypt every block (that's not being encrypted by some other
> encryption key, e.g. a per-file key) with the metadata encryption key
> except the superblock (and the redundant copy of the superblock). The DUN
> of a block is the offset of the block from the start of the F2FS
> filesystem.

Why not using nid as DUN, then GC could migrate encrypted node block directly via
meta inode's address space like we do for encrypted data block, rather than
decrypting node block to node page and then encrypting node page with DUN of new
blkaddr it migrates to.

Thanks,

> 
> Please refer to the commit message for why the superblock was excluded from
> en/decryption, and other limitations. The superblock and its copy are
> stored in plaintext on disk. The encryption algorithm used for metadata
> encryption is stored within the superblock itself. Changes to the userspace
> tools (that are required to test out metadata encryption with F2FS) are
> also being sent out - I'll post a link as a reply to this mail once it's
> out.
> 
> Satya Tangirala (3):
>    fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device
>    fscrypt: Add metadata encryption support
>    f2fs: Add metadata encryption support
> 
>   Documentation/filesystems/f2fs.rst |  12 ++
>   fs/crypto/Kconfig                  |   6 +
>   fs/crypto/Makefile                 |   1 +
>   fs/crypto/fscrypt_private.h        |  19 +++
>   fs/crypto/inline_crypt.c           |  37 +----
>   fs/crypto/metadata_crypt.c         | 220 +++++++++++++++++++++++++++++
>   fs/f2fs/data.c                     |  24 ++--
>   fs/f2fs/f2fs.h                     |   2 +
>   fs/f2fs/super.c                    |  83 +++++++++--
>   include/linux/f2fs_fs.h            |   3 +-
>   include/linux/fs.h                 |   3 +
>   include/linux/fscrypt.h            |  51 ++++++-
>   12 files changed, 410 insertions(+), 51 deletions(-)
>   create mode 100644 fs/crypto/metadata_crypt.c
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-10-10  9:53   ` [f2fs-dev] " Chao Yu
@ 2020-12-17 15:44     ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-12-17 15:44 UTC (permalink / raw)
  To: Chao Yu
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	linux-kernel, linux-fscrypt, linux-f2fs-devel

On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
> On 2020/10/5 15:36, Satya Tangirala wrote:
> > This patch series adds support for metadata encryption to F2FS using
> > blk-crypto.
> 
> It looks this implementation is based on hardware crypto engine, could you
> please add this info into f2fs.rst as well like inlinecrypt...
> 
To be precise, the implementation requires either a hardware crypto
engine *or* blk-crypto-fallback. I tried to clarify this a little better
in the commit messages and in fscrypt.rst, but thinking about it again
now, I think I should add a section about metadata encryption at the end
of f2fs.rst. I'll do that when I send out v3 of this patch series (I
just sent out v2).
> > 
> > Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
> > will encrypt every block (that's not being encrypted by some other
> > encryption key, e.g. a per-file key) with the metadata encryption key
> > except the superblock (and the redundant copy of the superblock). The DUN
> > of a block is the offset of the block from the start of the F2FS
> > filesystem.
> 
> Why not using nid as DUN, then GC could migrate encrypted node block directly via
> meta inode's address space like we do for encrypted data block, rather than
> decrypting node block to node page and then encrypting node page with DUN of new
> blkaddr it migrates to.
> 
The issue is, the bi_crypt_context in a bio holds a single DUN value,
which is the DUN for the first data unit in the bio. blk-crypto assumes
that the DUN of each subsequent data unit can be computed by simply
incrementing the DUN. So physically contiguous data units can only be put
into the same bio if they also have contiguous DUNs. I don't know much
about nids, but if the nid is invariant w.r.t the physical block location,
then there might be more fragmentation of bios in regular read/writes
(because physical contiguity may have no relation to DUN contiguity). So I
think we should continue using the fsblk number as the DUN.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-12-17 15:44     ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-12-17 15:44 UTC (permalink / raw)
  To: Chao Yu
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	Eric Biggers, linux-fscrypt, Jaegeuk Kim

On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
> On 2020/10/5 15:36, Satya Tangirala wrote:
> > This patch series adds support for metadata encryption to F2FS using
> > blk-crypto.
> 
> It looks this implementation is based on hardware crypto engine, could you
> please add this info into f2fs.rst as well like inlinecrypt...
> 
To be precise, the implementation requires either a hardware crypto
engine *or* blk-crypto-fallback. I tried to clarify this a little better
in the commit messages and in fscrypt.rst, but thinking about it again
now, I think I should add a section about metadata encryption at the end
of f2fs.rst. I'll do that when I send out v3 of this patch series (I
just sent out v2).
> > 
> > Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
> > will encrypt every block (that's not being encrypted by some other
> > encryption key, e.g. a per-file key) with the metadata encryption key
> > except the superblock (and the redundant copy of the superblock). The DUN
> > of a block is the offset of the block from the start of the F2FS
> > filesystem.
> 
> Why not using nid as DUN, then GC could migrate encrypted node block directly via
> meta inode's address space like we do for encrypted data block, rather than
> decrypting node block to node page and then encrypting node page with DUN of new
> blkaddr it migrates to.
> 
The issue is, the bi_crypt_context in a bio holds a single DUN value,
which is the DUN for the first data unit in the bio. blk-crypto assumes
that the DUN of each subsequent data unit can be computed by simply
incrementing the DUN. So physically contiguous data units can only be put
into the same bio if they also have contiguous DUNs. I don't know much
about nids, but if the nid is invariant w.r.t the physical block location,
then there might be more fragmentation of bios in regular read/writes
(because physical contiguity may have no relation to DUN contiguity). So I
think we should continue using the fsblk number as the DUN.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-12-17 15:44     ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-12-18  9:02       ` Chao Yu
  -1 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-12-18  9:02 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	linux-kernel, linux-fscrypt, linux-f2fs-devel

On 2020/12/17 23:44, Satya Tangirala wrote:
> On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
>> On 2020/10/5 15:36, Satya Tangirala wrote:
>>> This patch series adds support for metadata encryption to F2FS using
>>> blk-crypto.
>>
>> It looks this implementation is based on hardware crypto engine, could you
>> please add this info into f2fs.rst as well like inlinecrypt...
>>
> To be precise, the implementation requires either a hardware crypto
> engine *or* blk-crypto-fallback. I tried to clarify this a little better
> in the commit messages and in fscrypt.rst, but thinking about it again
> now, I think I should add a section about metadata encryption at the end
> of f2fs.rst. I'll do that when I send out v3 of this patch series (I
> just sent out v2).
>>>
>>> Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
>>> will encrypt every block (that's not being encrypted by some other
>>> encryption key, e.g. a per-file key) with the metadata encryption key
>>> except the superblock (and the redundant copy of the superblock). The DUN
>>> of a block is the offset of the block from the start of the F2FS
>>> filesystem.
>>
>> Why not using nid as DUN, then GC could migrate encrypted node block directly via
>> meta inode's address space like we do for encrypted data block, rather than
>> decrypting node block to node page and then encrypting node page with DUN of new
>> blkaddr it migrates to.
>>
> The issue is, the bi_crypt_context in a bio holds a single DUN value,
> which is the DUN for the first data unit in the bio. blk-crypto assumes
> that the DUN of each subsequent data unit can be computed by simply
> incrementing the DUN. So physically contiguous data units can only be put
> into the same bio if they also have contiguous DUNs. I don't know much
> about nids, but if the nid is invariant w.r.t the physical block location,
> then there might be more fragmentation of bios in regular read/writes

Correct, considering performance of in batch node flush, it will be better to
use pba as IV value.

But, what's the plan about supporting software encryption for metadata? Current
f2fs write flow will handle all operations which may encounter failure before
allocating block address for node, if we do allocation first, and then use pba
as IV to encrypt node block, it will be a little complicated to revert allocation
if we fail to encrypt node block.

Thanks,

> (because physical contiguity may have no relation to DUN contiguity). So I
> think we should continue using the fsblk number as the DUN.
> .
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-12-18  9:02       ` Chao Yu
  0 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-12-18  9:02 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	Eric Biggers, linux-fscrypt, Jaegeuk Kim

On 2020/12/17 23:44, Satya Tangirala wrote:
> On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
>> On 2020/10/5 15:36, Satya Tangirala wrote:
>>> This patch series adds support for metadata encryption to F2FS using
>>> blk-crypto.
>>
>> It looks this implementation is based on hardware crypto engine, could you
>> please add this info into f2fs.rst as well like inlinecrypt...
>>
> To be precise, the implementation requires either a hardware crypto
> engine *or* blk-crypto-fallback. I tried to clarify this a little better
> in the commit messages and in fscrypt.rst, but thinking about it again
> now, I think I should add a section about metadata encryption at the end
> of f2fs.rst. I'll do that when I send out v3 of this patch series (I
> just sent out v2).
>>>
>>> Patch 3 wires up F2FS with the functions introduced in Patch 2. F2FS
>>> will encrypt every block (that's not being encrypted by some other
>>> encryption key, e.g. a per-file key) with the metadata encryption key
>>> except the superblock (and the redundant copy of the superblock). The DUN
>>> of a block is the offset of the block from the start of the F2FS
>>> filesystem.
>>
>> Why not using nid as DUN, then GC could migrate encrypted node block directly via
>> meta inode's address space like we do for encrypted data block, rather than
>> decrypting node block to node page and then encrypting node page with DUN of new
>> blkaddr it migrates to.
>>
> The issue is, the bi_crypt_context in a bio holds a single DUN value,
> which is the DUN for the first data unit in the bio. blk-crypto assumes
> that the DUN of each subsequent data unit can be computed by simply
> incrementing the DUN. So physically contiguous data units can only be put
> into the same bio if they also have contiguous DUNs. I don't know much
> about nids, but if the nid is invariant w.r.t the physical block location,
> then there might be more fragmentation of bios in regular read/writes

Correct, considering performance of in batch node flush, it will be better to
use pba as IV value.

But, what's the plan about supporting software encryption for metadata? Current
f2fs write flow will handle all operations which may encounter failure before
allocating block address for node, if we do allocation first, and then use pba
as IV to encrypt node block, it will be a little complicated to revert allocation
if we fail to encrypt node block.

Thanks,

> (because physical contiguity may have no relation to DUN contiguity). So I
> think we should continue using the fsblk number as the DUN.
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-12-18  9:02       ` [f2fs-dev] " Chao Yu
@ 2020-12-18 11:53         ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-12-18 11:53 UTC (permalink / raw)
  To: Chao Yu
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	linux-kernel, linux-fscrypt, linux-f2fs-devel

On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
> On 2020/12/17 23:44, Satya Tangirala wrote:
> > On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
> > > Why not using nid as DUN, then GC could migrate encrypted node block directly via
> > > meta inode's address space like we do for encrypted data block, rather than
> > > decrypting node block to node page and then encrypting node page with DUN of new
> > > blkaddr it migrates to.
> > > 
> > The issue is, the bi_crypt_context in a bio holds a single DUN value,
> > which is the DUN for the first data unit in the bio. blk-crypto assumes
> > that the DUN of each subsequent data unit can be computed by simply
> > incrementing the DUN. So physically contiguous data units can only be put
> > into the same bio if they also have contiguous DUNs. I don't know much
> > about nids, but if the nid is invariant w.r.t the physical block location,
> > then there might be more fragmentation of bios in regular read/writes
> 
> Correct, considering performance of in batch node flush, it will be better to
> use pba as IV value.
> 
> But, what's the plan about supporting software encryption for metadata? Current
> f2fs write flow will handle all operations which may encounter failure before
> allocating block address for node, if we do allocation first, and then use pba
> as IV to encrypt node block, it will be a little complicated to revert allocation
> if we fail to encrypt node block.
> 
Software encryption for metadata is supported through the blk-crypto
framework - so encryption will happen in the block layer, not the
filesystem layer. So there's nothing extra/special we need to do if
there's an encryption failure - an encryption failure is no different
from a read/write failure in a lower layer from f2fs' perspective.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-12-18 11:53         ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-12-18 11:53 UTC (permalink / raw)
  To: Chao Yu
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	Eric Biggers, linux-fscrypt, Jaegeuk Kim

On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
> On 2020/12/17 23:44, Satya Tangirala wrote:
> > On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
> > > Why not using nid as DUN, then GC could migrate encrypted node block directly via
> > > meta inode's address space like we do for encrypted data block, rather than
> > > decrypting node block to node page and then encrypting node page with DUN of new
> > > blkaddr it migrates to.
> > > 
> > The issue is, the bi_crypt_context in a bio holds a single DUN value,
> > which is the DUN for the first data unit in the bio. blk-crypto assumes
> > that the DUN of each subsequent data unit can be computed by simply
> > incrementing the DUN. So physically contiguous data units can only be put
> > into the same bio if they also have contiguous DUNs. I don't know much
> > about nids, but if the nid is invariant w.r.t the physical block location,
> > then there might be more fragmentation of bios in regular read/writes
> 
> Correct, considering performance of in batch node flush, it will be better to
> use pba as IV value.
> 
> But, what's the plan about supporting software encryption for metadata? Current
> f2fs write flow will handle all operations which may encounter failure before
> allocating block address for node, if we do allocation first, and then use pba
> as IV to encrypt node block, it will be a little complicated to revert allocation
> if we fail to encrypt node block.
> 
Software encryption for metadata is supported through the blk-crypto
framework - so encryption will happen in the block layer, not the
filesystem layer. So there's nothing extra/special we need to do if
there's an encryption failure - an encryption failure is no different
from a read/write failure in a lower layer from f2fs' perspective.


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-12-18 11:53         ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-12-22 11:47           ` Chao Yu
  -1 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-12-22 11:47 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	linux-kernel, linux-fscrypt, linux-f2fs-devel

On 2020/12/18 19:53, Satya Tangirala wrote:
> On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
>> On 2020/12/17 23:44, Satya Tangirala wrote:
>>> On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
>>>> Why not using nid as DUN, then GC could migrate encrypted node block directly via
>>>> meta inode's address space like we do for encrypted data block, rather than
>>>> decrypting node block to node page and then encrypting node page with DUN of new
>>>> blkaddr it migrates to.
>>>>
>>> The issue is, the bi_crypt_context in a bio holds a single DUN value,
>>> which is the DUN for the first data unit in the bio. blk-crypto assumes
>>> that the DUN of each subsequent data unit can be computed by simply
>>> incrementing the DUN. So physically contiguous data units can only be put
>>> into the same bio if they also have contiguous DUNs. I don't know much
>>> about nids, but if the nid is invariant w.r.t the physical block location,
>>> then there might be more fragmentation of bios in regular read/writes
>>
>> Correct, considering performance of in batch node flush, it will be better to
>> use pba as IV value.
>>
>> But, what's the plan about supporting software encryption for metadata? Current
>> f2fs write flow will handle all operations which may encounter failure before
>> allocating block address for node, if we do allocation first, and then use pba
>> as IV to encrypt node block, it will be a little complicated to revert allocation
>> if we fail to encrypt node block.
>>
> Software encryption for metadata is supported through the blk-crypto

blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may
be encrypted twice?

And why not supporting hardware encryption for metadata in blk-crypto? then
both f2fs and ext4 can use inline-encryption based blk-crypto?

Thanks,

> framework - so encryption will happen in the block layer, not the
> filesystem layer. So there's nothing extra/special we need to do if
> there's an encryption failure - an encryption failure is no different
> from a read/write failure in a lower layer from f2fs' perspective.
> .
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-12-22 11:47           ` Chao Yu
  0 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-12-22 11:47 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	Eric Biggers, linux-fscrypt, Jaegeuk Kim

On 2020/12/18 19:53, Satya Tangirala wrote:
> On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
>> On 2020/12/17 23:44, Satya Tangirala wrote:
>>> On Sat, Oct 10, 2020 at 05:53:06PM +0800, Chao Yu wrote:
>>>> Why not using nid as DUN, then GC could migrate encrypted node block directly via
>>>> meta inode's address space like we do for encrypted data block, rather than
>>>> decrypting node block to node page and then encrypting node page with DUN of new
>>>> blkaddr it migrates to.
>>>>
>>> The issue is, the bi_crypt_context in a bio holds a single DUN value,
>>> which is the DUN for the first data unit in the bio. blk-crypto assumes
>>> that the DUN of each subsequent data unit can be computed by simply
>>> incrementing the DUN. So physically contiguous data units can only be put
>>> into the same bio if they also have contiguous DUNs. I don't know much
>>> about nids, but if the nid is invariant w.r.t the physical block location,
>>> then there might be more fragmentation of bios in regular read/writes
>>
>> Correct, considering performance of in batch node flush, it will be better to
>> use pba as IV value.
>>
>> But, what's the plan about supporting software encryption for metadata? Current
>> f2fs write flow will handle all operations which may encounter failure before
>> allocating block address for node, if we do allocation first, and then use pba
>> as IV to encrypt node block, it will be a little complicated to revert allocation
>> if we fail to encrypt node block.
>>
> Software encryption for metadata is supported through the blk-crypto

blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may
be encrypted twice?

And why not supporting hardware encryption for metadata in blk-crypto? then
both f2fs and ext4 can use inline-encryption based blk-crypto?

Thanks,

> framework - so encryption will happen in the block layer, not the
> filesystem layer. So there's nothing extra/special we need to do if
> there's an encryption failure - an encryption failure is no different
> from a read/write failure in a lower layer from f2fs' perspective.
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-12-22 11:47           ` [f2fs-dev] " Chao Yu
@ 2020-12-24 10:13             ` Satya Tangirala via Linux-f2fs-devel
  -1 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala @ 2020-12-24 10:13 UTC (permalink / raw)
  To: Chao Yu
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	linux-kernel, linux-fscrypt, linux-f2fs-devel

On Tue, Dec 22, 2020 at 07:47:45PM +0800, Chao Yu wrote:
> On 2020/12/18 19:53, Satya Tangirala wrote:
> > On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
> > > But, what's the plan about supporting software encryption for metadata? Current
> > > f2fs write flow will handle all operations which may encounter failure before
> > > allocating block address for node, if we do allocation first, and then use pba
> > > as IV to encrypt node block, it will be a little complicated to revert allocation
> > > if we fail to encrypt node block.
> > > 
> > Software encryption for metadata is supported through the blk-crypto
> 
> blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may
> be encrypted twice?
blk-crypto will only encrypt bios as directed to do so by the encryption
context set on the bio. That encryption context is constructed by the
submitter of the bio - in our case, the submitter is the filesystem.
So the filesystem decides which bio gets encrypted with
which key/algorithm/etc (and in the current implementation, each bio
only supports a single bi_crypt_context, so *only one* layer of
encryption is possible with blk-crypto anyway). So no, data won't be
encrypted twice, because F2FS/fscrypt ensure that it does not (and the
filesystem knows exactly which blocks need metadata encryption, and
which blocks need FBE).
> 
> And why not supporting hardware encryption for metadata in blk-crypto? then
> both f2fs and ext4 can use inline-encryption based blk-crypto?
>
I may be misunderstanding what you're asking, but I think you're asking
why not make blk-crypto do metadata encryption (without explicit
involvement from filesystems)? Or more generally, why not do metadata
encryption below the filesystem layer?

As mentioned above, the filesystem is what knows which blocks need to be
metadata encrypted and which blocks need to be FBE encrypted (or even
just read without any encryption at all) - the block layer doesn't have
this information, and so can't effectively decide which blocks to use
the metadata encryption key on. Fwiw, Android does take a somewhat
similar approach to what you're suggesting here (I explain more in
detail in the cover letter for v2 of this patch series at
https://lore.kernel.org/linux-fscrypt/20201217150435.1505269-1-satyat@google.com/
). In Android, we have a new DM target (called dm-default-key) that adds
an encryption context to any bio that doesn't already have an encryption
context - so the assumption in general is that if the filesystem wants to
use an FBE key, it would have already set the encryption context on the
bio, so if a bio reaches dm-default-key without an encryption context,
it must mean that it needs metadata encryption. However, that assumption
doesn't always hold because F2FS sometimes needs to read the ciphertext
of FBE files without having the file's FBE key available - in those
situations, F2FS will send a bio without any encryption context, but
will also tell dm-default-key to *not* add the metadata encryption
context. That's a layering violation, which is why I'm not using that
approach here.

Does that answer your question? Or am I misunderstanding what you're
asking?
> Thanks,
> 
> > framework - so encryption will happen in the block layer, not the
> > filesystem layer. So there's nothing extra/special we need to do if
> > there's an encryption failure - an encryption failure is no different
> > from a read/write failure in a lower layer from f2fs' perspective.
> > .
> > 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-12-24 10:13             ` Satya Tangirala via Linux-f2fs-devel
  0 siblings, 0 replies; 42+ messages in thread
From: Satya Tangirala via Linux-f2fs-devel @ 2020-12-24 10:13 UTC (permalink / raw)
  To: Chao Yu
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	Eric Biggers, linux-fscrypt, Jaegeuk Kim

On Tue, Dec 22, 2020 at 07:47:45PM +0800, Chao Yu wrote:
> On 2020/12/18 19:53, Satya Tangirala wrote:
> > On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
> > > But, what's the plan about supporting software encryption for metadata? Current
> > > f2fs write flow will handle all operations which may encounter failure before
> > > allocating block address for node, if we do allocation first, and then use pba
> > > as IV to encrypt node block, it will be a little complicated to revert allocation
> > > if we fail to encrypt node block.
> > > 
> > Software encryption for metadata is supported through the blk-crypto
> 
> blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may
> be encrypted twice?
blk-crypto will only encrypt bios as directed to do so by the encryption
context set on the bio. That encryption context is constructed by the
submitter of the bio - in our case, the submitter is the filesystem.
So the filesystem decides which bio gets encrypted with
which key/algorithm/etc (and in the current implementation, each bio
only supports a single bi_crypt_context, so *only one* layer of
encryption is possible with blk-crypto anyway). So no, data won't be
encrypted twice, because F2FS/fscrypt ensure that it does not (and the
filesystem knows exactly which blocks need metadata encryption, and
which blocks need FBE).
> 
> And why not supporting hardware encryption for metadata in blk-crypto? then
> both f2fs and ext4 can use inline-encryption based blk-crypto?
>
I may be misunderstanding what you're asking, but I think you're asking
why not make blk-crypto do metadata encryption (without explicit
involvement from filesystems)? Or more generally, why not do metadata
encryption below the filesystem layer?

As mentioned above, the filesystem is what knows which blocks need to be
metadata encrypted and which blocks need to be FBE encrypted (or even
just read without any encryption at all) - the block layer doesn't have
this information, and so can't effectively decide which blocks to use
the metadata encryption key on. Fwiw, Android does take a somewhat
similar approach to what you're suggesting here (I explain more in
detail in the cover letter for v2 of this patch series at
https://lore.kernel.org/linux-fscrypt/20201217150435.1505269-1-satyat@google.com/
). In Android, we have a new DM target (called dm-default-key) that adds
an encryption context to any bio that doesn't already have an encryption
context - so the assumption in general is that if the filesystem wants to
use an FBE key, it would have already set the encryption context on the
bio, so if a bio reaches dm-default-key without an encryption context,
it must mean that it needs metadata encryption. However, that assumption
doesn't always hold because F2FS sometimes needs to read the ciphertext
of FBE files without having the file's FBE key available - in those
situations, F2FS will send a bio without any encryption context, but
will also tell dm-default-key to *not* add the metadata encryption
context. That's a layering violation, which is why I'm not using that
approach here.

Does that answer your question? Or am I misunderstanding what you're
asking?
> Thanks,
> 
> > framework - so encryption will happen in the block layer, not the
> > filesystem layer. So there's nothing extra/special we need to do if
> > there's an encryption failure - an encryption failure is no different
> > from a read/write failure in a lower layer from f2fs' perspective.
> > .
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [PATCH 0/3] add support for metadata encryption to F2FS
  2020-12-24 10:13             ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
@ 2020-12-25  9:31               ` Chao Yu
  -1 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-12-25  9:31 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, Jaegeuk Kim, Eric Biggers, Chao Yu,
	linux-kernel, linux-fscrypt, linux-f2fs-devel

On 2020/12/24 18:13, Satya Tangirala wrote:
> On Tue, Dec 22, 2020 at 07:47:45PM +0800, Chao Yu wrote:
>> On 2020/12/18 19:53, Satya Tangirala wrote:
>>> On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
>>>> But, what's the plan about supporting software encryption for metadata? Current
>>>> f2fs write flow will handle all operations which may encounter failure before
>>>> allocating block address for node, if we do allocation first, and then use pba
>>>> as IV to encrypt node block, it will be a little complicated to revert allocation
>>>> if we fail to encrypt node block.
>>>>
>>> Software encryption for metadata is supported through the blk-crypto
>>
>> blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may
>> be encrypted twice?
> blk-crypto will only encrypt bios as directed to do so by the encryption
> context set on the bio. That encryption context is constructed by the
> submitter of the bio - in our case, the submitter is the filesystem.
> So the filesystem decides which bio gets encrypted with
> which key/algorithm/etc (and in the current implementation, each bio
> only supports a single bi_crypt_context, so *only one* layer of
> encryption is possible with blk-crypto anyway). So no, data won't be
> encrypted twice, because F2FS/fscrypt ensure that it does not (and the
> filesystem knows exactly which blocks need metadata encryption, and
> which blocks need FBE).

Oh, sorry, I misunderstand blk-crypto as dm-crypt...

So once hardware encryption is absent, blk-crypto will use blk-crypto-fallback
to encrypt bio data with software crypto, I see.

>>
>> And why not supporting hardware encryption for metadata in blk-crypto? then
>> both f2fs and ext4 can use inline-encryption based blk-crypto?
>>
> I may be misunderstanding what you're asking, but I think you're asking
> why not make blk-crypto do metadata encryption (without explicit
> involvement from filesystems)? Or more generally, why not do metadata
> encryption below the filesystem layer?

Yes.

> 
> As mentioned above, the filesystem is what knows which blocks need to be
> metadata encrypted and which blocks need to be FBE encrypted (or even
> just read without any encryption at all) - the block layer doesn't have
> this information, and so can't effectively decide which blocks to use
> the metadata encryption key on. Fwiw, Android does take a somewhat
> similar approach to what you're suggesting here (I explain more in
> detail in the cover letter for v2 of this patch series at
> https://lore.kernel.org/linux-fscrypt/20201217150435.1505269-1-satyat@google.com/

Ah, thanks for all your detailed explanation, now, I can see the
context.

> ). In Android, we have a new DM target (called dm-default-key) that adds
> an encryption context to any bio that doesn't already have an encryption
> context - so the assumption in general is that if the filesystem wants to
> use an FBE key, it would have already set the encryption context on the
> bio, so if a bio reaches dm-default-key without an encryption context,
> it must mean that it needs metadata encryption. However, that assumption
> doesn't always hold because F2FS sometimes needs to read the ciphertext
> of FBE files without having the file's FBE key available - in those
> situations, F2FS will send a bio without any encryption context, but
> will also tell dm-default-key to *not* add the metadata encryption
> context. That's a layering violation, which is why I'm not using that
> approach here.
> 
> Does that answer your question? Or am I misunderstanding what you're
> asking?

Yup, thank you!

Thanks,

>> Thanks,
>>
>>> framework - so encryption will happen in the block layer, not the
>>> filesystem layer. So there's nothing extra/special we need to do if
>>> there's an encryption failure - an encryption failure is no different
>>> from a read/write failure in a lower layer from f2fs' perspective.
>>> .
>>>
> .
> 

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [f2fs-dev] [PATCH 0/3] add support for metadata encryption to F2FS
@ 2020-12-25  9:31               ` Chao Yu
  0 siblings, 0 replies; 42+ messages in thread
From: Chao Yu @ 2020-12-25  9:31 UTC (permalink / raw)
  To: Satya Tangirala
  Cc: Theodore Y . Ts'o, linux-kernel, linux-f2fs-devel,
	Eric Biggers, linux-fscrypt, Jaegeuk Kim

On 2020/12/24 18:13, Satya Tangirala wrote:
> On Tue, Dec 22, 2020 at 07:47:45PM +0800, Chao Yu wrote:
>> On 2020/12/18 19:53, Satya Tangirala wrote:
>>> On Fri, Dec 18, 2020 at 05:02:23PM +0800, Chao Yu wrote:
>>>> But, what's the plan about supporting software encryption for metadata? Current
>>>> f2fs write flow will handle all operations which may encounter failure before
>>>> allocating block address for node, if we do allocation first, and then use pba
>>>> as IV to encrypt node block, it will be a little complicated to revert allocation
>>>> if we fail to encrypt node block.
>>>>
>>> Software encryption for metadata is supported through the blk-crypto
>>
>> blk-crypto will encrypt all data in filesystem, if FBE is enabled, data may
>> be encrypted twice?
> blk-crypto will only encrypt bios as directed to do so by the encryption
> context set on the bio. That encryption context is constructed by the
> submitter of the bio - in our case, the submitter is the filesystem.
> So the filesystem decides which bio gets encrypted with
> which key/algorithm/etc (and in the current implementation, each bio
> only supports a single bi_crypt_context, so *only one* layer of
> encryption is possible with blk-crypto anyway). So no, data won't be
> encrypted twice, because F2FS/fscrypt ensure that it does not (and the
> filesystem knows exactly which blocks need metadata encryption, and
> which blocks need FBE).

Oh, sorry, I misunderstand blk-crypto as dm-crypt...

So once hardware encryption is absent, blk-crypto will use blk-crypto-fallback
to encrypt bio data with software crypto, I see.

>>
>> And why not supporting hardware encryption for metadata in blk-crypto? then
>> both f2fs and ext4 can use inline-encryption based blk-crypto?
>>
> I may be misunderstanding what you're asking, but I think you're asking
> why not make blk-crypto do metadata encryption (without explicit
> involvement from filesystems)? Or more generally, why not do metadata
> encryption below the filesystem layer?

Yes.

> 
> As mentioned above, the filesystem is what knows which blocks need to be
> metadata encrypted and which blocks need to be FBE encrypted (or even
> just read without any encryption at all) - the block layer doesn't have
> this information, and so can't effectively decide which blocks to use
> the metadata encryption key on. Fwiw, Android does take a somewhat
> similar approach to what you're suggesting here (I explain more in
> detail in the cover letter for v2 of this patch series at
> https://lore.kernel.org/linux-fscrypt/20201217150435.1505269-1-satyat@google.com/

Ah, thanks for all your detailed explanation, now, I can see the
context.

> ). In Android, we have a new DM target (called dm-default-key) that adds
> an encryption context to any bio that doesn't already have an encryption
> context - so the assumption in general is that if the filesystem wants to
> use an FBE key, it would have already set the encryption context on the
> bio, so if a bio reaches dm-default-key without an encryption context,
> it must mean that it needs metadata encryption. However, that assumption
> doesn't always hold because F2FS sometimes needs to read the ciphertext
> of FBE files without having the file's FBE key available - in those
> situations, F2FS will send a bio without any encryption context, but
> will also tell dm-default-key to *not* add the metadata encryption
> context. That's a layering violation, which is why I'm not using that
> approach here.
> 
> Does that answer your question? Or am I misunderstanding what you're
> asking?

Yup, thank you!

Thanks,

>> Thanks,
>>
>>> framework - so encryption will happen in the block layer, not the
>>> filesystem layer. So there's nothing extra/special we need to do if
>>> there's an encryption failure - an encryption failure is no different
>>> from a read/write failure in a lower layer from f2fs' perspective.
>>> .
>>>
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2020-12-25  9:32 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05  7:36 [PATCH 0/3] add support for metadata encryption to F2FS Satya Tangirala
2020-10-05  7:36 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-05  7:36 ` [PATCH 1/3] fscrypt, f2fs: replace fscrypt_get_devices with fscrypt_get_device Satya Tangirala
2020-10-05  7:36   ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-05  7:36 ` [PATCH 2/3] fscrypt: Add metadata encryption support Satya Tangirala
2020-10-05  7:36   ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-07 20:52   ` Eric Biggers
2020-10-07 20:52     ` [f2fs-dev] " Eric Biggers
2020-10-07 23:28     ` Satya Tangirala
2020-10-07 23:28       ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-08 17:05       ` Eric Biggers
2020-10-08 17:05         ` [f2fs-dev] " Eric Biggers
2020-10-05  7:36 ` [PATCH 3/3] f2fs: " Satya Tangirala
2020-10-05  7:36   ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-05 10:19   ` kernel test robot
2020-10-05 10:19     ` kernel test robot
2020-10-07 21:20   ` Eric Biggers
2020-10-07 21:20     ` [f2fs-dev] " Eric Biggers
2020-10-08  0:31     ` Satya Tangirala
2020-10-08  0:31       ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-05  7:43 ` [PATCH 0/3] add support for metadata encryption to F2FS Satya Tangirala
2020-10-05  7:43   ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-07 21:00 ` Eric Biggers
2020-10-07 21:00   ` [f2fs-dev] " Eric Biggers
2020-10-07 22:05   ` Satya Tangirala
2020-10-07 22:05     ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-10-08 17:01     ` Eric Biggers
2020-10-08 17:01       ` [f2fs-dev] " Eric Biggers
2020-10-10  9:53 ` Chao Yu
2020-10-10  9:53   ` [f2fs-dev] " Chao Yu
2020-12-17 15:44   ` Satya Tangirala
2020-12-17 15:44     ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-12-18  9:02     ` Chao Yu
2020-12-18  9:02       ` [f2fs-dev] " Chao Yu
2020-12-18 11:53       ` Satya Tangirala
2020-12-18 11:53         ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-12-22 11:47         ` Chao Yu
2020-12-22 11:47           ` [f2fs-dev] " Chao Yu
2020-12-24 10:13           ` Satya Tangirala
2020-12-24 10:13             ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
2020-12-25  9:31             ` Chao Yu
2020-12-25  9:31               ` [f2fs-dev] " Chao Yu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.