Re: [f2fs-dev] [PATCH 1/1] f2fs-tools: Introduce metadata encryption support

[Satya Tangirala via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

On Wed, Oct 07, 2020 at 12:42:09PM -0700, jaegeuk@kernel.org wrote:
> Hi Satya,
> 
> On 10/05, Satya Tangirala wrote:
> > This patch introduces two new options '-A' and '-M' for specifying metadata
> > crypt options. '-A' takes the desired metadata encryption algorithm as
> > argument. '-M' takes the linux key_serial of the metadata encryption key as
> > the argument. The keyring key provided must be of a key type that supports
> > reading the payload from userspace.
> 
> Could you please update manpages as well?
> 
Done
> > diff --git a/lib/f2fs_metadata_crypt.c b/lib/f2fs_metadata_crypt.c
> > new file mode 100644
> > index 0000000..faf399a
> > --- /dev/null
> > +++ b/lib/f2fs_metadata_crypt.c
> > @@ -0,0 +1,226 @@
> > +/**
> > + * f2fs_metadata_crypt.c
> > + *
> > + * Copyright (c) 2020 Google LLC
> > + *
> > + * Dual licensed under the GPL or LGPL version 2 licenses.
> > + */
> > +#include <string.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <sys/socket.h>
> > +#include <linux/if_alg.h>
> > +#include <linux/socket.h>
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <keyutils.h>
> > +
> > +#include "f2fs_fs.h"
> > +#include "f2fs_metadata_crypt.h"
> > +
> > +extern struct f2fs_configuration c;
> > +struct f2fs_crypt_mode {
> > +	const char *friendly_name;
> > +	const char *cipher_str;
> > +	unsigned int keysize;
> > +	unsigned int ivlen;
> > +} f2fs_crypt_modes[] = {
> > +	[FSCRYPT_MODE_AES_256_XTS] = {
> > +		.friendly_name = "AES-256-XTS",
> > +		.cipher_str = "xts(aes)",
> > +		.keysize = 64,
> > +		.ivlen = 16,
> > +	},
> > +	[FSCRYPT_MODE_ADIANTUM] = {
> > +		.friendly_name = "Adiantum",
> > +		.cipher_str = "adiantum(xchacha12,aes)",
> > +		.keysize = 32,
> > +		.ivlen = 32,
> > +	},
> > +};
> > +#define MAX_IV_LEN 32
> > +
> > +void f2fs_print_crypt_algs(void)
> > +{
> > +	int i;
> > +
> > +	for (i = 1; i <= __FSCRYPT_MODE_MAX; i++) {
> > +		if (!f2fs_crypt_modes[i].friendly_name)
> > +			continue;
> > +		MSG(0, "\t%s\n", f2fs_crypt_modes[i].friendly_name);
> > +	}
> > +}
> > +
> > +int f2fs_get_crypt_alg(const char *optarg)
> > +{
> > +	int i;
> > +
> > +	for (i = 1; i <= __FSCRYPT_MODE_MAX; i++) {
> > +		if (f2fs_crypt_modes[i].friendly_name &&
> > +		    !strcmp(f2fs_crypt_modes[i].friendly_name, optarg)) {
> > +			return i;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int f2fs_metadata_process_key(const char *key_serial_str)
> > +{
> > +	key_serial_t key_serial = strtol(key_serial_str, NULL, 10);
> > +
> > +	c.metadata_crypt_key_len =
> > +		keyctl_read_alloc(key_serial, (void **)&c.metadata_crypt_key);
> > +
> > +	if (c.metadata_crypt_key_len < 0)
> > +		return errno;
> > +
> > +	return 0;
> > +}
> > +
> > +int f2fs_metadata_verify_args(void)
> > +{
> > +	/* If neither specified, nothing to do */
> > +	if (!c.metadata_crypt_key && !c.metadata_crypt_alg)
> > +		return 0;
> > +
> > +	/* We need both specified */
> > +	if (!c.metadata_crypt_key || !c.metadata_crypt_alg)
> > +		return -EINVAL;
> > +
> > +	if (c.metadata_crypt_key_len !=
> > +	    f2fs_crypt_modes[c.metadata_crypt_alg].keysize) {
> > +		MSG(0, "\tMetadata encryption key length %d didn't match required size %d\n",
> > +		    c.metadata_crypt_key_len,
> > +		    f2fs_crypt_modes[c.metadata_crypt_alg].keysize);
> > +
> > +		return -EINVAL;
> > +	}
> 
> Need to check sparse mode here?
> 
I tried to support sparse mode with metadata encryption in v2 (that I
just sent out), but I haven't been able to even compile or test it yet.
Would you happen to know where I might find some info on how to compile
and test sparse mode?
> And, what about multiple partition case?
> 
IIUC I think multiple devices are handled correctly by the code - is there
something I'm missing?
> > @@ -138,6 +147,14 @@ static void f2fs_parse_options(int argc, char *argv[])
> >  		case 'a':
> >  			c.heap = atoi(optarg);
> >  			break;
> > +		case 'A':
> > +			c.metadata_crypt_alg = f2fs_get_crypt_alg(optarg);
> > +			if (c.metadata_crypt_alg < 0) {
> > +				MSG(0, "Error: invalid crypt algorithm specified. The choices are:");
> > +				f2fs_print_crypt_algs();
> > +				exit(1);
> > +			}
> > +			break;
> >  		case 'c':
> >  			if (c.ndevs >= MAX_DEVICES) {
> >  				MSG(0, "Error: Too many devices\n");
> > @@ -178,6 +195,12 @@ static void f2fs_parse_options(int argc, char *argv[])
> >  		case 'm':
> >  			c.zoned_mode = 1;
> >  			break;
> > +		case 'M':
> > +			if (f2fs_metadata_process_key(optarg)) {
> > +				MSG(0, "Error: Invalid metadata key\n");
> > +				mkfs_usage();
> > +			}
> > +			break;
> >  		case 'o':
> >  			c.overprovision = atof(optarg);
> >  			break;
> > @@ -244,6 +267,14 @@ static void f2fs_parse_options(int argc, char *argv[])
> >  		}
> >  	}
> >  
> > +	if ((!!c.metadata_crypt_key) != (!!c.metadata_crypt_alg)) {
> > +		MSG(0, "\tError: Both the metadata crypt key and crypt algorithm must be specified!");
> > +		exit(1);
> > +	}
> > +
> > +	if (f2fs_metadata_verify_args())
> > +		exit(1);
> > +
> >  	add_default_options();
> 
> Need to check options after add_default_options()?
> 
As in, you're suggesting moving the metadata_crypt_key and
metadata_crypt_alg check and the 
+ if (f2fs_metadata_verify_args())
to below the add_default_options() call? If so, I'll do that in v3 of
this patch series
> Thanks,
> 
> >  
> >  	if (!(c.feature & cpu_to_le32(F2FS_FEATURE_EXTRA_ATTR))) {
> > -- 
> > 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