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.
v1:
- Initial submission
v2:
- Added transformed_len as member of struct nvme_dhchap_key
v3:
- Return a struct nvme_dhchap_key from nvme_auth_transform_key()
Mark O'Donovan (3):
nvme-auth: alloc nvme_dhchap_key as single buffer
nvme-auth: use transformed key size to create resp
nvme-auth: allow mixing of secret and hash lengths
drivers/nvme/common/auth.c | 52 ++++++++++++++++++--------------------
drivers/nvme/host/auth.c | 30 +++++++++++-----------
drivers/nvme/target/auth.c | 30 ++++++++++++----------
include/linux/nvme-auth.h | 5 ++--
4 files changed, 59 insertions(+), 58 deletions(-)
--
2.39.2
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 <[email protected]>
Signed-off-by: Akash Appaiah <[email protected]>
Signed-off-by: Mark O'Donovan <[email protected]>
---
V1 -> V2: support target implementation and controller secrets also
V2 -> V3: return key from nvme_auth_transform_key
drivers/nvme/common/auth.c | 18 ++++++++++--------
drivers/nvme/host/auth.c | 30 +++++++++++++++---------------
drivers/nvme/target/auth.c | 30 ++++++++++++++++--------------
include/linux/nvme-auth.h | 2 +-
4 files changed, 42 insertions(+), 38 deletions(-)
diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 225fc474e34a..647931acc1ba 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -233,20 +233,21 @@ 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)
+struct nvme_dhchap_key *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
{
const char *hmac_name;
struct crypto_shash *key_tfm;
struct shash_desc *shash;
- u8 *transformed_key;
- int ret;
+ struct nvme_dhchap_key *transformed_key;
+ int ret, key_len;
if (!key || !key->key) {
pr_warn("No key specified\n");
return ERR_PTR(-ENOKEY);
}
if (key->hash == 0) {
- transformed_key = kmemdup(key->key, key->len, GFP_KERNEL);
+ key_len = sizeof(*key) + key->len;
+ transformed_key = kmemdup(key, key_len, GFP_KERNEL);
return transformed_key ? transformed_key : ERR_PTR(-ENOMEM);
}
hmac_name = nvme_auth_hmac_name(key->hash);
@@ -257,7 +258,7 @@ u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
key_tfm = crypto_alloc_shash(hmac_name, 0, 0);
if (IS_ERR(key_tfm))
- return (u8 *)key_tfm;
+ return (void *)key_tfm;
shash = kmalloc(sizeof(struct shash_desc) +
crypto_shash_descsize(key_tfm),
@@ -267,7 +268,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);
+ key_len = crypto_shash_digestsize(key_tfm);
+ transformed_key = nvme_auth_alloc_key(key_len, key->hash);
if (!transformed_key) {
ret = -ENOMEM;
goto out_free_shash;
@@ -286,7 +288,7 @@ u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
ret = crypto_shash_update(shash, "NVMe-over-Fabrics", 17);
if (ret < 0)
goto out_free_transformed_key;
- ret = crypto_shash_final(shash, transformed_key);
+ ret = crypto_shash_final(shash, transformed_key->key);
if (ret < 0)
goto out_free_transformed_key;
@@ -296,7 +298,7 @@ u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
return transformed_key;
out_free_transformed_key:
- kfree_sensitive(transformed_key);
+ nvme_auth_free_key(transformed_key);
out_free_shash:
kfree(shash);
out_free_key:
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index daf5d144a8ea..de1390d705dc 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -23,6 +23,7 @@ struct nvme_dhchap_queue_context {
struct nvme_ctrl *ctrl;
struct crypto_shash *shash_tfm;
struct crypto_kpp *dh_tfm;
+ struct nvme_dhchap_key *transformed_key;
void *buf;
int qid;
int error;
@@ -36,7 +37,6 @@ struct nvme_dhchap_queue_context {
u8 c1[64];
u8 c2[64];
u8 response[64];
- u8 *host_response;
u8 *ctrl_key;
u8 *host_key;
u8 *sess_key;
@@ -428,12 +428,12 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
dev_dbg(ctrl->device, "%s: qid %d host response seq %u transaction %d\n",
__func__, chap->qid, chap->s1, chap->transaction);
- if (!chap->host_response) {
- chap->host_response = nvme_auth_transform_key(ctrl->host_key,
+ if (!chap->transformed_key) {
+ chap->transformed_key = nvme_auth_transform_key(ctrl->host_key,
ctrl->opts->host->nqn);
- if (IS_ERR(chap->host_response)) {
- ret = PTR_ERR(chap->host_response);
- chap->host_response = NULL;
+ if (IS_ERR(chap->transformed_key)) {
+ ret = PTR_ERR(chap->transformed_key);
+ chap->transformed_key = NULL;
return ret;
}
} else {
@@ -442,7 +442,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->transformed_key->key, chap->transformed_key->len);
if (ret) {
dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
chap->qid, ret);
@@ -508,19 +508,19 @@ static int nvme_auth_dhchap_setup_ctrl_response(struct nvme_ctrl *ctrl,
struct nvme_dhchap_queue_context *chap)
{
SHASH_DESC_ON_STACK(shash, chap->shash_tfm);
- u8 *ctrl_response;
+ struct nvme_dhchap_key *transformed_key;
u8 buf[4], *challenge = chap->c2;
int ret;
- ctrl_response = nvme_auth_transform_key(ctrl->ctrl_key,
+ transformed_key = nvme_auth_transform_key(ctrl->ctrl_key,
ctrl->opts->subsysnqn);
- if (IS_ERR(ctrl_response)) {
- ret = PTR_ERR(ctrl_response);
+ if (IS_ERR(transformed_key)) {
+ ret = PTR_ERR(transformed_key);
return ret;
}
ret = crypto_shash_setkey(chap->shash_tfm,
- ctrl_response, ctrl->ctrl_key->len);
+ transformed_key->key, transformed_key->len);
if (ret) {
dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
chap->qid, ret);
@@ -586,7 +586,7 @@ static int nvme_auth_dhchap_setup_ctrl_response(struct nvme_ctrl *ctrl,
out:
if (challenge != chap->c2)
kfree(challenge);
- kfree(ctrl_response);
+ nvme_auth_free_key(transformed_key);
return ret;
}
@@ -648,8 +648,8 @@ static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl,
static void nvme_auth_reset_dhchap(struct nvme_dhchap_queue_context *chap)
{
- kfree_sensitive(chap->host_response);
- chap->host_response = NULL;
+ nvme_auth_free_key(chap->transformed_key);
+ chap->transformed_key = NULL;
kfree_sensitive(chap->host_key);
chap->host_key = NULL;
chap->host_key_len = 0;
diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 4dcddcf95279..84f3ab1f9d02 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -267,7 +267,8 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
struct shash_desc *shash;
struct nvmet_ctrl *ctrl = req->sq->ctrl;
const char *hash_name;
- u8 *challenge = req->sq->dhchap_c1, *host_response;
+ u8 *challenge = req->sq->dhchap_c1;
+ struct nvme_dhchap_key *transformed_key;
u8 buf[4];
int ret;
@@ -291,14 +292,14 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
goto out_free_tfm;
}
- host_response = nvme_auth_transform_key(ctrl->host_key, ctrl->hostnqn);
- if (IS_ERR(host_response)) {
- ret = PTR_ERR(host_response);
+ transformed_key = nvme_auth_transform_key(ctrl->host_key, ctrl->hostnqn);
+ if (IS_ERR(transformed_key)) {
+ ret = PTR_ERR(transformed_key);
goto out_free_tfm;
}
- ret = crypto_shash_setkey(shash_tfm, host_response,
- ctrl->host_key->len);
+ ret = crypto_shash_setkey(shash_tfm, transformed_key->key,
+ transformed_key->len);
if (ret)
goto out_free_response;
@@ -365,7 +366,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
kfree(challenge);
kfree(shash);
out_free_response:
- kfree_sensitive(host_response);
+ nvme_auth_free_key(transformed_key);
out_free_tfm:
crypto_free_shash(shash_tfm);
return 0;
@@ -378,7 +379,8 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
struct shash_desc *shash;
struct nvmet_ctrl *ctrl = req->sq->ctrl;
const char *hash_name;
- u8 *challenge = req->sq->dhchap_c2, *ctrl_response;
+ u8 *challenge = req->sq->dhchap_c2;
+ struct nvme_dhchap_key *transformed_key;
u8 buf[4];
int ret;
@@ -402,15 +404,15 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
goto out_free_tfm;
}
- ctrl_response = nvme_auth_transform_key(ctrl->ctrl_key,
+ transformed_key = nvme_auth_transform_key(ctrl->ctrl_key,
ctrl->subsysnqn);
- if (IS_ERR(ctrl_response)) {
- ret = PTR_ERR(ctrl_response);
+ if (IS_ERR(transformed_key)) {
+ ret = PTR_ERR(transformed_key);
goto out_free_tfm;
}
- ret = crypto_shash_setkey(shash_tfm, ctrl_response,
- ctrl->ctrl_key->len);
+ ret = crypto_shash_setkey(shash_tfm, transformed_key->key,
+ transformed_key->len);
if (ret)
goto out_free_response;
@@ -474,7 +476,7 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
kfree(challenge);
kfree(shash);
out_free_response:
- kfree_sensitive(ctrl_response);
+ nvme_auth_free_key(transformed_key);
out_free_tfm:
crypto_free_shash(shash_tfm);
return 0;
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index df96940be930..fd8f69183d55 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -28,7 +28,7 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
u8 key_hash);
void nvme_auth_free_key(struct nvme_dhchap_key *key);
struct nvme_dhchap_key *nvme_auth_alloc_key(u32 len, u8 hash);
-u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn);
+struct nvme_dhchap_key *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn);
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.39.2
Co-developed-by: Akash Appaiah <[email protected]>
Signed-off-by: Akash Appaiah <[email protected]>
Signed-off-by: Mark O'Donovan <[email protected]>
---
drivers/nvme/common/auth.c | 26 +++++++++++++++-----------
include/linux/nvme-auth.h | 3 ++-
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index d90e4f0c08b7..225fc474e34a 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -163,14 +163,9 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
p = strrchr(secret, ':');
if (p)
allocated_len = p - secret;
- key = kzalloc(sizeof(*key), GFP_KERNEL);
+ key = nvme_auth_alloc_key(allocated_len, 0);
if (!key)
return ERR_PTR(-ENOMEM);
- key->key = kzalloc(allocated_len, GFP_KERNEL);
- if (!key->key) {
- ret = -ENOMEM;
- goto out_free_key;
- }
key_len = base64_decode(secret, allocated_len, key->key);
if (key_len < 0) {
@@ -213,19 +208,28 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
key->hash = key_hash;
return key;
out_free_secret:
- kfree_sensitive(key->key);
-out_free_key:
- kfree(key);
+ nvme_auth_free_key(key);
return ERR_PTR(ret);
}
EXPORT_SYMBOL_GPL(nvme_auth_extract_key);
+struct nvme_dhchap_key *nvme_auth_alloc_key(u32 len, u8 hash)
+{
+ struct nvme_dhchap_key *key = kzalloc(len + sizeof(*key), GFP_KERNEL);
+
+ if (key) {
+ key->len = len;
+ key->hash = hash;
+ }
+ return key;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_alloc_key);
+
void nvme_auth_free_key(struct nvme_dhchap_key *key)
{
if (!key)
return;
- kfree_sensitive(key->key);
- kfree(key);
+ kfree_sensitive(key);
}
EXPORT_SYMBOL_GPL(nvme_auth_free_key);
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index dcb8030062dd..df96940be930 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -9,9 +9,9 @@
#include <crypto/kpp.h>
struct nvme_dhchap_key {
- u8 *key;
size_t len;
u8 hash;
+ u8 key[];
};
u32 nvme_auth_get_seqnum(void);
@@ -27,6 +27,7 @@ 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);
+struct nvme_dhchap_key *nvme_auth_alloc_key(u32 len, u8 hash);
u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn);
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,
--
2.39.2
On 10/17/23 00:58, Mark O'Donovan wrote:
> Co-developed-by: Akash Appaiah <[email protected]>
> Signed-off-by: Akash Appaiah <[email protected]>
> Signed-off-by: Mark O'Donovan <[email protected]>
> ---
> drivers/nvme/common/auth.c | 26 +++++++++++++++-----------
> include/linux/nvme-auth.h | 3 ++-
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index d90e4f0c08b7..225fc474e34a 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -163,14 +163,9 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
> p = strrchr(secret, ':');
> if (p)
> allocated_len = p - secret;
> - key = kzalloc(sizeof(*key), GFP_KERNEL);
> + key = nvme_auth_alloc_key(allocated_len, 0);
> if (!key)
> return ERR_PTR(-ENOMEM);
> - key->key = kzalloc(allocated_len, GFP_KERNEL);
> - if (!key->key) {
> - ret = -ENOMEM;
> - goto out_free_key;
> - }
>
> key_len = base64_decode(secret, allocated_len, key->key);
> if (key_len < 0) {
> @@ -213,19 +208,28 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
> key->hash = key_hash;
> return key;
> out_free_secret:
> - kfree_sensitive(key->key);
> -out_free_key:
> - kfree(key);
> + nvme_auth_free_key(key);
> return ERR_PTR(ret);
> }
> EXPORT_SYMBOL_GPL(nvme_auth_extract_key);
>
> +struct nvme_dhchap_key *nvme_auth_alloc_key(u32 len, u8 hash)
> +{
> + struct nvme_dhchap_key *key = kzalloc(len + sizeof(*key), GFP_KERNEL);
> +
> + if (key) {
> + key->len = len;
> + key->hash = hash;
> + }
> + return key;
> +}
> +EXPORT_SYMBOL_GPL(nvme_auth_alloc_key);
> +
> void nvme_auth_free_key(struct nvme_dhchap_key *key)
> {
> if (!key)
> return;
> - kfree_sensitive(key->key);
> - kfree(key);
> + kfree_sensitive(key);
> }
> EXPORT_SYMBOL_GPL(nvme_auth_free_key);
>
> diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
> index dcb8030062dd..df96940be930 100644
> --- a/include/linux/nvme-auth.h
> +++ b/include/linux/nvme-auth.h
> @@ -9,9 +9,9 @@
> #include <crypto/kpp.h>
>
> struct nvme_dhchap_key {
> - u8 *key;
> size_t len;
> u8 hash;
> + u8 key[];
> };
>
> u32 nvme_auth_get_seqnum(void);
> @@ -27,6 +27,7 @@ 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);
> +struct nvme_dhchap_key *nvme_auth_alloc_key(u32 len, u8 hash);
> u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn);
> 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,
Good idea.
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +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
On 10/17/23 00:58, 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 <[email protected]>
> Signed-off-by: Akash Appaiah <[email protected]>
> Signed-off-by: Mark O'Donovan <[email protected]>
> ---
> V1 -> V2: support target implementation and controller secrets also
> V2 -> V3: return key from nvme_auth_transform_key
>
Exactly what I had in mind. Thanks for doing this.
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +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
> +struct nvme_dhchap_key *nvme_auth_alloc_key(u32 len, u8 hash)
> +{
> + struct nvme_dhchap_key *key = kzalloc(len + sizeof(*key), GFP_KERNEL);
This should use the struct_size() helper:
key = kzalloc(struct_size(key, key, len), GFP_KERNEL);
Otherwise the change looks good.
> +struct nvme_dhchap_key *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
Please avoid the overly long line.
> + key_len = sizeof(*key) + key->len;
struct_size again. And maybe add a helper to calculate the size for
a key instea dof duplicating it.
> + transformed_key = kmemdup(key, key_len, GFP_KERNEL);
> return transformed_key ? transformed_key : ERR_PTR(-ENOMEM);
Nit, but I find the ? : syntax very confusing when not used in things
like macros or argument lines. A
if (!transformed_key)
return ERR_PTR(-ENOMEM);
return transformed_key;
is a bit longer, but much easier to read.
> }
> hmac_name = nvme_auth_hmac_name(key->hash);
> @@ -257,7 +258,7 @@ u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
>
> key_tfm = crypto_alloc_shash(hmac_name, 0, 0);
> if (IS_ERR(key_tfm))
> - return (u8 *)key_tfm;
> + return (void *)key_tfm;
This should (already in the original code) use ERR_CAST instead.
> + transformed_key = nvme_auth_transform_key(ctrl->host_key, ctrl->hostnqn);
Please avoid the overly long line here as well.
> +struct nvme_dhchap_key *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn);
... and here.