[PATCH 0/2] Remove secret-size restrictions for hashes

[PATCH 0/2] Remove secret-size restrictions for hashes

[Mark O'Donovan]

This relates to the hash functions used to transform the secret.
The kernel currently restricts us to using secrets equal in size
to the transformation hash function they use.
e.g. 32 byte secrets with the SHA-256(32 byte) hash function.

This restriction is not required by the spec and means
incompatibility with more permissive implementations.

With these patches the example secret from the spec should now
be permitted with any of the following:
DHHC-1:00:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
DHHC-1:01:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
DHHC-1:02:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
DHHC-1:03:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:

Note: Secrets are still restricted to 32,48 or 64 bits.


Mark O'Donovan (2):
  nvme-auth: use transformed key size to create resp
  nvme-auth: allow mixing of secret and hash lengths

 drivers/nvme/common/auth.c |  8 --------
 drivers/nvme/host/auth.c   | 11 ++++++++++-
 2 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.39.2

[PATCH 1/2] nvme-auth: use transformed key size to create resp

[Mark O'Donovan]

This does not change current behaviour as the driver currently
verifies that the secret size is the same size as the length of
the transformation hash.

Co-developed-by: Akash Appaiah <Akash.Appaiah@dell.com>
Signed-off-by: Akash Appaiah <Akash.Appaiah@dell.com>
Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
---
 drivers/nvme/host/auth.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index daf5d144a8ea..e7d478d17b06 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -418,6 +418,14 @@ static int nvme_auth_set_dhchap_failure2_data(struct nvme_ctrl *ctrl,
 	return size;
 }
 
+static int nvme_auth_dhchap_transformed_key_len(struct nvme_dhchap_key *key)
+{
+	if (key->hash)
+		return nvme_auth_hmac_hash_len(key->hash);
+
+	return key->len;
+}
+
 static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
 		struct nvme_dhchap_queue_context *chap)
 {
@@ -442,7 +450,8 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
 	}
 
 	ret = crypto_shash_setkey(chap->shash_tfm,
-			chap->host_response, ctrl->host_key->len);
+			chap->host_response,
+			nvme_auth_dhchap_transformed_key_len(ctrl->host_key));
 	if (ret) {
 		dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
 			 chap->qid, ret);
-- 
2.39.2

Re: [PATCH 1/2] nvme-auth: use transformed key size to create resp

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 1983 bytes --]

On 10/13/23 22:28, Mark O'Donovan wrote:
> This does not change current behaviour as the driver currently
> verifies that the secret size is the same size as the length of
> the transformation hash.
> 
> Co-developed-by: Akash Appaiah <Akash.Appaiah@dell.com>
> Signed-off-by: Akash Appaiah <Akash.Appaiah@dell.com>
> Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
> ---
>   drivers/nvme/host/auth.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index daf5d144a8ea..e7d478d17b06 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -418,6 +418,14 @@ static int nvme_auth_set_dhchap_failure2_data(struct nvme_ctrl *ctrl,
>   	return size;
>   }
>   
> +static int nvme_auth_dhchap_transformed_key_len(struct nvme_dhchap_key *key)
> +{
> +	if (key->hash)
> +		return nvme_auth_hmac_hash_len(key->hash);
> +
> +	return key->len;
> +}
> +
>   static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
>   		struct nvme_dhchap_queue_context *chap)
>   {
> @@ -442,7 +450,8 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
>   	}
>   
>   	ret = crypto_shash_setkey(chap->shash_tfm,
> -			chap->host_response, ctrl->host_key->len);
> +			chap->host_response,
> +			nvme_auth_dhchap_transformed_key_len(ctrl->host_key));
>   	if (ret) {
>   		dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
>   			 chap->qid, ret);

Hmm. Yeah, hash size vs secret size always gets me.
However, wouldn't it be better to return the key size from
nvme_auth_transform_key and us that directly?
(cf the attached patch)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

