Avoid overwritting LUKS2 keyslot area if possible.

With LUKS2 crypt_keyslot_change_by_passphrase() call
does not have to overwrite binary keyslot
area in-place when user asked for specific keyslot id.

If there's enough free space in keyslot binary area
we can write new keyslot material in the the free area
(identified temporarily by new keyslot id) and switch
pointers (json metadata) to point to the new keyslot area after
the keyslot area write is complete. The old keyslot
area gets deleted after the new area write is finished.

Otherwise we needlesly risk to lose the existing keyslot
if the operation gets interupted.

With this patch LUKS2 crypt_keyslot_change_by_passphrase()
overwrites existing keyslot (including keyslot area)
only if there's no free space and therefore in-place update
is necessary.

Fixes: #839.

lib/setup.c

@@ -4327,7 +4327,8 @@ int crypt_keyslot_change_by_passphrase(struct crypt_device *cd,
 	const char *new_passphrase,
 	size_t new_passphrase_size)
 {
-	int digest = -1, r, keyslot_new_orig = keyslot_new;
+	bool keyslot_swap = false;
+	int digest = -1, r;
 	struct luks2_keyslot_params params;
 	struct volume_key *vk = NULL;
 
@@ -4362,13 +4363,21 @@ int crypt_keyslot_change_by_passphrase(struct crypt_device *cd,
 	}
 	keyslot_old = r;
 
-	if (keyslot_new == CRYPT_ANY_SLOT) {
-		if (isLUKS1(cd->type))
-			keyslot_new = LUKS_keyslot_find_empty(&cd->u.luks1.hdr);
-		else if (isLUKS2(cd->type))
+	if (isLUKS2(cd->type)) {
+		/* If there is a free keyslot (both id and binary area) avoid in-place keyslot area overwrite  */
+		if (keyslot_new == CRYPT_ANY_SLOT || keyslot_new == keyslot_old) {
 			keyslot_new = LUKS2_keyslot_find_empty(cd, &cd->u.luks2.hdr, vk->keylength);
-		if (keyslot_new < 0)
-			keyslot_new = keyslot_old;
+			if (keyslot_new < 0)
+				keyslot_new = keyslot_old;
+			else
+				keyslot_swap = true;
+		}
+	} else if (isLUKS1(cd->type)) {
+		if (keyslot_new == CRYPT_ANY_SLOT) {
+			keyslot_new = LUKS_keyslot_find_empty(&cd->u.luks1.hdr);
+			if (keyslot_new < 0)
+				keyslot_new = keyslot_old;
+		}
 	}
 	log_dbg(cd, "Key change, old slot %d, new slot %d.", keyslot_old, keyslot_new);
 
@@ -4409,7 +4418,7 @@ int crypt_keyslot_change_by_passphrase(struct crypt_device *cd,
 			goto out;
 
 		/* Swap old & new so the final keyslot number remains */
-		if (keyslot_new_orig == CRYPT_ANY_SLOT && keyslot_old != keyslot_new) {
+		if (keyslot_swap && keyslot_old != keyslot_new) {
 			r = LUKS2_keyslot_swap(cd, &cd->u.luks2.hdr, keyslot_old, keyslot_new);
 			if (r < 0)
 				goto out;

man/cryptsetup-luksChangeKey.8.adoc

@@ -30,7 +30,9 @@ overwritten directly.
 
 *WARNING:* If a key-slot is overwritten, a media failure during this
 operation can cause the overwrite to fail after the old passphrase has
-been wiped and make the LUKS container inaccessible.
+been wiped and make the LUKS container inaccessible. LUKS2 mitigates
+that by never overwritting existing keyslot area as long as there's
+a free space in keyslots area at least for one more LUKS2 keyslot.
 
 *NOTE:* some parameters are effective only if used with LUKS2 format
 that supports per-keyslot parameters. For LUKS1, PBKDF type and hash

tests/compat-test2

@@ -682,6 +682,24 @@ $CRYPTSETUP luksDump $LOOPDEV | grep -q "2: luks2" && fail
 $CRYPTSETUP luksChangeKey $LOOPDEV $FAST_PBKDF_OPT -d $KEY1 $KEY2 || fail
 $CRYPTSETUP luksDump $LOOPDEV | grep -q "0: luks2" || fail
 $CRYPTSETUP luksChangeKey $LOOPDEV $FAST_PBKDF_OPT -d $KEY1 $KEY2 2>/dev/null && fail
+# make a free space in keyslot area
+echo $PWD1 | $CRYPTSETUP luksKillSlot -q $LOOPDEV 0 || fail
+
+# assert LUKS2 does not overwrite existing area with specific keyslot id
+AREA_OFFSET_OLD=$($CRYPTSETUP luksDump $LOOPDEV | grep -e "1: luks2" -A12 | grep -e "Area offset:" | cut -d: -f 2 | sed -e 's/[[:space:]]*\[bytes\]//g')
+[ 0$AREA_OFFSET_OLD -gt 0 ] || fail
+echo -e "$PWD1\n$PWD2\n" | $CRYPTSETUP luksChangeKey --key-slot 1 $LOOPDEV $FAST_PBKDF_OPT
+AREA_OFFSET_NEW=$($CRYPTSETUP luksDump $LOOPDEV | grep -e "1: luks2" -A12 | grep -e "Area offset:" | cut -d: -f 2 | sed -e 's/[[:space:]]*\[bytes\]//g')
+[ 0$AREA_OFFSET_NEW -gt 0 ] || fail
+[ $AREA_OFFSET_OLD -ne $AREA_OFFSET_NEW ] || fail "Area offsets remained same: old area $AREA_OFFSET_OLD, new area $AREA_OFFSET_NEW"
+
+# assert LUKS2 does not overwrite existing area with any sklot
+AREA_OFFSET_OLD=$($CRYPTSETUP luksDump $LOOPDEV | grep -e "1: luks2" -A12 | grep -e "Area offset:" | cut -d: -f 2 | sed -e 's/[[:space:]]*\[bytes\]//g')
+[ 0$AREA_OFFSET_OLD -gt 0 ] || fail
+echo -e "$PWD2\n$PWD1\n" | $CRYPTSETUP luksChangeKey $LOOPDEV $FAST_PBKDF_OPT
+AREA_OFFSET_NEW=$($CRYPTSETUP luksDump $LOOPDEV | grep -e "1: luks2" -A12 | grep -e "Area offset:" | cut -d: -f 2 | sed -e 's/[[:space:]]*\[bytes\]//g')
+[ 0$AREA_OFFSET_NEW -gt 0 ] || fail
+[ $AREA_OFFSET_OLD -ne $AREA_OFFSET_NEW ] || fail "Area offsets remained same: old area $AREA_OFFSET_OLD, new area $AREA_OFFSET_NEW"
 
 prepare "[24] Keyfile limit" wipe
 $CRYPTSETUP -q luksFormat $FAST_PBKDF_OPT --type luks2 $LOOPDEV $KEY1 --key-slot 0 -l 13 || fail