From 536879480a5ec4d4be10941aa837791ddd68edc2 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Tue, 14 Jul 2020 15:07:21 +0100 Subject: [PATCH 1/2] dm-util: use CRYPT_DEACTIVATE_DEFERRED instead of ioctl --- src/home/homework-luks.c | 2 +- src/shared/dissect-image.c | 2 +- src/shared/dm-util.c | 34 ---------------------------------- src/shared/dm-util.h | 2 -- 4 files changed, 2 insertions(+), 38 deletions(-) diff --git a/src/home/homework-luks.c b/src/home/homework-luks.c index 99cab0929e..ba47688b0f 100644 --- a/src/home/homework-luks.c +++ b/src/home/homework-luks.c @@ -1295,7 +1295,7 @@ int home_activate_luks( loop_device_relinquish(setup.loop); - r = dm_deferred_remove(setup.dm_name); + r = crypt_deactivate_by_name(NULL, setup.dm_name, CRYPT_DEACTIVATE_DEFERRED); if (r < 0) log_warning_errno(r, "Failed to relinquish DM device, ignoring: %m"); diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index fdf4d481f6..3641412dd1 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -1437,7 +1437,7 @@ int decrypted_image_relinquish(DecryptedImage *d) { if (p->relinquished) continue; - r = dm_deferred_remove(p->name); + r = crypt_deactivate_by_name(NULL, p->name, CRYPT_DEACTIVATE_DEFERRED); if (r < 0) return log_debug_errno(r, "Failed to mark %s for auto-removal: %m", p->name); diff --git a/src/shared/dm-util.c b/src/shared/dm-util.c index d817e5b0e5..9ffa427027 100644 --- a/src/shared/dm-util.c +++ b/src/shared/dm-util.c @@ -5,37 +5,3 @@ #include "dm-util.h" #include "fd-util.h" #include "string-util.h" - -int dm_deferred_remove(const char *name) { - - struct dm_ioctl dm = { - .version = { - DM_VERSION_MAJOR, - DM_VERSION_MINOR, - DM_VERSION_PATCHLEVEL - }, - .data_size = sizeof(dm), - .flags = DM_DEFERRED_REMOVE, - }; - - _cleanup_close_ int fd = -1; - - assert(name); - - /* Unfortunately, libcryptsetup doesn't provide a proper API for this, hence call the ioctl() - * directly. */ - - if (strlen(name) >= sizeof(dm.name)) - return -ENODEV; /* A device with a name longer than this cannot possibly exist */ - - fd = open("/dev/mapper/control", O_RDWR|O_CLOEXEC); - if (fd < 0) - return -errno; - - strncpy_exact(dm.name, name, sizeof(dm.name)); - - if (ioctl(fd, DM_DEV_REMOVE, &dm)) - return -errno; - - return 0; -} diff --git a/src/shared/dm-util.h b/src/shared/dm-util.h index 6c78bfe75b..3bae3d43cb 100644 --- a/src/shared/dm-util.h +++ b/src/shared/dm-util.h @@ -1,4 +1,2 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #pragma once - -int dm_deferred_remove(const char *name); From ac1f3ad05f7476ae58981dcba45dfeb2c0006824 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 8 Jul 2020 19:57:31 +0100 Subject: [PATCH 2/2] verity: re-use already open devices if the hashes match Opening a verity device is an expensive operation. The kernelspace operations are mostly sequential with a global lock held regardless of which device is being opened. In userspace jumps in and out of multiple libraries are required. When signatures are used, there's the additional cryptographic checks. We know when two devices are identical: they have the same root hash. If libcrypsetup returns EEXIST, double check that the hashes are really the same, and that either both or none have a signature, and if everything matches simply remount the already open device. The kernel will do reference counting for us. In order to quickly and reliably discover if a device is already open, change the node naming scheme from '/dev/mapper/major:minor-verity' to '/dev/mapper/$roothash-verity'. Unfortunately libdevmapper is not 100% reliable, so in some case it will say that the device already exists and it is active, but in reality it is not usable. Fallback to an individually-activated unique device name in those cases for robustness. --- src/shared/dissect-image.c | 149 ++++++++++++++++++++++++++++++------- src/shared/dissect-image.h | 1 + src/shared/dm-util.c | 36 +++++++++ src/shared/dm-util.h | 2 + test/units/testsuite-50.sh | 13 +++- 5 files changed, 175 insertions(+), 26 deletions(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 3641412dd1..24be6de6c5 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -1140,8 +1140,9 @@ static int make_dm_name_and_node(const void *original_node, const char *suffix, base = strrchr(original_node, '/'); if (!base) - return -EINVAL; - base++; + base = original_node; + else + base++; if (isempty(base)) return -EINVAL; @@ -1217,6 +1218,50 @@ static int decrypt_partition( return 0; } +static int verity_can_reuse(const void *root_hash, size_t root_hash_size, bool has_sig, const char *name, struct crypt_device **ret_cd) { + /* If the same volume was already open, check that the root hashes match, and reuse it if they do */ + _cleanup_free_ char *root_hash_existing = NULL; + _cleanup_(crypt_freep) struct crypt_device *cd = NULL; + struct crypt_params_verity crypt_params = {}; + size_t root_hash_existing_size = root_hash_size; + int r; + + assert(ret_cd); + + r = crypt_init_by_name(&cd, name); + if (r < 0) + return log_debug_errno(r, "Error opening verity device, crypt_init_by_name failed: %m"); + r = crypt_get_verity_info(cd, &crypt_params); + if (r < 0) + return log_debug_errno(r, "Error opening verity device, crypt_get_verity_info failed: %m"); + root_hash_existing = malloc0(root_hash_size); + if (!root_hash_existing) + return -ENOMEM; + r = crypt_volume_key_get(cd, CRYPT_ANY_SLOT, root_hash_existing, &root_hash_existing_size, NULL, 0); + if (r < 0) + return log_debug_errno(r, "Error opening verity device, crypt_volume_key_get failed: %m"); + if (root_hash_size != root_hash_existing_size || memcmp(root_hash_existing, root_hash, root_hash_size) != 0) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Error opening verity device, it already exists but root hashes are different."); +#if HAVE_CRYPT_ACTIVATE_BY_SIGNED_KEY + /* Ensure that, if signatures are supported, we only reuse the device if the previous mount + * used the same settings, so that a previous unsigned mount will not be reused if the user + * asks to use signing for the new one, and viceversa. */ + if (has_sig != !!(crypt_params.flags & CRYPT_VERITY_ROOT_HASH_SIGNATURE)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Error opening verity device, it already exists but signature settings are not the same."); +#endif + + *ret_cd = TAKE_PTR(cd); + return 0; +} + +static inline void dm_deferred_remove_clean(char *name) { + if (!name) + return; + (void) crypt_deactivate_by_name(NULL, name, CRYPT_DEACTIVATE_DEFERRED); + free(name); +} +DEFINE_TRIVIAL_CLEANUP_FUNC(char *, dm_deferred_remove_clean); + static int verity_partition( DissectedPartition *m, DissectedPartition *v, @@ -1229,8 +1274,9 @@ static int verity_partition( DissectImageFlags flags, DecryptedImage *d) { - _cleanup_free_ char *node = NULL, *name = NULL; + _cleanup_free_ char *node = NULL, *name = NULL, *hash_sig_from_file = NULL; _cleanup_(crypt_freep) struct crypt_device *cd = NULL; + _cleanup_(dm_deferred_remove_cleanp) char *restore_deferred_remove = NULL; int r; assert(m); @@ -1249,12 +1295,23 @@ static int verity_partition( return 0; } - r = make_dm_name_and_node(m->node, "-verity", &name, &node); + if (FLAGS_SET(flags, DISSECT_IMAGE_VERITY_SHARE)) { + /* Use the roothash, which is unique per volume, as the device node name, so that it can be reused */ + _cleanup_free_ char *root_hash_encoded = NULL; + root_hash_encoded = hexmem(root_hash, root_hash_size); + if (!root_hash_encoded) + return -ENOMEM; + r = make_dm_name_and_node(root_hash_encoded, "-verity", &name, &node); + } else + r = make_dm_name_and_node(m->node, "-verity", &name, &node); if (r < 0) return r; - if (!GREEDY_REALLOC0(d->decrypted, d->n_allocated, d->n_decrypted + 1)) - return -ENOMEM; + if (!root_hash_sig && root_hash_sig_path) { + r = read_full_file_full(AT_FDCWD, root_hash_sig_path, 0, &hash_sig_from_file, &root_hash_sig_size); + if (r < 0) + return r; + } r = crypt_init(&cd, verity_data ?: v->node); if (r < 0) @@ -1270,27 +1327,69 @@ static int verity_partition( if (r < 0) return r; - if (root_hash_sig || root_hash_sig_path) { + if (!GREEDY_REALLOC0(d->decrypted, d->n_allocated, d->n_decrypted + 1)) + return -ENOMEM; + + /* If activating fails because the device already exists, check the metadata and reuse it if it matches. + * In case of ENODEV/ENOENT, which can happen if another process is activating at the exact same time, + * retry a few times before giving up. */ + for (unsigned i = 0; i < N_DEVICE_NODE_LIST_ATTEMPTS; i++) { + if (root_hash_sig || hash_sig_from_file) { #if HAVE_CRYPT_ACTIVATE_BY_SIGNED_KEY - if (root_hash_sig) - r = crypt_activate_by_signed_key(cd, name, root_hash, root_hash_size, root_hash_sig, root_hash_sig_size, CRYPT_ACTIVATE_READONLY); - else { - _cleanup_free_ char *hash_sig = NULL; - size_t hash_sig_size; - - r = read_full_file_full(AT_FDCWD, root_hash_sig_path, 0, &hash_sig, &hash_sig_size); - if (r < 0) - return r; - - r = crypt_activate_by_signed_key(cd, name, root_hash, root_hash_size, hash_sig, hash_sig_size, CRYPT_ACTIVATE_READONLY); - } + r = crypt_activate_by_signed_key(cd, name, root_hash, root_hash_size, root_hash_sig ?: hash_sig_from_file, root_hash_sig_size, CRYPT_ACTIVATE_READONLY); #else - r = log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "activation of verity device with signature requested, but not supported by cryptsetup due to missing crypt_activate_by_signed_key()"); + r = log_debug_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "activation of verity device with signature requested, but not supported by cryptsetup due to missing crypt_activate_by_signed_key()"); #endif - } else - r = crypt_activate_by_volume_key(cd, name, root_hash, root_hash_size, CRYPT_ACTIVATE_READONLY); - if (r < 0) - return r; + } else + r = crypt_activate_by_volume_key(cd, name, root_hash, root_hash_size, CRYPT_ACTIVATE_READONLY); + /* libdevmapper can return EINVAL when the device is already in the activation stage. + * There's no way to distinguish this situation from a genuine error due to invalid + * parameters, so immediately fallback to activating the device with a unique name. + * Improvements in libcrypsetup can ensure this never happens: https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/96 */ + if (r == -EINVAL && FLAGS_SET(flags, DISSECT_IMAGE_VERITY_SHARE)) + return verity_partition(m, v, root_hash, root_hash_size, verity_data, NULL, root_hash_sig ?: hash_sig_from_file, root_hash_sig_size, flags & ~DISSECT_IMAGE_VERITY_SHARE, d); + if (!IN_SET(r, 0, -EEXIST, -ENODEV)) + return r; + if (r == -EEXIST) { + struct crypt_device *existing_cd = NULL; + + if (!restore_deferred_remove){ + /* To avoid races, disable automatic removal on umount while setting up the new device. Restore it on failure. */ + r = dm_deferred_remove_cancel(name); + if (r < 0) + return log_debug_errno(r, "Disabling automated deferred removal for verity device %s failed: %m", node); + restore_deferred_remove = strdup(name); + if (!restore_deferred_remove) + return -ENOMEM; + } + + r = verity_can_reuse(root_hash, root_hash_size, !!root_hash_sig || !!hash_sig_from_file, name, &existing_cd); + /* Same as above, -EINVAL can randomly happen when it actually means -EEXIST */ + if (r == -EINVAL && FLAGS_SET(flags, DISSECT_IMAGE_VERITY_SHARE)) + return verity_partition(m, v, root_hash, root_hash_size, verity_data, NULL, root_hash_sig ?: hash_sig_from_file, root_hash_sig_size, flags & ~DISSECT_IMAGE_VERITY_SHARE, d); + if (!IN_SET(r, 0, -ENODEV, -ENOENT)) + return log_debug_errno(r, "Checking whether existing verity device %s can be reused failed: %m", node); + if (r == 0) { + if (cd) + crypt_free(cd); + cd = existing_cd; + } + } + if (r == 0) + break; + } + + /* Sanity check: libdevmapper is known to report that the device already exists and is active, + * but it's actually not there, so the later filesystem probe or mount would fail. */ + if (r == 0) + r = access(node, F_OK); + /* An existing verity device was reported by libcryptsetup/libdevmapper, but we can't use it at this time. + * Fall back to activating it with a unique device name. */ + if (r != 0 && FLAGS_SET(flags, DISSECT_IMAGE_VERITY_SHARE)) + return verity_partition(m, v, root_hash, root_hash_size, verity_data, NULL, root_hash_sig ?: hash_sig_from_file, root_hash_sig_size, flags & ~DISSECT_IMAGE_VERITY_SHARE, d); + + /* Everything looks good and we'll be able to mount the device, so deferred remove will be re-enabled at that point. */ + restore_deferred_remove = mfree(restore_deferred_remove); d->decrypted[d->n_decrypted].name = TAKE_PTR(name); d->decrypted[d->n_decrypted].device = TAKE_PTR(cd); @@ -1357,7 +1456,7 @@ int dissected_image_decrypt( k = PARTITION_VERITY_OF(i); if (k >= 0) { - r = verity_partition(p, m->partitions + k, root_hash, root_hash_size, verity_data, root_hash_sig_path, root_hash_sig, root_hash_sig_size, flags, d); + r = verity_partition(p, m->partitions + k, root_hash, root_hash_size, verity_data, root_hash_sig_path, root_hash_sig, root_hash_sig_size, flags | DISSECT_IMAGE_VERITY_SHARE, d); if (r < 0) return r; } diff --git a/src/shared/dissect-image.h b/src/shared/dissect-image.h index c7d078f4b7..84ec1ce331 100644 --- a/src/shared/dissect-image.h +++ b/src/shared/dissect-image.h @@ -64,6 +64,7 @@ typedef enum DissectImageFlags { DISSECT_IMAGE_RELAX_VAR_CHECK = 1 << 10, /* Don't insist that the UUID of /var is hashed from /etc/machine-id */ DISSECT_IMAGE_FSCK = 1 << 11, /* File system check the partition before mounting (no effect when combined with DISSECT_IMAGE_READ_ONLY) */ DISSECT_IMAGE_NO_PARTITION_TABLE = 1 << 12, /* Only recognize single file system images */ + DISSECT_IMAGE_VERITY_SHARE = 1 << 13, /* When activating a verity device, reuse existing one if already open */ } DissectImageFlags; struct DissectedImage { diff --git a/src/shared/dm-util.c b/src/shared/dm-util.c index 9ffa427027..7efcb6b2aa 100644 --- a/src/shared/dm-util.c +++ b/src/shared/dm-util.c @@ -5,3 +5,39 @@ #include "dm-util.h" #include "fd-util.h" #include "string-util.h" + +int dm_deferred_remove_cancel(const char *name) { + _cleanup_close_ int fd = -1; + struct message { + struct dm_ioctl dm_ioctl; + struct dm_target_msg dm_target_msg; + char msg_text[STRLEN("@cancel_deferred_remove") + 1]; + } _packed_ message = { + .dm_ioctl = { + .version = { + DM_VERSION_MAJOR, + DM_VERSION_MINOR, + DM_VERSION_PATCHLEVEL + }, + .data_size = sizeof(struct message), + .data_start = sizeof(struct dm_ioctl), + }, + .msg_text = "@cancel_deferred_remove", + }; + + assert(name); + + if (strlen(name) >= sizeof(message.dm_ioctl.name)) + return -ENODEV; /* A device with a name longer than this cannot possibly exist */ + + strncpy_exact(message.dm_ioctl.name, name, sizeof(message.dm_ioctl.name)); + + fd = open("/dev/mapper/control", O_RDWR|O_CLOEXEC); + if (fd < 0) + return -errno; + + if (ioctl(fd, DM_TARGET_MSG, &message)) + return -errno; + + return 0; +} diff --git a/src/shared/dm-util.h b/src/shared/dm-util.h index 3bae3d43cb..997963c042 100644 --- a/src/shared/dm-util.h +++ b/src/shared/dm-util.h @@ -1,2 +1,4 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #pragma once + +int dm_deferred_remove_cancel(const char *name); diff --git a/test/units/testsuite-50.sh b/test/units/testsuite-50.sh index 0e7588ad7f..81e48e0ad1 100755 --- a/test/units/testsuite-50.sh +++ b/test/units/testsuite-50.sh @@ -40,12 +40,23 @@ mv ${image}.roothash ${image}.foohash mv ${image}.fooverity ${image}.verity mv ${image}.foohash ${image}.roothash -mkdir -p ${image_dir}/mount +mkdir -p ${image_dir}/mount ${image_dir}/mount2 /usr/lib/systemd/systemd-dissect --mount ${image}.raw ${image_dir}/mount cat ${image_dir}/mount/usr/lib/os-release | grep -q -F -f /usr/lib/os-release cat ${image_dir}/mount/etc/os-release | grep -q -F -f /usr/lib/os-release cat ${image_dir}/mount/usr/lib/os-release | grep -q -F "MARKER=1" +# Verity volume should be shared (opened only once) +/usr/lib/systemd/systemd-dissect --mount ${image}.raw ${image_dir}/mount2 +verity_count=$(ls -1 /dev/mapper/ | grep -c verity) +# In theory we should check that count is exactly one. In practice, libdevmapper +# randomly and unpredictably fails with an unhelpful EINVAL when a device is open +# (and even mounted and in use), so best-effort is the most we can do for now +if [ ${verity_count} -lt 1 ]; then + echo "Verity device ${image}.raw not found in /dev/mapper/" + exit 1 +fi umount ${image_dir}/mount +umount ${image_dir}/mount2 systemd-run -t --property RootImage=${image}.raw /usr/bin/cat /usr/lib/os-release | grep -q -F "MARKER=1" mv ${image}.verity ${image}.fooverity