Cryptfs fstrim + FDE fix #532

quitschbo · 2025-04-04T09:28:56Z

The used stacked approach where dm-crypt is using AEAD cipher on top
of dm-integrity does not support discards for SSDs. Thus, we provide
a new 'mode' parameter to cryptfs_setup_volume_new(). We introduce a
new enum as 'cryptfs_mode_t'. The following cryptfs modes are
available:

CRYPTFS_MODE_NOT_IMPLEMENTED:
This mode is used if in no c_* sub module implements cryptfs
support. (e.g. missing c_vol)

CRYPTFS_MODE_AUTHENC:
This mode ueses AEAD mode as the previous stacked approach

CRYPTFS_MODE_ENCRYPT_ONLY:
This mode sets up encryption only as used by the tpm2d for FDE.

CRYPTFS_MODE_INTEGRITY_ENCRYPT:
This selects a new mode which allows to setup a new crypt table
layout without stacking. We provide a stand-a-lone dm-integrity
device which uses internal_hash with hmac(sha256) for data
integrity and authenticity. On top of that we use a dm-crypt
device with aes-xts-plain64 for data confidentiality.

CRYPTFS_MODE_INTEGRITY_ONLY
Now it is possible to create a cryptfs volume without
encryption but integrity/authenticity only by using a dm-integrity
device with the provided key as hmac key for internal_hash.

For backward-compatibility merge this together with: gyroidos/meta-gyroidos#269

common/cryptfs.c

k0ch4lo · 2025-04-14T06:14:47Z

common/cryptfs.c

+		if (errno != ENXIO) {
+			ERROR_ERRNO("DM_DEV_STATUS ioctl failed for lookup");
+		}
+		mem_free0(buffer);


May a goto statement with buffer free at one location would increase readability in this function?

k0ch4lo · 2025-04-14T06:17:50Z

common/cryptfs.c

 	close(fd); /* If fd is <0 from a failed open call, it's safe to just ignore the close error */
-	DEBUG("Returning %d from create_crypte_bkl_dev", retval);
-	return retval;
+	ERROR("Failed crypto block creation");


May log device name / UUID here?

common/cryptfs.c

k0ch4lo · 2025-04-14T07:10:16Z

common/cryptfs.c

-		if (errno != ENXIO)
-			ERROR_ERRNO("Cannot remove dm-crypt device");
-		goto error;
+	bool encrypt = true, integrity = false;


i'd suggest setting both values in each switch-case branch and skip setting them here

k0ch4lo · 2025-04-14T07:11:07Z

common/cryptfs.c


-	DEBUG("Successfully deleted dm-crypt device");
+	IF_FALSE_RETVAL_TRACE(integrity, 0);


Imho failure to remove this device should be a warning/error

k0ch4lo · 2025-04-14T07:13:52Z

daemon/c_vol.c

+						CRYPTFS_MODE_INTEGRITY_ENCRYPT :
+						CRYPTFS_MODE_AUTHENC;
+			mem_free0(not_stacked_file);
+			if (cryptfs_delete_blk_dev(fd, label, mode) < 0)
 				DEBUG("Could not delete dm-crypt dev %s", label);


warn/error?

The crypto key was truncated to 64 byte key length in hex representation. This would result in a 256bit aes-xts key which in the end results in an AES 128 bit key for the actual encryption and decryption. Do not use the CRYPTFS_FDE_KEY_LEN define which is the length of the key, but the newly introduced CRYPTO_HEXKEY_LEN which is twice the size for the ascii key. Fixes: 79fc87d ("common/cryptfs: switched authenticated encryption scheme to hmac,xts") Signed-off-by: Michael Weiß <[email protected]>

The used stacked approach where dm-crypt is using AEAD cipher on top of dm-integrity does not support discards for SSDs. Thus, we provide a new 'mode' parameter to cryptfs_setup_volume_new(). We introduce a new enum as 'cryptfs_mode_t'. The following cryptfs modes are available: CRYPTFS_MODE_AUTHENC: This mode sets up AEAD mode as the previous stacked approach CRYPTFS_MODE_ENCRYPT_ONLY: This mode sets up encryption only as used by the tpm2d for FDE. CRYPTFS_MODE_INTEGRITY_ENCRYPT: This selects a new mode which allows to setup a new crypt table layout without stacking. We provide a stand-alone dm-integrity device which uses internal_hash with hmac(sha256) for data integrity and authenticity. On top of that we use a dm-crypt device with aes-xts-plain64 for data confidentiality. Also some related functions were refactored, especially all buffers which were previously allocated ion the stack are now allocated on the heap. Signed-off-by: Michael Weiß <[email protected]>

Refactored error handling to improve readability of function create_device_node(). Signed-off-by: Michael Weiß <[email protected]>

The new helper container_images_dir_contains_image() returns true if the container images directory contains image files. Signed-off-by: Michael Weiß <[email protected]>

To support TRIM on SSDs for container images, we switch to the currently implemented new mode CRYPTFS_MODE_INTEGRITY_ENCRYPT as non-stacked approach of common/cryptfs. Further a clean way to migrate to new on disk layout is provided. New containers get the non-stacked layout by default. Others, can switch to new layout by a container wipe. A file "non-stacked" is created in the container images dir which persists the non stacking approach. See helper function c_vol_set_dm_mode() which is called in c_vol_start_child_early() hook just before images are mounted. Signed-off-by: Michael Weiß <[email protected]>

The API of common/cryptfs has been changed. Thus, we switch to this API here, too. We select the mode CRYPTFS_MODE_ENCRYPT_ONLY which does not change behaviour of nvmcrypt. Further, properly remove temporarily stored encryption keys from memory. Signed-off-by: Michael Weiß <[email protected]>

Expose integrity only device as cryptfs volume by new mode through cryptfs API. Now it is possible to create a cryptfs volume without encryption but integrity/authenticity only by using a dm-integrity device with the provided key as hmac key for internal_hash. The corresponding cryptfs volume could be created by using mode CRYPTFS_MODE_INTEGRITY_ONLY in cryptfs_setup_volume_new(). Since now the cryptfs provided volume may not necessarily have an associated dm-crypt block_dev anymore, it is inevitable to provide the mode also to the cryptfs_delete_blk_dev() for proper cleanup. Signed-off-by: Michael Weiß <[email protected]>

The API of common/cryptfs has been changed. Thus, we switch to this API in c_vol_cleanup_dm(), too. We select the proper cryptfs mode CRYPTFS_MODE_AUTHENC or CRYPTFS_MODE_INTEGRITY_ENCRYPT during cleanup. Signed-off-by: Michael Weiß <[email protected]>

In c_vol_cleanup_dm, we only print debug logs on failed deletion of dm-crypt/integrity or dm-verity devices. We increase the log level from DEBUG to WARN on errors of cryptfs_delete_blk_dev and verity_delete_blk_dev(). Signed-off-by: Michael Weiß <[email protected]>

Since, c0 is currently encrypted by the DUMMY_KEY and therefore effectively unencrypted, we switch to an integrity only device mapper configuration. Which we later will properly protect by an individual random key. See followup commits. Signed-off-by: Michael Weiß <[email protected]>

Instead of just using the DUMMY_KEY we now provide a random key for the hmac() which is used in the device mapper device set up by c_vol. If the tpm-bound FDE is used, the corresponding hmac key for c0 is stored in that protected storage and thus also properly secured. Signed-off-by: Michael Weiß <[email protected]>

The 'container_path' variable was used as parameter for mkdir() of the keys subdirectory. Use 'cmld_wrapped_keys_path' instead. Fixes: 08b1ef3 ("make token container specific and enable usage of usb token") Signed-off-by: Michael Weiß <[email protected]>

Added default mode CRYPTFS_MODE_NOT_IMPLEMENTED which we later can use if some sub-modules do not implement the specific calls. Signed-off-by: Michael Weiß <[email protected]>