[-- Attachment #2: 0001-nvme-auth-use-length-of-the-transformed-key.patch --]
[-- Type: text/x-patch, Size: 3998 bytes --]

From 14ac06a586c44ca186b5270095f551f3bdb77f18 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Sat, 14 Oct 2023 13:37:31 +0200
Subject: [PATCH] nvme-auth: use length of the transformed key

The key length for generating the host response is the length
of the transformed host response, which may be different from
the length of host key.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/auth.c | 9 +++++++--
 drivers/nvme/host/auth.c   | 7 +++++--
 include/linux/nvme-auth.h  | 3 ++-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index d90e4f0c08b7..dd5f1a221f52 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -229,7 +229,8 @@ void nvme_auth_free_key(struct nvme_dhchap_key *key)
 }
 EXPORT_SYMBOL_GPL(nvme_auth_free_key);
 
-u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
+u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn,
+			    size_t *transformed_len)
 {
 	const char *hmac_name;
 	struct crypto_shash *key_tfm;
@@ -243,6 +244,8 @@ u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
 	}
 	if (key->hash == 0) {
 		transformed_key = kmemdup(key->key, key->len, GFP_KERNEL);
+		if (transformed_key)
+			*transformed_len = key->len;
 		return transformed_key ? transformed_key : ERR_PTR(-ENOMEM);
 	}
 	hmac_name = nvme_auth_hmac_name(key->hash);
@@ -263,7 +266,8 @@ u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
 		goto out_free_key;
 	}
 
-	transformed_key = kzalloc(crypto_shash_digestsize(key_tfm), GFP_KERNEL);
+	*transformed_len = crypto_shash_digestsize(key_tfm);
+	transformed_key = kzalloc(*transformed_len, GFP_KERNEL);
 	if (!transformed_key) {
 		ret = -ENOMEM;
 		goto out_free_shash;
@@ -294,6 +298,7 @@ u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
 out_free_transformed_key:
 	kfree_sensitive(transformed_key);
 out_free_shash:
+	*transformed_len = 0;
 	kfree(shash);
 out_free_key:
 	crypto_free_shash(key_tfm);
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index daf5d144a8ea..929b37502f12 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -40,6 +40,7 @@ struct nvme_dhchap_queue_context {
 	u8 *ctrl_key;
 	u8 *host_key;
 	u8 *sess_key;
+	int host_response_len;
 	int ctrl_key_len;
 	int host_key_len;
 	int sess_key_len;
@@ -430,10 +431,12 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
 
 	if (!chap->host_response) {
 		chap->host_response = nvme_auth_transform_key(ctrl->host_key,
-						ctrl->opts->host->nqn);
+						ctrl->opts->host->nqn,
+						&chap->host_response_len);
 		if (IS_ERR(chap->host_response)) {
 			ret = PTR_ERR(chap->host_response);
 			chap->host_response = NULL;
+			chap->host_response_len = 0;
 			return ret;
 		}
 	} else {
@@ -442,7 +445,7 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
 	}
 
 	ret = crypto_shash_setkey(chap->shash_tfm,
-			chap->host_response, ctrl->host_key->len);
+			chap->host_response, chap->host_response_len);
 	if (ret) {
 		dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
 			 chap->qid, ret);
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index dcb8030062dd..72f59632bf38 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -27,7 +27,8 @@ u8 nvme_auth_hmac_id(const char *hmac_name);
 struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
 					      u8 key_hash);
 void nvme_auth_free_key(struct nvme_dhchap_key *key);
-u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn);
+u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn,
+			    size_t *transformed_len);
 int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key);
 int nvme_auth_augmented_challenge(u8 hmac_id, u8 *skey, size_t skey_len,
 				  u8 *challenge, u8 *aug, size_t hlen);
-- 
2.35.3

Re: [PATCH 1/2] nvme-auth: use transformed key size to create resp

