diff mbox

[3/3] dm-crypt: modifications to previous patch

Message ID 1478511495-20306-4-git-send-email-okozina@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Mike Snitzer
Headers show

Commit Message

Ondrej Kozina Nov. 7, 2016, 9:38 a.m. UTC 
1) if we load kernel keyring key referenced by key description we
should also return same key description via later status call. First
the table should remain the same, second it's not reasonable to expose
kernel key payload that could already have been be invalidated/expired
by kernel keyring API.

2) search 'logon' key type instead of 'user' key type. 'logon' type
key payloads can't be read from uspace. Not sure this is _the_ correct
way either. Do we want dm-crypt to be able to load arbitrary key
type? If so perhaps we should prefix a key description with key type?
---
 drivers/md/dm-crypt.c | 83 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 23 deletions(-)

Comments

Milan Broz Nov. 13, 2016, 5:22 p.m. UTC | #1 
I think that this patch should be merged with the previous one...

On 11/07/2016 10:38 AM, Ondrej Kozina wrote:
> 1) if we load kernel keyring key referenced by key description we
> should also return same key description via later status call. First
> the table should remain the same, second it's not reasonable to expose
> kernel key payload that could already have been be invalidated/expired
> by kernel keyring API.
> 
> 2) search 'logon' key type instead of 'user' key type. 'logon' type
> key payloads can't be read from uspace. Not sure this is _the_ correct
> way either. Do we want dm-crypt to be able to load arbitrary key
> type? If so perhaps we should prefix a key description with key type?

I definitely prefer that userspace cannot access the key in keyring once
it is set (this was one of the design goals for keyring use).

Unfortunately key length is determined (in userspace) only by parsing
hexa representation in DM table.
(For example you will check if AES-128/AES-256 is used according the key length.)

I would say we should put expected key length into dm table, something like this:
<cipher> [<raw_hex_key>|:<key_bytes>:<keyring_id>] ...

(Key length is not secret attribute, moreover it was always visible
through hexa string there even if it was zeroed by dmsetup.)

Some notes:

- In previous patch, raw key is copied to cc->key always,
this patch wipes it if keyring key is used
(merging two patches avoid this).

- increase dm-crypt target version (maybe increasing minor is enough, just
old userspace will fail while parsing the new keyring string)

- please add your signed-of-by line :)

- shouldn't be a key description validated before passing it to keyring api?
(so we avoid similar surprises as just mikulas found with '+' in key :)

- please also fix Documentation/device-mapper/dm-crypt.txt

Anyway, I think this is a good start for the keyring use in dm-crypt.

Thanks,
Milan