We provide a new API to export the cryptfs mode which is used by this container. The corresponding helper which could be implemented by a c_* submodule is container_get_cryptfs_mode(). Signed-off-by: Michael Weiß <[email protected]>

Implement actual helper for container_get_cryptfs() by internal function c_vol_get_mode(). Signed-off-by: Michael Weiß <[email protected]>

Use the lately introduced cryptfs mode helper container_get_cryptfs_mode() in control_container_status_new() to provide the information through the control interface. Signed-off-by: Michael Weiß <[email protected]>

The Make flag NVMCRYPT_ONLY sets the corresponding cflag '-DTPM2D_NVMCRYPT_ONLY'. Remove ml and rcontrol module if NVMCRYPT_ONLY is set. Signed-off-by: Michael Weiß <[email protected]>

Allow to give the tpm2d information which key type should be used for FDE. Currently XTS_AES128, XTS_AES192 and XTS_AES256 are valid options. This could be set as parameter dmcrypt_key_type in dmcrypt_setup message. If not explicitly set defaults to XTS-AES256. Signed-off-by: Michael Weiß <[email protected]>

Use new FdeKeyType to determine the corresponding key length and truncate the TPM-provided key to that length. This allows to setup XTS-AES128 and XTS-AES192 encryption while the NV index still holds the full 512 bit random data in the TPM. Signed-off-by: Michael Weiß <[email protected]>

Fixes following linker errors when build on debian bookworm: cc -std=gnu99 -I.. -I../include -I../tpm2d -Icommon -pedantic -O2 -Wall -Wextra -Wformat -Wformat-security -fstack-protector-all -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now -fpic -pie -Wcast-align -ggdb -DDEBUG_BUILD -Werror -DDEFAULT_BASE_PATH=\"/var/lib/cml\" -DLOGFILE_PATH=\"/var/log\" attestation.pb-c.c tpm2d.pb-c.c tpm2d_control.c -lc -lprotobuf-c -lprotobuf-c-text -Lcommon -lcommon_full -o tpm2_control /usr/bin/ld: common/libcommon_full.a(protobuf-text.o): in function `protobuf_message_new_from_textfile': [..]/common/protobuf-text.c:75: undefined reference to `protobuf_c_text_from_file' /usr/bin/ld: common/libcommon_full.a(protobuf-text.o): in function `protobuf_message_new_from_string': /home/weiss/cml/common/protobuf-text.c:104: undefined reference to `protobuf_c_text_from_string' /usr/bin/ld: [..]/common/protobuf-text.c:121: undefined reference to `protobuf_c_text_free_ProtobufCTextError_data' /usr/bin/ld: [..]/common/protobuf-text.c:116: undefined reference to `protobuf_c_text_free_ProtobufCTextError_data' /usr/bin/ld: [..]/common/protobuf-text.c:109: undefined reference to `protobuf_c_text_free_ProtobufCTextError_data' /usr/bin/ld: common/libcommon_full.a(protobuf-text.o): in function `protobuf_string_from_message': [..]/common/protobuf-text.c:174: undefined reference to `protobuf_c_text_to_string' collect2: error: ld returned 1 exit status make: *** [Makefile:76: tpm2_control] Error 1 Fixes: e329078 ("tpm2_control/Makefile: link libcommon_full") Signed-off-by: Michael Weiß <[email protected]>

Expose paramter --key_len | -l to dmcrypt_setup. This allows to truncate the key to XTS-AES128 or XTS-AES192. The tpm2d internally always generates an FDE key of CRYPTFS_FDE_KEY_LEN which is 512 bit random data for aes-xts with AES256. Signed-off-by: Michael Weiß <[email protected]>

quitschbo force-pushed the cryptfs-fstrim branch 8 times, most recently from a95917e to edca379 Compare April 11, 2025 12:29

k0ch4lo reviewed Apr 14, 2025

View reviewed changes

quitschbo force-pushed the cryptfs-fstrim branch 4 times, most recently from 665dab6 to 38da88e Compare April 14, 2025 10:05

