From: Sagi Grimberg <sagi@grimberg.me>
To: Hannes Reinecke <hare@kernel.org>, Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <kbusch@kernel.org>, linux-nvme@lists.infradead.org
Subject: Re: [PATCH 1/9] nvme-auth: modify nvme_auth_transform_key() to return status
Date: Wed, 26 Nov 2025 09:39:36 +0200 [thread overview]
Message-ID: <c01b35d7-d1a6-467e-b3e8-a1aa3d613f2d@grimberg.me> (raw)
In-Reply-To: <20250528140517.3284-2-hare@kernel.org>
Patch title is misleading. The addition is the transformed secret output...
On 28/05/2025 17:05, Hannes Reinecke wrote:
> Modify nvme_auth_transform_key() to return a status and provide
> the transformed data as argument on the command line as raw data.
The patch is missing the why explanation. I mean it looks fine, its unclear
why we need this change.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---
> drivers/nvme/common/auth.c | 72 +++++++++++++++++++++++---------------
> drivers/nvme/host/auth.c | 40 +++++++++++----------
> drivers/nvme/target/auth.c | 36 +++++++++----------
> include/linux/nvme-auth.h | 4 +--
> 4 files changed, 85 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index 3b6d759bcdf2..918c92cbd8c5 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -237,70 +237,86 @@ void nvme_auth_free_key(struct nvme_dhchap_key *key)
> }
> EXPORT_SYMBOL_GPL(nvme_auth_free_key);
>
> -struct nvme_dhchap_key *nvme_auth_transform_key(
> - struct nvme_dhchap_key *key, char *nqn)
> +int nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn,
> + u8 **transformed_secret)
> {
> const char *hmac_name;
> struct crypto_shash *key_tfm;
> SHASH_DESC_ON_STACK(shash, key_tfm);
> - struct nvme_dhchap_key *transformed_key;
> - int ret, key_len;
> + u8 *transformed_data;
> + u8 *key_data;
> + size_t transformed_len;
> + int ret;
>
> if (!key) {
> pr_warn("No key specified\n");
> - return ERR_PTR(-ENOKEY);
> + return -ENOKEY;
> }
> + key_data = kzalloc(key->len, GFP_KERNEL);
> + if (!key_data)
> + return -ENOMEM;
> + memcpy(key_data, key->key, key->len);
> if (key->hash == 0) {
> - key_len = nvme_auth_key_struct_size(key->len);
> - transformed_key = kmemdup(key, key_len, GFP_KERNEL);
> - if (!transformed_key)
> - return ERR_PTR(-ENOMEM);
> - return transformed_key;
> + *transformed_secret = key_data;
> + return key->len;
> }
> hmac_name = nvme_auth_hmac_name(key->hash);
> if (!hmac_name) {
> pr_warn("Invalid key hash id %d\n", key->hash);
> - return ERR_PTR(-EINVAL);
> + ret = -EINVAL;
> + goto out_free_data;
> }
>
> key_tfm = crypto_alloc_shash(hmac_name, 0, 0);
> - if (IS_ERR(key_tfm))
> - return ERR_CAST(key_tfm);
> + if (IS_ERR(key_tfm)) {
> + ret = PTR_ERR(key_tfm);
> + goto out_free_data;
> + }
>
> - key_len = crypto_shash_digestsize(key_tfm);
> - transformed_key = nvme_auth_alloc_key(key_len, key->hash);
> - if (!transformed_key) {
> + transformed_len = crypto_shash_digestsize(key_tfm);
> + if (transformed_len != key->len) {
> + pr_warn("incompatible digest size %ld for key (hash %s, len %ld)\n",
> + transformed_len, hmac_name, key->len);
> + ret = -EINVAL;
> + goto out_free_tfm;
> + }
> +
> + transformed_data = kzalloc(transformed_len, GFP_KERNEL);
> + if (!transformed_data) {
> ret = -ENOMEM;
> - goto out_free_key;
> + goto out_free_tfm;
> }
>
> shash->tfm = key_tfm;
> ret = crypto_shash_setkey(key_tfm, key->key, key->len);
> if (ret < 0)
> - goto out_free_transformed_key;
> + goto out_free_transformed_data;
> ret = crypto_shash_init(shash);
> if (ret < 0)
> - goto out_free_transformed_key;
> + goto out_free_transformed_data;
> ret = crypto_shash_update(shash, nqn, strlen(nqn));
> if (ret < 0)
> - goto out_free_transformed_key;
> + goto out_free_transformed_data;
> ret = crypto_shash_update(shash, "NVMe-over-Fabrics", 17);
> if (ret < 0)
> - goto out_free_transformed_key;
> - ret = crypto_shash_final(shash, transformed_key->key);
> + goto out_free_transformed_data;
> + ret = crypto_shash_final(shash, transformed_data);
> if (ret < 0)
> - goto out_free_transformed_key;
> + goto out_free_transformed_data;
>
> crypto_free_shash(key_tfm);
> + *transformed_secret = transformed_data;
>
> - return transformed_key;
> + return transformed_len;
>
> -out_free_transformed_key:
> - nvme_auth_free_key(transformed_key);
> -out_free_key:
> +out_free_transformed_data:
> + kfree(transformed_data);
> +out_free_tfm:
> crypto_free_shash(key_tfm);
> +out_free_data:
> + kfree(key_data);
>
> - return ERR_PTR(ret);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(nvme_auth_transform_key);
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index f6ddbe553289..9e7c2e889ee0 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -24,7 +24,8 @@ 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;
> + u8 *transformed_secret;
> + size_t transformed_len;
> void *buf;
> int qid;
> int error;
> @@ -437,21 +438,20 @@ 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->transformed_key) {
> - chap->transformed_key = nvme_auth_transform_key(ctrl->host_key,
> - ctrl->opts->host->nqn);
> - if (IS_ERR(chap->transformed_key)) {
> - ret = PTR_ERR(chap->transformed_key);
> - chap->transformed_key = NULL;
> + if (!chap->transformed_secret) {
> + ret = nvme_auth_transform_key(ctrl->host_key,
> + ctrl->opts->host->nqn,
> + &chap->transformed_secret);
> + if (ret < 0)
> return ret;
> - }
> + chap->transformed_len = ret;
> } else {
> dev_dbg(ctrl->device, "%s: qid %d re-using host response\n",
> __func__, chap->qid);
> }
>
> ret = crypto_shash_setkey(chap->shash_tfm,
> - chap->transformed_key->key, chap->transformed_key->len);
> + chap->transformed_secret, chap->transformed_len);
> if (ret) {
> dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
> chap->qid, ret);
> @@ -517,19 +517,20 @@ 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);
> - struct nvme_dhchap_key *transformed_key;
> + u8 *transformed_secret;
> + size_t transformed_len;
> u8 buf[4], *challenge = chap->c2;
> int ret;
>
> - transformed_key = nvme_auth_transform_key(ctrl->ctrl_key,
> - ctrl->opts->subsysnqn);
> - if (IS_ERR(transformed_key)) {
> - ret = PTR_ERR(transformed_key);
> + ret = nvme_auth_transform_key(ctrl->ctrl_key,
> + ctrl->opts->subsysnqn,
> + &transformed_secret);
> + if (ret < 0)
> return ret;
> - }
> + transformed_len = ret;
>
> ret = crypto_shash_setkey(chap->shash_tfm,
> - transformed_key->key, transformed_key->len);
> + transformed_secret, transformed_len);
> if (ret) {
> dev_warn(ctrl->device, "qid %d: failed to set key, error %d\n",
> chap->qid, ret);
> @@ -595,7 +596,7 @@ static int nvme_auth_dhchap_setup_ctrl_response(struct nvme_ctrl *ctrl,
> out:
> if (challenge != chap->c2)
> kfree(challenge);
> - nvme_auth_free_key(transformed_key);
> + kfree(transformed_secret);
> return ret;
> }
>
> @@ -657,8 +658,9 @@ static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl,
>
> static void nvme_auth_reset_dhchap(struct nvme_dhchap_queue_context *chap)
> {
> - nvme_auth_free_key(chap->transformed_key);
> - chap->transformed_key = NULL;
> + kfree(chap->transformed_secret);
> + chap->transformed_secret = NULL;
> + chap->transformed_len = 0;
> 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 b340380f3892..c70d9d259a7f 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -297,7 +297,8 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> struct nvmet_ctrl *ctrl = req->sq->ctrl;
> const char *hash_name;
> u8 *challenge = req->sq->dhchap_c1;
> - struct nvme_dhchap_key *transformed_key;
> + u8 *transformed_secret;
> + size_t transformed_len;
> u8 buf[4];
> int ret;
>
> @@ -321,15 +322,14 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> goto out_free_tfm;
> }
>
> - transformed_key = nvme_auth_transform_key(ctrl->host_key,
> - ctrl->hostnqn);
> - if (IS_ERR(transformed_key)) {
> - ret = PTR_ERR(transformed_key);
> + ret = nvme_auth_transform_key(ctrl->host_key, ctrl->hostnqn,
> + &transformed_secret);
> + if (ret < 0)
> goto out_free_tfm;
> - }
>
> - ret = crypto_shash_setkey(shash_tfm, transformed_key->key,
> - transformed_key->len);
> + transformed_len = ret;
> + ret = crypto_shash_setkey(shash_tfm, transformed_secret,
> + transformed_len);
> if (ret)
> goto out_free_response;
>
> @@ -389,7 +389,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> if (challenge != req->sq->dhchap_c1)
> kfree(challenge);
> out_free_response:
> - nvme_auth_free_key(transformed_key);
> + kfree(transformed_secret);
> out_free_tfm:
> crypto_free_shash(shash_tfm);
> return ret;
> @@ -403,7 +403,8 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
> struct nvmet_ctrl *ctrl = req->sq->ctrl;
> const char *hash_name;
> u8 *challenge = req->sq->dhchap_c2;
> - struct nvme_dhchap_key *transformed_key;
> + u8 *transformed_secret;
> + size_t transformed_len;
> u8 buf[4];
> int ret;
>
> @@ -427,15 +428,14 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
> goto out_free_tfm;
> }
>
> - transformed_key = nvme_auth_transform_key(ctrl->ctrl_key,
> - ctrl->subsysnqn);
> - if (IS_ERR(transformed_key)) {
> - ret = PTR_ERR(transformed_key);
> + ret = nvme_auth_transform_key(ctrl->ctrl_key, ctrl->subsysnqn,
> + &transformed_secret);
> + if (ret < 0)
> goto out_free_tfm;
> - }
> + transformed_len = ret;
>
> - ret = crypto_shash_setkey(shash_tfm, transformed_key->key,
> - transformed_key->len);
> + ret = crypto_shash_setkey(shash_tfm, transformed_secret,
> + transformed_len);
> if (ret)
> goto out_free_response;
>
> @@ -500,7 +500,7 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
> if (challenge != req->sq->dhchap_c2)
> kfree(challenge);
> out_free_response:
> - nvme_auth_free_key(transformed_key);
> + kfree(transformed_secret);
> out_free_tfm:
> crypto_free_shash(shash_tfm);
> return ret;
> diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
> index 60e069a6757f..fd43fa042c88 100644
> --- a/include/linux/nvme-auth.h
> +++ b/include/linux/nvme-auth.h
> @@ -29,8 +29,8 @@ 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);
> -struct nvme_dhchap_key *nvme_auth_transform_key(
> - struct nvme_dhchap_key *key, char *nqn);
> +int nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn,
> + u8 **transformed_secret) ;
> 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);
next prev parent reply other threads:[~2025-11-26 7:39 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-28 14:05 [PATCHv2 0/9] nvme-auth: switch to use the kernel keyring Hannes Reinecke
2025-05-28 14:05 ` [PATCH 1/9] nvme-auth: modify nvme_auth_transform_key() to return status Hannes Reinecke
2025-11-26 7:39 ` Sagi Grimberg [this message]
2025-11-27 8:01 ` Hannes Reinecke
2025-11-30 21:42 ` Sagi Grimberg
2025-12-01 8:49 ` Hannes Reinecke
2025-05-28 14:05 ` [PATCH 2/9] nvme-keyring: add 'dhchap' key type Hannes Reinecke
2025-06-03 0:32 ` Shinichiro Kawasaki
2025-06-03 6:11 ` Hannes Reinecke
2025-11-26 7:46 ` Sagi Grimberg
2025-11-27 8:06 ` Hannes Reinecke
2025-05-28 14:05 ` [PATCH 3/9] nvme-auth: switch to use 'struct key' Hannes Reinecke
2025-11-26 7:53 ` Sagi Grimberg
2025-11-27 8:15 ` Hannes Reinecke
2025-11-30 21:44 ` Sagi Grimberg
2025-12-01 8:50 ` Hannes Reinecke
2025-05-28 14:05 ` [PATCH 4/9] nvme: parse dhchap keys during option parsing Hannes Reinecke
2025-11-26 9:04 ` Sagi Grimberg
2025-05-28 14:05 ` [PATCH 5/9] nvmet-auth: parse dhchap key from configfs attribute Hannes Reinecke
2025-11-30 21:53 ` Sagi Grimberg
2025-12-01 9:02 ` Hannes Reinecke
2025-05-28 14:05 ` [PATCH 6/9] nvme: allow to pass in key description as dhchap secret Hannes Reinecke
2025-11-30 22:06 ` Sagi Grimberg
2025-05-28 14:05 ` [PATCH 7/9] nvme-auth: wait for authentication to finish when changing keys Hannes Reinecke
2025-11-30 22:08 ` Sagi Grimberg
2025-05-28 14:05 ` [PATCH 8/9] nvme-fabrics: allow to pass in keyring by name Hannes Reinecke
2025-11-30 22:11 ` Sagi Grimberg
2025-05-28 14:05 ` [PATCH 9/9] nvmet: add configfs attribute 'dhchap_keyring' Hannes Reinecke
2025-11-30 22:14 ` Sagi Grimberg
2025-07-03 9:39 ` [PATCHv2 0/9] nvme-auth: switch to use the kernel keyring Christoph Hellwig
2025-07-03 9:43 ` Hannes Reinecke
2025-11-30 22:15 ` Sagi Grimberg
2025-09-30 12:27 ` Keith Busch
2025-12-01 17:02 ` Keith Busch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c01b35d7-d1a6-467e-b3e8-a1aa3d613f2d@grimberg.me \
--to=sagi@grimberg.me \
--cc=hare@kernel.org \
--cc=hch@lst.de \
--cc=kbusch@kernel.org \
--cc=linux-nvme@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox