Re: [RFC PATCH 1/1] dm-integrity: support keys in the kernel keyring

[Mikulas Patocka <mpatocka@redhat.com>]

Hi

Generally, the approach in this patch is OK.

Here I have some comments for the patch:

Mikulas



The patch should increase target version - from "{1, 15, 0}" to "{1, 16, 
0}".


On Mon, 28 Sep 2026, Lorenz Kofler wrote:

> The keys of internal_hash, journal_crypt and journal_mac can only be
> given as hex strings in the table. Keys that never leave the kernel,
> such as trusted keys, cannot be used.
> 
> In addition to hex strings, accept keys from the kernel keyring in the
> same format as dm-crypt: ":<key_size>:<key_type>:<key_description>".
> The lookup is the same as in dm-crypt.
> 
> Like DM_CRYPT, DM_INTEGRITY must not be built in when the encrypted or
> trusted key types are modules.
> 
> Signed-off-by: Lorenz Kofler <lorenz@sigma-star.at>
> ---
>  .../device-mapper/dm-integrity.rst            |  25 +++
>  drivers/md/Kconfig                            |   2 +
>  drivers/md/dm-integrity.c                     | 164 ++++++++++++++++++
>  3 files changed, 191 insertions(+)
> 
> diff --git a/Documentation/admin-guide/device-mapper/dm-integrity.rst b/Documentation/admin-guide/device-mapper/dm-integrity.rst
> index 9c21301423c9..2b7f82912962 100644
> --- a/Documentation/admin-guide/device-mapper/dm-integrity.rst
> +++ b/Documentation/admin-guide/device-mapper/dm-integrity.rst
> @@ -146,6 +146,8 @@ internal_hash:algorithm(:key)	(the key is optional)
>  	from an upper layer target, such as dm-crypt. The upper layer
>  	target should check the validity of the integrity tags.
>  
> +	The format of the key is described below the list of arguments.
> +
>  recalculate
>  	Recalculate the integrity tags automatically. It is only valid
>  	when using internal hash.
> @@ -162,6 +164,8 @@ journal_crypt:algorithm(:key)	(the key is optional)
>  	the size of files that were written. To protect against this
>  	situation, you can encrypt the journal.
>  
> +	The format of the key is described below the list of arguments.
> +
>  journal_mac:algorithm(:key)	(the key is optional)
>  	Protect sector numbers in the journal from accidental or malicious
>  	modification. To protect against accidental modification, use a
> @@ -173,6 +177,8 @@ journal_mac:algorithm(:key)	(the key is optional)
>  	the journal. Thus, modified sector number would be detected at
>  	this stage.
>  
> +	The format of the key is described below the list of arguments.
> +
>  block_size:number (default 512)
>  	The size of a data block in bytes. The larger the block size the
>  	less overhead there is for per-block integrity metadata.
> @@ -225,6 +231,25 @@ legacy_recalculate
>  	set recalc_sector to zero, and the kernel would not detect the
>  	modification.
>  
> +The key of internal_hash, journal_crypt and journal_mac is encoded either as
> +a hexadecimal number or it can be passed as <key_string> prefixed with single
> +colon character (':') for keys residing in kernel keyring service.

The documentation omits the fact that you need double colon "::" - one 
colon to end the argument name and one colon as the key prefix.

> +
> +<key_string>
> +	The kernel keyring key is identified by string in following format:
> +	<key_size>:<key_type>:<key_description>.
> +
> +<key_size>
> +	The key size in bytes. The kernel key payload size must match
> +	the value passed in <key_size>.
> +
> +<key_type>
> +	Either 'logon', 'user', 'encrypted' or 'trusted' kernel key type.
> +
> +<key_description>
> +	The kernel keyring key description integrity target should look for
> +	when loading key of <key_type>.
> +
>  The journal mode (D/J), buffer_sectors, journal_watermark, commit_time and
>  allow_discards can be changed when reloading the target (load an inactive
>  table and swap the tables with suspend and resume). The other arguments
> diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
> index df27c7d066d2..e93bf0c3119d 100644
> --- a/drivers/md/Kconfig
> +++ b/drivers/md/Kconfig
> @@ -653,6 +653,8 @@ config DM_LOG_WRITES
>  config DM_INTEGRITY
>  	tristate "Integrity target support"
>  	depends on BLK_DEV_DM
> +	depends on (ENCRYPTED_KEYS || ENCRYPTED_KEYS=n)
> +	depends on (TRUSTED_KEYS || TRUSTED_KEYS=n)
>  	select BLK_DEV_INTEGRITY
>  	select DM_BUFIO
>  	select CRYPTO
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 92970e12267a..6995c870cbf5 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -25,6 +25,11 @@
>  #include <crypto/utils.h>
>  #include <linux/async_tx.h>
>  #include <linux/dm-bufio.h>
> +#include <linux/ctype.h>
> +#include <linux/key.h>
> +#include <keys/user-type.h>
> +#include <keys/encrypted-type.h>
> +#include <keys/trusted-type.h>
>  
>  #include "dm-audit.h"
>  
> @@ -4415,6 +4420,152 @@ static void free_alg(struct alg_spec *a)
>  	memset(a, 0, sizeof(*a));
>  }
>  
> +#ifdef CONFIG_KEYS
> +
> +static bool contains_whitespace(const char *str)
> +{
> +	while (*str)
> +		if (isspace(*str++))
> +			return true;
> +	return false;
> +}
> +
> +static int set_key_user(struct alg_spec *a, struct key *key)
> +{
> +	const struct user_key_payload *ukp;
> +
> +	ukp = user_key_payload_locked(key);
> +	if (!ukp)
> +		return -EKEYREVOKED;
> +
> +	if (a->key_size != ukp->datalen)
> +		return -EINVAL;
> +
> +	memcpy(a->key, ukp->data, a->key_size);
> +
> +	return 0;
> +}
> +
> +static int set_key_encrypted(struct alg_spec *a, struct key *key)
> +{
> +	const struct encrypted_key_payload *ekp;
> +
> +	ekp = key->payload.data[0];
> +	if (!ekp)
> +		return -EKEYREVOKED;
> +
> +	if (a->key_size != ekp->decrypted_datalen)
> +		return -EINVAL;
> +
> +	memcpy(a->key, ekp->decrypted_data, a->key_size);
> +
> +	return 0;
> +}
> +
> +static int set_key_trusted(struct alg_spec *a, struct key *key)
> +{
> +	const struct trusted_key_payload *tkp;
> +
> +	tkp = key->payload.data[0];
> +	if (!tkp)
> +		return -EKEYREVOKED;
> +
> +	if (a->key_size != tkp->key_len)
> +		return -EINVAL;
> +
> +	memcpy(a->key, tkp->key, a->key_size);
> +
> +	return 0;
> +}

