Re: [PATCH 5/5] dm verity: add support for version 0 of the on-disk format

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 11/04/17 19:21, Milan Broz wrote:
> On 04/11/2017 05:33 PM, Enric Balletbo i Serra wrote:
>> From: Will Drewry <wad@xxxxxxxxxxxx>
>>
>> Version 0 of the on-disk format is compatible with the format used in the
>> Chromium OS. This patch adds support for this version.
>>
>> Format type 0 is the original Chrome OS version, whereas the format type 1
>> is current version, but 0, the original format used in the Chromium OS is
>> still used in most devices. This patch adds support for the original
>> on-disk format so you can decide which want you want to use.
> 
> NACK!
> 
> What the ...  is this patch doing?
> 

Argh! Very sorry about that was *not* my intention add this patch in this
series, I was only trying to submit 1 to 4 which contains the patches already
send long time ago and the patches to move to the public header some functions
needed for init/do_mount_dm.c

I'm using this specific patch to test 1 to 4 and to boot against a chromeos
rootfs as it parses the current chromeos args, this patch also has the use of
some deprecated functions like simple_strtoull, so definitely this *shouldn't*
be here, my bad.

Besides that, with the below comments, you have made me see that I have some
lacks and I need to rethink some things that I wanted to send after this series,
so many thanks and sorry again if you spend some time looking at this.

