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