These four functions are copied from dm-crypt.c and dm-inlinecrypt.c. 
Copying code is generally malpattern, they should be unified and moved to 
an include file (that would be included in all three targets) or to the 
key management code (that would be called from all three targets).

> +/*
> + * Load the key from the kernel keyring. a->key_string has the format
> + * ":<key_size>:<key_type>:<key_description>".
> + */
> +static int get_alg_key_from_keyring(struct alg_spec *a)
> +{
> +	int (*set_key)(struct alg_spec *a, struct key *key);
> +	const char *key_type_desc, *key_desc;
> +	struct key_type *type;
> +	unsigned int key_size;
> +	struct key *key;
> +	char dummy;
> +	int r;
> +
> +	/*
> +	 * Reject key_string with whitespace. dm core currently lacks code for
> +	 * proper whitespace escaping in arguments on DM_TABLE_STATUS path.
> +	 */
> +	if (contains_whitespace(a->key_string))
> +		return -EINVAL;
> +
> +	if (sscanf(a->key_string, ":%u%c", &key_size, &dummy) != 2 || dummy != ':')
> +		return -EINVAL;
> +	if (!key_size || key_size > KMALLOC_MAX_SIZE)
> +		return -EINVAL;

KMALLOC_MAX_SIZE allocations are unreliable, they may fail randomly 
anytime.

You should use "PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER" as the limit (I 
assume that this is enough for the key) - this is the reliability 
threshold for kmalloc.

If you really need larger allocation, you should use vmalloc or kvmalloc.

> +	/* skip ":<key_size>:", the sscanf() above guarantees the second ':' */
> +	key_type_desc = strchr(a->key_string + 1, ':') + 1;
> +
> +	/* look for next ':' separating key_type from key_description */
> +	key_desc = strchr(key_type_desc, ':');
> +	if (!key_desc || key_desc == key_type_desc || !strlen(key_desc + 1))
> +		return -EINVAL;
> +
> +	if (!strncmp(key_type_desc, "logon:", key_desc - key_type_desc + 1)) {
> +		type = &key_type_logon;
> +		set_key = set_key_user;
> +	} else if (!strncmp(key_type_desc, "user:", key_desc - key_type_desc + 1)) {
> +		type = &key_type_user;
> +		set_key = set_key_user;
> +	} else if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS) &&
> +		   !strncmp(key_type_desc, "encrypted:", key_desc - key_type_desc + 1)) {
> +		type = &key_type_encrypted;
> +		set_key = set_key_encrypted;
> +	} else if (IS_ENABLED(CONFIG_TRUSTED_KEYS) &&
> +		   !strncmp(key_type_desc, "trusted:", key_desc - key_type_desc + 1)) {
> +		type = &key_type_trusted;
> +		set_key = set_key_trusted;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	a->key = kmalloc(key_size, GFP_KERNEL);
> +	if (!a->key)
> +		return -ENOMEM;
> +	a->key_size = key_size;
> +
> +	key = request_key(type, key_desc + 1, NULL);
> +	if (IS_ERR(key)) {
> +		r = PTR_ERR(key);
> +		goto free_key;
> +	}
> +
> +	down_read(&key->sem);
> +	r = set_key(a, key);
> +	up_read(&key->sem);
> +	key_put(key);
> +	if (r < 0)
> +		goto free_key;
> +
> +	return 0;
> +
> +free_key:
> +	kfree_sensitive(a->key);
> +	a->key = NULL;
> +	a->key_size = 0;
> +	return r;
> +}
> +
> +#else
> +
> +static int get_alg_key_from_keyring(struct alg_spec *a)
> +{
> +	return -EINVAL;
> +}
> +
> +#endif /* CONFIG_KEYS */
> +
>  static int get_alg_and_key(const char *arg, struct alg_spec *a, char **error, char *error_inval)
>  {
>  	char *k;
> @@ -4429,6 +4580,19 @@ static int get_alg_and_key(const char *arg, struct alg_spec *a, char **error, ch
>  	if (k) {
>  		*k = 0;
>  		a->key_string = k + 1;
> +
> +		if (a->key_string[0] == ':') {
> +			int r = get_alg_key_from_keyring(a);
> +
> +			if (r == -ENOMEM)
> +				goto nomem;
> +			if (r == -EINVAL)
> +				goto inval;
> +			if (r)
> +				*error = "Cannot get key from the kernel keyring";
> +			return r;
> +		}
> +
>  		if (strlen(a->key_string) & 1)
>  			goto inval;
>  
> -- 
> 2.55.0
>