[Mark O'Donovan]

On 14/10/2023 12:41, Hannes Reinecke wrote:
> On 10/13/23 22:28, Mark O'Donovan wrote:
>> This does not change current behaviour as the driver currently
>> verifies that the secret size is the same size as the length of
>> the transformation hash.
>>
>> Co-developed-by: Akash Appaiah <Akash.Appaiah@dell.com>
>> Signed-off-by: Akash Appaiah <Akash.Appaiah@dell.com>
>> Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
>> ---
>>   drivers/nvme/host/auth.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>> index daf5d144a8ea..e7d478d17b06 100644
>> --- a/drivers/nvme/host/auth.c
>> +++ b/drivers/nvme/host/auth.c
>> @@ -418,6 +418,14 @@ static int nvme_auth_set_dhchap_failure2_data(struct nvme_ctrl *ctrl,
>>       return size;
>>   }
>> +static int nvme_auth_dhchap_transformed_key_len(struct nvme_dhchap_key *key)
>> +{
>> +    if (key->hash)
>> +        return nvme_auth_hmac_hash_len(key->hash);
>> +
>> +    return key->len;
>> +}
>> +
>>   static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
>>           struct nvme_dhchap_queue_context *chap)
>>   {
>> @@ -442,7 +450,8 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
>>       }
>>       ret = crypto_shash_setkey(chap->shash_tfm,
>> -            chap->host_response, ctrl->host_key->len);
>> +            chap->host_response,
>> +            nvme_auth_dhchap_transformed_key_len(ctrl->host_key));
>>       if (ret) {
>>           dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
>>                chap->qid, ret);
> 
> Hmm. Yeah, hash size vs secret size always gets me.
> However, wouldn't it be better to return the key size from
> nvme_auth_transform_key and us that directly?
> (cf the attached patch)
> 
> Cheers,
> 
> Hannes

Hi Hannes,

I gave this a try and it ended up being easier to put it in struct nvme_dhchap_key.
V2 also does the nvme target code, and this means the length is stored in the same place.
Let me know if this works for you.

Thanks,
Mark

[PATCH 2/2] nvme-auth: allow mixing of secret and hash lengths

[Mark O'Donovan]

We can now use any of the secret transformation hashes with a
secret, regardless of the secret size.
e.g. a 32 byte key with the SHA-512(64 byte) hash.

The example secret from the spec should now be permitted with
any of the following:
DHHC-1:00:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
DHHC-1:01:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
DHHC-1:02:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
DHHC-1:03:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:

Note: Secrets are still restricted to 32,48 or 64 bits.

Co-developed-by: Akash Appaiah <Akash.Appaiah@dell.com>
Signed-off-by: Akash Appaiah <Akash.Appaiah@dell.com>
Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
---
 drivers/nvme/common/auth.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index d90e4f0c08b7..176855f86f0d 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -187,14 +187,6 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
 		goto out_free_secret;
 	}
 
-	if (key_hash > 0 &&
-	    (key_len - 4) != nvme_auth_hmac_hash_len(key_hash)) {
-		pr_err("Mismatched key len %d for %s\n", key_len,
-		       nvme_auth_hmac_name(key_hash));
-		ret = -EINVAL;
-		goto out_free_secret;
-	}
-
 	/* The last four bytes is the CRC in little-endian format */
 	key_len -= 4;
 	/*
-- 
2.39.2

Re: [PATCH 2/2] nvme-auth: allow mixing of secret and hash lengths

[Hannes Reinecke]

On 10/13/23 22:28, Mark O'Donovan wrote:
> We can now use any of the secret transformation hashes with a
> secret, regardless of the secret size.
> e.g. a 32 byte key with the SHA-512(64 byte) hash.
> 
> The example secret from the spec should now be permitted with
> any of the following:
> DHHC-1:00:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
> DHHC-1:01:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
> DHHC-1:02:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
> DHHC-1:03:ia6zGodOr4SEG0Zzaw398rpY0wqipUWj4jWjUh4HWUz6aQ2n:
> 
> Note: Secrets are still restricted to 32,48 or 64 bits.
> 
> Co-developed-by: Akash Appaiah <Akash.Appaiah@dell.com>
> Signed-off-by: Akash Appaiah <Akash.Appaiah@dell.com>
> Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
> ---
>   drivers/nvme/common/auth.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index d90e4f0c08b7..176855f86f0d 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -187,14 +187,6 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
>   		goto out_free_secret;
>   	}
>   
> -	if (key_hash > 0 &&
> -	    (key_len - 4) != nvme_auth_hmac_hash_len(key_hash)) {
> -		pr_err("Mismatched key len %d for %s\n", key_len,
> -		       nvme_auth_hmac_name(key_hash));
> -		ret = -EINVAL;
> -		goto out_free_secret;
> -	}
> -
>   	/* The last four bytes is the CRC in little-endian format */
>   	key_len -= 4;
>   	/*

That, however, looks good.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman