diff mbox series

[3/3] f2fs: Add metadata encryption support

Message ID 20201005073606.1949772-4-satyat@google.com
State New
Headers show
Series add support for metadata encryption to F2FS | expand

Commit Message

Satya Tangirala Oct. 5, 2020, 7:36 a.m. UTC
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(-)

Comments

kernel test robot Oct. 5, 2020, 10:19 a.m. UTC | #1
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
Eric Biggers Oct. 7, 2020, 9:20 p.m. UTC | #2
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
Satya Tangirala Oct. 8, 2020, 12:31 a.m. UTC | #3
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
diff mbox series

Patch

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;