> ---
>  drivers/md/dm-crypt.c | 83 +++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 60 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 3b0f2a3..a038942 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -142,6 +142,7 @@ struct crypt_config {
>  
>  	char *cipher;
>  	char *cipher_string;
> +	char *key_description;
>  
>  	struct crypt_iv_operations *iv_gen_ops;
>  	union {
> @@ -1495,13 +1496,20 @@ static int crypt_setkey_allcpus(struct crypt_config *cc)
>  #ifdef CONFIG_KEYS
>  static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
>  {
> -	int ret = 0;
> +	char *new_key_desc;
> +	int ret;
>  	struct key *key;
>  	const struct user_key_payload *ukp;
>  
> -	key = request_key(&key_type_user, key_desc, NULL);
> -	if (IS_ERR(key))
> +	new_key_desc = kstrdup(key_desc, GFP_KERNEL);
> +	if (!new_key_desc)
> +		return -ENOMEM;
> +
> +	key = request_key(&key_type_logon, key_desc, NULL);
> +	if (IS_ERR(key)) {
> +		kzfree(new_key_desc);
>  		return PTR_ERR(key);
> +	}
>  
>  	rcu_read_lock();
>  	ret = key_validate(key);
> @@ -1514,9 +1522,30 @@ static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
>  		goto out;
>  	}
>  	memcpy(cc->key, ukp->data, cc->key_size);
> +
> +	rcu_read_unlock();
> +	key_put(key);
> +
> +	/* clear the flag since following operations may invalidate previously valid key */
> +	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
> +
> +	ret = crypt_setkey_allcpus(cc);
> +
> +	/* wipe the kernel key payload in each case */
> +	memset(cc->key, 0, cc->key_size * sizeof(u8));
> +
> +	if (!ret) {
> +		set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
> +		kzfree(cc->key_description);
> +		cc->key_description = new_key_desc;
> +	} else
> +		kzfree(new_key_desc);
> +
> +	return ret;
>  out:
>  	rcu_read_unlock();
>  	key_put(key);
> +	kzfree(new_key_desc);
>  	return ret;
>  }
>  
> @@ -1528,17 +1557,14 @@ static int get_key_size(const char *key_desc)
>  	if (key_desc[0] != ':')
>  		return strlen(key_desc) >> 1;
>  
> -	key = request_key(&key_type_user, key_desc + 1, NULL);
> +	key = request_key(&key_type_logon, key_desc + 1, NULL);
>  	if (IS_ERR(key))
>  		return PTR_ERR(key);
>  
>  	rcu_read_lock();
>  	ret = key_validate(key);
> -	if (ret < 0)
> -		goto out;
> -
> -	ret = user_key_payload(key)->datalen;
> -out:
> +	if (!ret)
> +		ret = user_key_payload(key)->datalen;
>  	rcu_read_unlock();
>  	key_put(key);
>  	return ret;
> @@ -1566,25 +1592,30 @@ static int crypt_set_key(struct crypt_config *cc, char *key)
>  
>  	/* ':' means that the key is in kernel keyring */
>  	if (key[0] == ':') {
> -		if (crypt_set_keyring_key(cc, key + 1))
> +		if (key_string_len < 2)
>  			goto out;
> +
> +		r = crypt_set_keyring_key(cc, key + 1);
>  	} else {
>  		/* The key size may not be changed. */
>  		if (cc->key_size != (key_string_len >> 1))
>  			goto out;
> -	}
>  
> -	/* clear the flag since following operations may invalidate previously valid key */
> -	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
> +		/* clear the flag since following operations may invalidate previously valid key */
> +		clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
>  
> -	if (key[0] != ':' && cc->key_size &&
> -		crypt_decode_key(cc->key, key, cc->key_size) < 0)
> -		goto out;
> +		/* wipe references to any kernel keyring key */
> +		kzfree(cc->key_description);
> +		cc->key_description = NULL;
>  
> -	r = crypt_setkey_allcpus(cc);
> -	if (!r)
> -		set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
> +		if (cc->key_size &&
> +		    crypt_decode_key(cc->key, key, cc->key_size) < 0)
> +			goto out;
>  
> +		r = crypt_setkey_allcpus(cc);
> +		if (!r)
> +			set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
> +	}
>  out:
>  	/* Hex key string not needed after here, so wipe it. */
>  	memset(key, '0', key_string_len);
> @@ -1596,6 +1627,8 @@ static int crypt_wipe_key(struct crypt_config *cc)
>  {
>  	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
>  	memset(&cc->key, 0, cc->key_size * sizeof(u8));
> +	kzfree(cc->key_description);
> +	cc->key_description = NULL;
>  
>  	return crypt_setkey_allcpus(cc);
>  }
> @@ -1633,6 +1666,7 @@ static void crypt_dtr(struct dm_target *ti)
>  
>  	kzfree(cc->cipher);
>  	kzfree(cc->cipher_string);
> +	kzfree(cc->key_description);
>  
>  	/* Must zero key material before freeing */
>  	kzfree(cc);
> @@ -2035,10 +2069,13 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
>  	case STATUSTYPE_TABLE:
>  		DMEMIT("%s ", cc->cipher_string);
>  
> -		if (cc->key_size > 0)
> -			for (i = 0; i < cc->key_size; i++)
> -				DMEMIT("%02x", cc->key[i]);
> -		else
> +		if (cc->key_size > 0) {
> +			if (cc->key_description)
> +				DMEMIT(":%s", cc->key_description);
> +			else
> +				for (i = 0; i < cc->key_size; i++)
> +					DMEMIT("%02x", cc->key[i]);
> +		} else
>  			DMEMIT("-");
>  
>  		DMEMIT(" %llu %s %llu", (unsigned long long)cc->iv_offset,
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3b0f2a3..a038942 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -142,6 +142,7 @@  struct crypt_config {
 
 	char *cipher;
 	char *cipher_string;
+	char *key_description;
 
 	struct crypt_iv_operations *iv_gen_ops;
 	union {
@@ -1495,13 +1496,20 @@  static int crypt_setkey_allcpus(struct crypt_config *cc)
 #ifdef CONFIG_KEYS
 static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
 {
-	int ret = 0;
+	char *new_key_desc;
+	int ret;
 	struct key *key;
 	const struct user_key_payload *ukp;
 
-	key = request_key(&key_type_user, key_desc, NULL);
-	if (IS_ERR(key))
+	new_key_desc = kstrdup(key_desc, GFP_KERNEL);
+	if (!new_key_desc)
+		return -ENOMEM;
+
+	key = request_key(&key_type_logon, key_desc, NULL);
+	if (IS_ERR(key)) {
+		kzfree(new_key_desc);
 		return PTR_ERR(key);
+	}
 
 	rcu_read_lock();
 	ret = key_validate(key);
@@ -1514,9 +1522,30 @@  static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
 		goto out;
 	}
 	memcpy(cc->key, ukp->data, cc->key_size);
+
+	rcu_read_unlock();
+	key_put(key);
+
+	/* clear the flag since following operations may invalidate previously valid key */
+	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+
+	ret = crypt_setkey_allcpus(cc);
+
+	/* wipe the kernel key payload in each case */
+	memset(cc->key, 0, cc->key_size * sizeof(u8));
+
+	if (!ret) {
+		set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+		kzfree(cc->key_description);
+		cc->key_description = new_key_desc;
+	} else
+		kzfree(new_key_desc);
+
+	return ret;
 out:
 	rcu_read_unlock();
 	key_put(key);
+	kzfree(new_key_desc);
 	return ret;
 }
 
@@ -1528,17 +1557,14 @@  static int get_key_size(const char *key_desc)
 	if (key_desc[0] != ':')
 		return strlen(key_desc) >> 1;
 
-	key = request_key(&key_type_user, key_desc + 1, NULL);
+	key = request_key(&key_type_logon, key_desc + 1, NULL);
 	if (IS_ERR(key))
 		return PTR_ERR(key);
 
 	rcu_read_lock();
 	ret = key_validate(key);
-	if (ret < 0)
-		goto out;
-
-	ret = user_key_payload(key)->datalen;
-out:
+	if (!ret)
+		ret = user_key_payload(key)->datalen;
 	rcu_read_unlock();
 	key_put(key);
 	return ret;
@@ -1566,25 +1592,30 @@  static int crypt_set_key(struct crypt_config *cc, char *key)
 
 	/* ':' means that the key is in kernel keyring */
 	if (key[0] == ':') {
-		if (crypt_set_keyring_key(cc, key + 1))
+		if (key_string_len < 2)
 			goto out;
+
+		r = crypt_set_keyring_key(cc, key + 1);
 	} else {
 		/* The key size may not be changed. */
 		if (cc->key_size != (key_string_len >> 1))
 			goto out;
-	}
 
-	/* clear the flag since following operations may invalidate previously valid key */
-	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+		/* clear the flag since following operations may invalidate previously valid key */
+		clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 
-	if (key[0] != ':' && cc->key_size &&
-		crypt_decode_key(cc->key, key, cc->key_size) < 0)
-		goto out;
+		/* wipe references to any kernel keyring key */
+		kzfree(cc->key_description);
+		cc->key_description = NULL;
 
-	r = crypt_setkey_allcpus(cc);
-	if (!r)
-		set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+		if (cc->key_size &&
+		    crypt_decode_key(cc->key, key, cc->key_size) < 0)
+			goto out;
 
+		r = crypt_setkey_allcpus(cc);
+		if (!r)
+			set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+	}
 out:
 	/* Hex key string not needed after here, so wipe it. */
 	memset(key, '0', key_string_len);
@@ -1596,6 +1627,8 @@  static int crypt_wipe_key(struct crypt_config *cc)
 {
 	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 	memset(&cc->key, 0, cc->key_size * sizeof(u8));
+	kzfree(cc->key_description);
+	cc->key_description = NULL;
 
 	return crypt_setkey_allcpus(cc);
 }
@@ -1633,6 +1666,7 @@  static void crypt_dtr(struct dm_target *ti)
 
 	kzfree(cc->cipher);
 	kzfree(cc->cipher_string);
+	kzfree(cc->key_description);
 
 	/* Must zero key material before freeing */
 	kzfree(cc);
@@ -2035,10 +2069,13 @@  static void crypt_status(struct dm_target *ti, status_type_t type,
 	case STATUSTYPE_TABLE:
 		DMEMIT("%s ", cc->cipher_string);
 
-		if (cc->key_size > 0)
-			for (i = 0; i < cc->key_size; i++)
-				DMEMIT("%02x", cc->key[i]);
-		else
+		if (cc->key_size > 0) {
+			if (cc->key_description)
+				DMEMIT(":%s", cc->key_description);
+			else
+				for (i = 0; i < cc->key_size; i++)
+					DMEMIT("%02x", cc->key[i]);
+		} else
 			DMEMIT("-");
 
 		DMEMIT(" %llu %s %llu", (unsigned long long)cc->iv_offset,