quitschbo requested a review from smo4201 April 15, 2025 08:09

quitschbo force-pushed the cryptfs-fstrim branch from 38da88e to 7a5bec6 Compare April 15, 2025 08:10

smo4201 approved these changes Apr 22, 2025

View reviewed changes

quitschbo force-pushed the cryptfs-fstrim branch 6 times, most recently from ba2b29a to efc5db2 Compare April 24, 2025 10:23

quitschbo marked this pull request as ready for review April 24, 2025 14:44

quitschbo force-pushed the cryptfs-fstrim branch 2 times, most recently from 3e13f49 to d4cdfb7 Compare April 25, 2025 12:25

quitschbo changed the title ~~Cryptfs fstrim~~ Cryptfs fstrim + FDE fix Apr 25, 2025

quitschbo mentioned this pull request May 5, 2025

recipes-initscripts: cml-boot-script: FDE re-encrypt with AES256 gyroidos/meta-gyroidos#269

Merged

quitschbo added 3 commits May 6, 2025 19:59

common/cryptfs: refactored error handling in create_device_node()

dd0eda8

Refactored error handling to improve readability of function create_device_node(). Signed-off-by: Michael Weiß <[email protected]>

quitschbo added 18 commits May 6, 2025 20:00

daemon/container: helper container_images_dir_contains_image()

1feab4d

The new helper container_images_dir_contains_image() returns true if the container images directory contains image files. Signed-off-by: Michael Weiß <[email protected]>

common/cryptfs: added mode CRYPTFS_MODE_NOT_IMPLEMENTED

e67f4c7

Added default mode CRYPTFS_MODE_NOT_IMPLEMENTED which we later can use if some sub-modules do not implement the specific calls. Signed-off-by: Michael Weiß <[email protected]>

daemon/c_vol: implement container_get_cryptfs_mode helper

eba5773

Implement actual helper for container_get_cryptfs() by internal function c_vol_get_mode(). Signed-off-by: Michael Weiß <[email protected]>

daemon/control: show cryptfs mode of container in status message

09f6baa

Use the lately introduced cryptfs mode helper container_get_cryptfs_mode() in control_container_status_new() to provide the information through the control interface. Signed-off-by: Michael Weiß <[email protected]>

tpm2d: allow to build with flag NVMCRYPT_ONLY=y

ab1a8a2

The Make flag NVMCRYPT_ONLY sets the corresponding cflag '-DTPM2D_NVMCRYPT_ONLY'. Remove ml and rcontrol module if NVMCRYPT_ONLY is set. Signed-off-by: Michael Weiß <[email protected]>

quitschbo force-pushed the cryptfs-fstrim branch from d4cdfb7 to 2cc24b9 Compare May 6, 2025 18:04

smo4201 merged commit 7e852b0 into gyroidos:main May 7, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cryptfs fstrim + FDE fix #532

Cryptfs fstrim + FDE fix #532

Uh oh!

quitschbo commented Apr 4, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

k0ch4lo Apr 14, 2025

Uh oh!

quitschbo Apr 14, 2025

Uh oh!

k0ch4lo Apr 14, 2025

Uh oh!

quitschbo Apr 14, 2025

Uh oh!

Uh oh!

Uh oh!

k0ch4lo Apr 14, 2025

Uh oh!

quitschbo Apr 14, 2025

Uh oh!

k0ch4lo Apr 14, 2025

Uh oh!

quitschbo Apr 14, 2025

Uh oh!

k0ch4lo Apr 14, 2025

Uh oh!

quitschbo Apr 14, 2025

Uh oh!

Uh oh!

Uh oh!


		DEBUG("Successfully deleted dm-crypt device");
		IF_FALSE_RETVAL_TRACE(integrity, 0);

Cryptfs fstrim + FDE fix #532

Cryptfs fstrim + FDE fix #532

Uh oh!

Conversation

quitschbo commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

quitschbo commented Apr 4, 2025 •

edited

Loading