> Isn't the format 0 already there? According to
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/device-mapper/verity.txt
> 
> <version>
>     This is the type of the on-disk hash format.
> 
>     0 is the original format used in the Chromium OS.
>       The salt is appended when hashing, digests are stored continuously and
>       the rest of the block is padded with zeroes.
> 
> Reading this patch, it does something completely different than it describes
> in header.
> 
> How is superblock format related to:
> +MODULE_PARM_DESC(dev_wait, "Wait forever for a backing device");
> ...
> not mentioning complete rewrite of verity table parsing... why?
> 
> Also please note that there is userspace (veritysetup) that have to work with
> the old format as well (I think it does already).
> I think we solved the compatibility years ago.
> 
> If not, please describe properly what is missing.
> I do not see any on-disk format changes here...
> 
> Milan
> 
>>
>> Signed-off-by: Will Drewry <wad@xxxxxxxxxxxx>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>> ---
>>  drivers/md/dm-verity-target.c | 279 ++++++++++++++++++++++++++++++++----------
>>  init/do_mounts_dm.c           |  16 +--
>>  2 files changed, 220 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>> index 7335d8a..c098d22 100644
>> --- a/drivers/md/dm-verity-target.c
>> +++ b/drivers/md/dm-verity-target.c
>> @@ -17,6 +17,7 @@
>>  #include "dm-verity.h"
>>  #include "dm-verity-fec.h"
>>  
>> +#include <linux/delay.h>
>>  #include <linux/module.h>
>>  #include <linux/reboot.h>
>>  
>> @@ -28,6 +29,7 @@
>>  #define DM_VERITY_DEFAULT_PREFETCH_SIZE	262144
>>  
>>  #define DM_VERITY_MAX_CORRUPTED_ERRS	100
>> +#define DM_VERITY_NUM_POSITIONAL_ARGS	10
>>  
>>  #define DM_VERITY_OPT_LOGGING		"ignore_corruption"
>>  #define DM_VERITY_OPT_RESTART		"restart_on_corruption"
>> @@ -46,6 +48,11 @@ struct dm_verity_prefetch_work {
>>  	unsigned n_blocks;
>>  };
>>  
>> +/* Controls whether verity_get_device will wait forever for a device. */
>> +static int dev_wait;
>> +module_param(dev_wait, int, 0444);
>> +MODULE_PARM_DESC(dev_wait, "Wait forever for a backing device");
>> +
>>  /*
>>   * Auxiliary structure appended to each dm-bufio buffer. If the value
>>   * hash_verified is nonzero, hash of the block has been verified.
>> @@ -803,6 +810,183 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
>>  	return r;
>>  }
>>  
>> +static int verity_get_device(struct dm_target *ti, const char *devname,
>> +			     struct dm_dev **dm_dev)
>> +{
>> +	do {
>> +		/* Try the normal path first since if everything is ready, it
>> +		 * will be the fastest.
>> +		 */
>> +		if (!dm_get_device(ti, devname, /*FMODE_READ*/
>> +				   dm_table_get_mode(ti->table), dm_dev))
>> +			return 0;
>> +
>> +		if (!dev_wait)
>> +			break;
>> +
>> +		/* No need to be too aggressive since this is a slow path. */
>> +		msleep(500);
>> +	} while (driver_probe_done() != 0 || !(*dm_dev));
>> +	return -1;
>> +}
>> +
>> +struct verity_args {
>> +	int version;
>> +	char *data_device;
>> +	char *hash_device;
>> +	int data_block_size_bits;
>> +	int hash_block_size_bits;
>> +	u64 num_data_blocks;
>> +	u64 hash_start_block;
>> +	char *algorithm;
>> +	char *digest;
>> +	char *salt;
>> +};
>> +
>> +static void pr_args(struct verity_args *args)
>> +{
>> +	printk(KERN_INFO "VERITY args: version=%d data_device=%s hash_device=%s"
>> +		" data_block_size_bits=%d hash_block_size_bits=%d"
>> +		" num_data_blocks=%lld hash_start_block=%lld"
>> +		" algorithm=%s digest=%s salt=%s\n",
>> +		args->version,
>> +		args->data_device,
>> +		args->hash_device,
>> +		args->data_block_size_bits,
>> +		args->hash_block_size_bits,
>> +		args->num_data_blocks,
>> +		args->hash_start_block,
>> +		args->algorithm,
>> +		args->digest,
>> +		args->salt);
>> +}
>> +
>> +/*
>> + * positional_args - collects the argments using the positional
>> + * parameters.
>> + * arg# - parameter
>> + *    0 - version
>> + *    1 - data device
>> + *    2 - hash device - may be same as data device
>> + *    3 - data block size log2
>> + *    4 - hash block size log2
>> + *    5 - number of data blocks
>> + *    6 - hash start block
>> + *    7 - algorithm
>> + *    8 - digest
>> + *    9 - salt
>> + */
>> +static char *positional_args(unsigned argc, char **argv,
>> +			     struct verity_args *args)
>> +{
>> +	unsigned int num;
>> +	unsigned long long num_ll;
>> +	char dummy;
>> +
>> +	if (argc < DM_VERITY_NUM_POSITIONAL_ARGS)
>> +		return "Invalid argument count: at least 10 arguments required";
>> +
>> +	if (sscanf(argv[0], "%d%c", &num, &dummy) != 1 ||
>> +	    num < 0 || num > 1)
>> +		return "Invalid version";
>> +	args->version = num;
>> +
>> +	args->data_device = argv[1];
>> +	args->hash_device = argv[2];
>> +
>> +	if (sscanf(argv[3], "%u%c", &num, &dummy) != 1 ||
>> +	    !num || (num & (num - 1)) ||
>> +	    num > PAGE_SIZE)
>> +		return "Invalid data device block size";
>> +	args->data_block_size_bits = ffs(num) - 1;
>> +
>> +	if (sscanf(argv[4], "%u%c", &num, &dummy) != 1 ||
>> +	    !num || (num & (num - 1)) ||
>> +	    num > INT_MAX)
>> +		return "Invalid hash device block size";
>> +	args->hash_block_size_bits = ffs(num) - 1;
>> +
>> +	if (sscanf(argv[5], "%llu%c", &num_ll, &dummy) != 1 ||
>> +	    (sector_t)(num_ll << (args->data_block_size_bits - SECTOR_SHIFT))
>> +	    >> (args->data_block_size_bits - SECTOR_SHIFT) != num_ll)
>> +		return "Invalid data blocks";
>> +	args->num_data_blocks = num_ll;
>> +
>> +	if (sscanf(argv[6], "%llu%c", &num_ll, &dummy) != 1 ||
>> +	    (sector_t)(num_ll << (args->hash_block_size_bits - SECTOR_SHIFT))
>> +	    >> (args->hash_block_size_bits - SECTOR_SHIFT) != num_ll)
>> +		return "Invalid hash start";
>> +	args->hash_start_block = num_ll;
>> +
>> +	args->algorithm = argv[7];
>> +	args->digest = argv[8];
>> +	args->salt = argv[9];
>> +
>> +	return NULL;
>> +}
>> +
>> +static void splitarg(char *arg, char **key, char **val)
>> +{
>> +	*key = strsep(&arg, "=");
>> +	*val = strsep(&arg, "");
>> +}
>> +
>> +static char *chromeos_args(unsigned int argc, char **argv,
>> +			   struct verity_args *args)
>> +{
>> +	char *key, *val;
>> +	unsigned long num;
>> +	int i;
>> +
>> +	args->version = 0;
>> +	args->data_block_size_bits = 12;
>> +	args->hash_block_size_bits = 12;
>> +	for (i = 0; i < argc; ++i) {
>> +		DMWARN("Argument %d: '%s'", i, argv[i]);
>> +		splitarg(argv[i], &key, &val);
>> +		if (!key) {
>> +			DMWARN("Bad argument %d: missing key?", i);
>> +			return "Bad argument: missing key";
>> +		}
>> +		if (!val) {
>> +			DMWARN("Bad argument %d='%s': missing value", i, key);
>> +			return "Bad argument: missing value";
>> +		}
>> +		if (!strcmp(key, "alg")) {
>> +			args->algorithm = val;
>> +		} else if (!strcmp(key, "payload")) {
>> +			args->data_device = val;
>> +		} else if (!strcmp(key, "hashtree")) {
>> +			args->hash_device = val;
>> +		} else if (!strcmp(key, "root_hexdigest")) {
>> +			args->digest = val;
>> +		} else if (!strcmp(key, "hashstart")) {
>> +			if (kstrtoul(val, 10, &num))
>> +				return "Invalid hashstart";
>> +			args->hash_start_block =
>> +			   num >> (args->hash_block_size_bits - SECTOR_SHIFT);
>> +			args->num_data_blocks = args->hash_start_block;
>> +		} else if (!strcmp(key, "salt")) {
>> +			args->salt = val;
>> +		}
>> +	}
>> +	if (!args->salt)
>> +		args->salt = "";
>> +
>> +#define NEEDARG(n) do { \
>> +	if (!(n)) { \
>> +		return "Missing argument: " #n; \
>> +	} } while (0)
>> +
>> +	NEEDARG(args->algorithm);
>> +	NEEDARG(args->data_device);
>> +	NEEDARG(args->hash_device);
>> +	NEEDARG(args->digest);
>> +
>> +#undef NEEDARG
>> +	return NULL;
>> +}
>> +
>>  /*
>>   * Target parameters:
>>   *	<version>	The current format is version 1.
>> @@ -819,14 +1003,21 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v)
>>   */
>>  static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
>>  {
>> +	struct verity_args args = { 0 };
>>  	struct dm_verity *v;
>>  	struct dm_arg_set as;
>> -	unsigned int num;
>> -	unsigned long long num_ll;
>>  	int r;
>>  	int i;
>>  	sector_t hash_position;
>> -	char dummy;
>> +
>> +	if (argc >= DM_VERITY_NUM_POSITIONAL_ARGS)
>> +		ti->error = positional_args(argc, argv, &args);
>> +	else
>> +		ti->error = chromeos_args(argc, argv, &args);
>> +	if (ti->error)
>> +		return -EINVAL;
>> +	if (0)
>> +		pr_args(&args);
>>  
>>  	v = kzalloc(sizeof(struct dm_verity), GFP_KERNEL);
>>  	if (!v) {
>> @@ -840,83 +1031,46 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
>>  	if (r)
>>  		goto bad;
>>  
>> -	if ((dm_table_get_mode(ti->table) & ~FMODE_READ)) {
>> -		ti->error = "Device must be readonly";
>> -		r = -EINVAL;
>> -		goto bad;
>> -	}
>> +	v->version = args.version;
>>  
>> -	if (argc < 10) {
>> -		ti->error = "Not enough arguments";
>> -		r = -EINVAL;
>> -		goto bad;
>> -	}
>> -
>> -	if (sscanf(argv[0], "%u%c", &num, &dummy) != 1 ||
>> -	    num > 1) {
>> -		ti->error = "Invalid version";
>> -		r = -EINVAL;
>> -		goto bad;
>> -	}
>> -	v->version = num;
>> -
>> -	r = dm_get_device(ti, argv[1], FMODE_READ, &v->data_dev);
>> +	r = verity_get_device(ti, args.data_device, &v->data_dev);
>>  	if (r) {
>>  		ti->error = "Data device lookup failed";
>>  		goto bad;
>>  	}
>>  
>> -	r = dm_get_device(ti, argv[2], FMODE_READ, &v->hash_dev);
>> +	r = verity_get_device(ti, args.hash_device, &v->hash_dev);
>>  	if (r) {
>>  		ti->error = "Hash device lookup failed";
>>  		goto bad;
>>  	}
>>  
>> -	if (sscanf(argv[3], "%u%c", &num, &dummy) != 1 ||
>> -	    !num || (num & (num - 1)) ||
>> -	    num < bdev_logical_block_size(v->data_dev->bdev) ||
>> -	    num > PAGE_SIZE) {
>> +	v->data_dev_block_bits = args.data_block_size_bits;
>> +	if ((1 << v->data_dev_block_bits) <
>> +	    bdev_logical_block_size(v->data_dev->bdev)) {
>>  		ti->error = "Invalid data device block size";
>>  		r = -EINVAL;
>>  		goto bad;
>>  	}
>> -	v->data_dev_block_bits = __ffs(num);
>>  
>> -	if (sscanf(argv[4], "%u%c", &num, &dummy) != 1 ||
>> -	    !num || (num & (num - 1)) ||
>> -	    num < bdev_logical_block_size(v->hash_dev->bdev) ||
>> -	    num > INT_MAX) {
>> +	v->hash_dev_block_bits = args.hash_block_size_bits;
>> +	if ((1 << v->data_dev_block_bits) <
>> +	    bdev_logical_block_size(v->hash_dev->bdev)) {
>>  		ti->error = "Invalid hash device block size";
>>  		r = -EINVAL;
>>  		goto bad;
>>  	}
>> -	v->hash_dev_block_bits = __ffs(num);
>> -
>> -	if (sscanf(argv[5], "%llu%c", &num_ll, &dummy) != 1 ||
>> -	    (sector_t)(num_ll << (v->data_dev_block_bits - SECTOR_SHIFT))
>> -	    >> (v->data_dev_block_bits - SECTOR_SHIFT) != num_ll) {
>> -		ti->error = "Invalid data blocks";
>> -		r = -EINVAL;
>> -		goto bad;
>> -	}
>> -	v->data_blocks = num_ll;
>>  
>> +	v->data_blocks = args.num_data_blocks;
>>  	if (ti->len > (v->data_blocks << (v->data_dev_block_bits - SECTOR_SHIFT))) {
>>  		ti->error = "Data device is too small";
>>  		r = -EINVAL;
>>  		goto bad;
>>  	}
>>  
>> -	if (sscanf(argv[6], "%llu%c", &num_ll, &dummy) != 1 ||
>> -	    (sector_t)(num_ll << (v->hash_dev_block_bits - SECTOR_SHIFT))
>> -	    >> (v->hash_dev_block_bits - SECTOR_SHIFT) != num_ll) {
>> -		ti->error = "Invalid hash start";
>> -		r = -EINVAL;
>> -		goto bad;
>> -	}
>> -	v->hash_start = num_ll;
>> +	v->hash_start = args.hash_start_block;
>>  
>> -	v->alg_name = kstrdup(argv[7], GFP_KERNEL);
>> +	v->alg_name = kstrdup(args.algorithm, GFP_KERNEL);
>>  	if (!v->alg_name) {
>>  		ti->error = "Cannot allocate algorithm name";
>>  		r = -ENOMEM;
>> @@ -945,36 +1099,33 @@ static int verity_ctr(struct dm_target *ti, unsigned argc, char **argv)
>>  		r = -ENOMEM;
>>  		goto bad;
>>  	}
>> -	if (strlen(argv[8]) != v->digest_size * 2 ||
>> -	    hex2bin(v->root_digest, argv[8], v->digest_size)) {
>> +	if (strlen(args.digest) != v->digest_size * 2 ||
>> +	    hex2bin(v->root_digest, args.digest, v->digest_size)) {
>>  		ti->error = "Invalid root digest";
>>  		r = -EINVAL;
>>  		goto bad;
>>  	}
>>  
>> -	if (strcmp(argv[9], "-")) {
>> -		v->salt_size = strlen(argv[9]) / 2;
>> +	if (strcmp(args.salt, "-")) {
>> +		v->salt_size = strlen(args.salt) / 2;
>>  		v->salt = kmalloc(v->salt_size, GFP_KERNEL);
>>  		if (!v->salt) {
>>  			ti->error = "Cannot allocate salt";
>>  			r = -ENOMEM;
>>  			goto bad;
>>  		}
>> -		if (strlen(argv[9]) != v->salt_size * 2 ||
>> -		    hex2bin(v->salt, argv[9], v->salt_size)) {
>> +		if (strlen(args.salt) != v->salt_size * 2 ||
>> +		    hex2bin(v->salt, args.salt, v->salt_size)) {
>>  			ti->error = "Invalid salt";
>>  			r = -EINVAL;
>>  			goto bad;
>>  		}
>>  	}
>>  
>> -	argv += 10;
>> -	argc -= 10;
>> -
>>  	/* Optional parameters */
>> -	if (argc) {
>> -		as.argc = argc;
>> -		as.argv = argv;
>> +	if (argc > DM_VERITY_NUM_POSITIONAL_ARGS) {
>> +		as.argc = argc - DM_VERITY_NUM_POSITIONAL_ARGS;
>> +		as.argv = argv + DM_VERITY_NUM_POSITIONAL_ARGS;
>>  
>>  		r = verity_parse_opt_args(&as, v);
>>  		if (r < 0)
>> diff --git a/init/do_mounts_dm.c b/init/do_mounts_dm.c
>> index 54c3299..25a040e 100644
>> --- a/init/do_mounts_dm.c
>> +++ b/init/do_mounts_dm.c
>> @@ -187,8 +187,7 @@ static char * __init dm_parse_device(struct dm_device *dev, char *str)
>>  	if (opt.delim == DM_FIELD_SEP[0]) {
>>  		if (!get_dm_option(&opt, DM_LINE_SEP))
>>  			return NULL;
>> -		if (kstrtoul(opt.start, 10, &dev->num_targets))
>> -			dev->num_targets = 1;
>> +		dev->num_targets = simple_strtoul(opt.start, NULL, 10);
>>  	} else {
>>  		dev->num_targets = 1;
>>  	}
>> @@ -214,7 +213,7 @@ static char * __init dm_parse_targets(struct dm_device *dev, char *str)
>>  	 */
>>  	opt.next = str;
>>  	for (i = 0; i < num_targets; i++) {
>> -		*target = kzalloc(sizeof(*target), GFP_KERNEL);
>> +		*target = kzalloc(sizeof(struct dm_setup_target), GFP_KERNEL);
>>  		if (!*target) {
>>  			DMERR("failed to allocate memory for target %s<%ld>",
>>  			      dev->name, i);
>> @@ -227,18 +226,14 @@ static char * __init dm_parse_targets(struct dm_device *dev, char *str)
>>  			      " for target %s<%ld>", dev->name, i);
>>  			goto parse_fail;
>>  		}
>> -
>> -		if (kstrtoull(opt.start, 10, &(*target)->begin))
>> -			goto parse_fail;
>> +		(*target)->begin = simple_strtoull(opt.start, NULL, 10);
>>  
>>  		if (!get_dm_option(&opt, DM_FIELD_SEP)) {
>>  			DMERR("failed to parse length for target %s<%ld>",
>>  			      dev->name, i);
>>  			goto parse_fail;
>>  		}
>> -
>> -		if (kstrtoull(opt.start, 10, &(*target)->length))
>> -			goto parse_fail;
>> +		(*target)->length = simple_strtoull(opt.start, NULL, 10);
>>  
>>  		if (get_dm_option(&opt, DM_FIELD_SEP))
>>  			(*target)->type = kstrndup(opt.start, opt.len,
>> @@ -328,8 +323,7 @@ static int __init dm_setup(char *str)
>>  	if (!get_dm_option(&opt, DM_FIELD_SEP))
>>  		goto parse_fail;
>>  	if (isdigit(opt.start[0])) {	/* Optional number field */
>> -		if (kstrtoul(opt.start, 10, &num_devices))
>> -			num_devices = 1;
>> +		num_devices = simple_strtoul(opt.start, NULL, 10);
>>  		str = opt.next;
>>  	} else {
>>  		num_devices = 1;
>>
> 

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel



[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux