* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-04 23:14 [PATCH v2] nvmet-auth: update sc_c in target host hash calculation alistair23
@ 2025-11-05 7:29 ` Hannes Reinecke
2025-11-06 2:55 ` Alistair Francis
2025-11-05 13:20 ` Christoph Hellwig
2025-11-06 13:05 ` Martin George
2 siblings, 1 reply; 9+ messages in thread
From: Hannes Reinecke @ 2025-11-05 7:29 UTC (permalink / raw)
To: alistair23, kbusch, axboe, hch, sagi, kch, linux-nvme
Cc: linux-kernel, Alistair Francis
On 11/5/25 00:14, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Commit 7e091add9c43 "nvme-auth: update sc_c in host response" added
> the sc_c variable to the dhchap queue context structure which is
> appropriately set during negotiate and then used in the host response.
>
> This breaks secure concat connections with a Linux target as the target
> code wasn't updated at the same time. This patch fixes this by adding a
> new sc_c variable to the host hash calculations.
>
> Fixes: 7e091add9c43 ("nvme-auth: update sc_c in host response")
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v2:
> - Rebase on v6.18-rc4
> - Add Fixes tag
>
> drivers/nvme/host/auth.c | 1 +
> drivers/nvme/target/auth.c | 5 +++--
> drivers/nvme/target/fabrics-cmd-auth.c | 1 +
> drivers/nvme/target/nvmet.h | 1 +
> 4 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index a01178caf15b..19980122d3d5 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -492,6 +492,7 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
> ret = crypto_shash_update(shash, buf, 2);
> if (ret)
> goto out;
> + memset(buf, 0, sizeof(buf));
> *buf = chap->sc_c;
> ret = crypto_shash_update(shash, buf, 1);
> if (ret)
> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index 02c23998e13c..f54a1425262d 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -298,7 +298,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> const char *hash_name;
> u8 *challenge = req->sq->dhchap_c1;
> struct nvme_dhchap_key *transformed_key;
> - u8 buf[4], sc_c = ctrl->concat ? 1 : 0;
> + u8 buf[4];
> int ret;
>
> hash_name = nvme_auth_hmac_name(ctrl->shash_id);
> @@ -367,7 +367,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> ret = crypto_shash_update(shash, buf, 2);
> if (ret)
> goto out;
> - *buf = sc_c;
> + *buf = req->sq->sc_c;
> ret = crypto_shash_update(shash, buf, 1);
> if (ret)
> goto out;
> @@ -378,6 +378,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> ret = crypto_shash_update(shash, ctrl->hostnqn, strlen(ctrl->hostnqn));
> if (ret)
> goto out;
> + memset(buf, 0, sizeof(buf));
> ret = crypto_shash_update(shash, buf, 1);
> if (ret)
> goto out;
> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
> index 5d7d913927d8..16894302ebe1 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -43,6 +43,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
> data->auth_protocol[0].dhchap.halen,
> data->auth_protocol[0].dhchap.dhlen);
> req->sq->dhchap_tid = le16_to_cpu(data->t_id);
> + req->sq->sc_c = le16_to_cpu(data->sc_c);
> if (data->sc_c != NVME_AUTH_SECP_NOSC) {
> if (!IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
> return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
Hmm. Storing 'sc_c' directly does away with the need for the 'concat'
controller setting.
Can't we just drop that and use 'sq->sc_c' directly when checking if
secure concatenation is enabled?
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 73da823a91fb..20be2fe43307 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -159,6 +159,7 @@ struct nvmet_sq {
> bool authenticated;
> struct delayed_work auth_expired_work;
> u16 dhchap_tid;
> + u8 sc_c;
> u8 dhchap_status;
> u8 dhchap_step;
> u8 *dhchap_c1;
Cheers,Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-05 7:29 ` Hannes Reinecke
@ 2025-11-06 2:55 ` Alistair Francis
0 siblings, 0 replies; 9+ messages in thread
From: Alistair Francis @ 2025-11-06 2:55 UTC (permalink / raw)
To: Hannes Reinecke
Cc: kbusch, axboe, hch, sagi, kch, linux-nvme, linux-kernel,
Alistair Francis
On Wed, Nov 5, 2025 at 5:29 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 11/5/25 00:14, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > Commit 7e091add9c43 "nvme-auth: update sc_c in host response" added
> > the sc_c variable to the dhchap queue context structure which is
> > appropriately set during negotiate and then used in the host response.
> >
> > This breaks secure concat connections with a Linux target as the target
> > code wasn't updated at the same time. This patch fixes this by adding a
> > new sc_c variable to the host hash calculations.
> >
> > Fixes: 7e091add9c43 ("nvme-auth: update sc_c in host response")
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > v2:
> > - Rebase on v6.18-rc4
> > - Add Fixes tag
> >
> > drivers/nvme/host/auth.c | 1 +
> > drivers/nvme/target/auth.c | 5 +++--
> > drivers/nvme/target/fabrics-cmd-auth.c | 1 +
> > drivers/nvme/target/nvmet.h | 1 +
> > 4 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> > index a01178caf15b..19980122d3d5 100644
> > --- a/drivers/nvme/host/auth.c
> > +++ b/drivers/nvme/host/auth.c
> > @@ -492,6 +492,7 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
> > ret = crypto_shash_update(shash, buf, 2);
> > if (ret)
> > goto out;
> > + memset(buf, 0, sizeof(buf));
> > *buf = chap->sc_c;
> > ret = crypto_shash_update(shash, buf, 1);
> > if (ret)
> > diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> > index 02c23998e13c..f54a1425262d 100644
> > --- a/drivers/nvme/target/auth.c
> > +++ b/drivers/nvme/target/auth.c
> > @@ -298,7 +298,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> > const char *hash_name;
> > u8 *challenge = req->sq->dhchap_c1;
> > struct nvme_dhchap_key *transformed_key;
> > - u8 buf[4], sc_c = ctrl->concat ? 1 : 0;
> > + u8 buf[4];
> > int ret;
> >
> > hash_name = nvme_auth_hmac_name(ctrl->shash_id);
> > @@ -367,7 +367,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> > ret = crypto_shash_update(shash, buf, 2);
> > if (ret)
> > goto out;
> > - *buf = sc_c;
> > + *buf = req->sq->sc_c;
> > ret = crypto_shash_update(shash, buf, 1);
> > if (ret)
> > goto out;
> > @@ -378,6 +378,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> > ret = crypto_shash_update(shash, ctrl->hostnqn, strlen(ctrl->hostnqn));
> > if (ret)
> > goto out;
> > + memset(buf, 0, sizeof(buf));
> > ret = crypto_shash_update(shash, buf, 1);
> > if (ret)
> > goto out;
> > diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
> > index 5d7d913927d8..16894302ebe1 100644
> > --- a/drivers/nvme/target/fabrics-cmd-auth.c
> > +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> > @@ -43,6 +43,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
> > data->auth_protocol[0].dhchap.halen,
> > data->auth_protocol[0].dhchap.dhlen);
> > req->sq->dhchap_tid = le16_to_cpu(data->t_id);
> > + req->sq->sc_c = le16_to_cpu(data->sc_c);
> > if (data->sc_c != NVME_AUTH_SECP_NOSC) {
> > if (!IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
> > return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
>
> Hmm. Storing 'sc_c' directly does away with the need for the 'concat'
> controller setting.
> Can't we just drop that and use 'sq->sc_c' directly when checking if
> secure concatenation is enabled?
I think we could drop ctrl->concat, but I feel like maybe that should
be a merge window change instead of late in the RC window.
Alistair
>
> > diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> > index 73da823a91fb..20be2fe43307 100644
> > --- a/drivers/nvme/target/nvmet.h
> > +++ b/drivers/nvme/target/nvmet.h
> > @@ -159,6 +159,7 @@ struct nvmet_sq {
> > bool authenticated;
> > struct delayed_work auth_expired_work;
> > u16 dhchap_tid;
> > + u8 sc_c;
> > u8 dhchap_status;
> > u8 dhchap_step;
> > u8 *dhchap_c1;
> Cheers,Hannes
>
> --
> Dr. Hannes Reinecke Kernel Storage Architect
> hare@suse.de +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-04 23:14 [PATCH v2] nvmet-auth: update sc_c in target host hash calculation alistair23
2025-11-05 7:29 ` Hannes Reinecke
@ 2025-11-05 13:20 ` Christoph Hellwig
2025-11-06 3:01 ` Alistair Francis
2025-11-06 13:05 ` Martin George
2 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2025-11-05 13:20 UTC (permalink / raw)
To: alistair23
Cc: hare, kbusch, axboe, hch, sagi, kch, linux-nvme, linux-kernel,
Alistair Francis
> 4 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index a01178caf15b..19980122d3d5 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -492,6 +492,7 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
> ret = crypto_shash_update(shash, buf, 2);
> if (ret)
> goto out;
> + memset(buf, 0, sizeof(buf));
> *buf = chap->sc_c;
> ret = crypto_shash_update(shash, buf, 1);
I'm really confused about both the existing code and this fixup.
Why isn't chap->sc_c directly passed to crypto_shash_update here?
Why do we need to memset buf when only a single byte is passed to
crypto_shash_update?
> ret = crypto_shash_update(shash, buf, 2);
> if (ret)
> goto out;
> - *buf = sc_c;
> + *buf = req->sq->sc_c;
> ret = crypto_shash_update(shash, buf, 1);
Just pass it directly here?
> if (ret)
> goto out;
> @@ -378,6 +378,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> ret = crypto_shash_update(shash, ctrl->hostnqn, strlen(ctrl->hostnqn));
> if (ret)
> goto out;
> + memset(buf, 0, sizeof(buf));
> ret = crypto_shash_update(shash, buf, 1);
just have a
sttic const u8 zero = 0;
and use that here instead of the memset?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-05 13:20 ` Christoph Hellwig
@ 2025-11-06 3:01 ` Alistair Francis
2025-11-06 11:15 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Alistair Francis @ 2025-11-06 3:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: hare, kbusch, axboe, sagi, kch, linux-nvme, linux-kernel,
Alistair Francis
On Wed, Nov 5, 2025 at 11:20 PM Christoph Hellwig <hch@lst.de> wrote:
>
> > 4 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> > index a01178caf15b..19980122d3d5 100644
> > --- a/drivers/nvme/host/auth.c
> > +++ b/drivers/nvme/host/auth.c
> > @@ -492,6 +492,7 @@ static int nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
> > ret = crypto_shash_update(shash, buf, 2);
> > if (ret)
> > goto out;
> > + memset(buf, 0, sizeof(buf));
> > *buf = chap->sc_c;
> > ret = crypto_shash_update(shash, buf, 1);
>
> I'm really confused about both the existing code and this fixup.
Mostly just to keep the original style from the fixes commit from the host code.
7e091add9c43 ("nvme-auth: update sc_c in host response")
>
> Why isn't chap->sc_c directly passed to crypto_shash_update here?
> Why do we need to memset buf when only a single byte is passed to
> crypto_shash_update?
>
> > ret = crypto_shash_update(shash, buf, 2);
> > if (ret)
> > goto out;
> > - *buf = sc_c;
> > + *buf = req->sq->sc_c;
> > ret = crypto_shash_update(shash, buf, 1);
>
> Just pass it directly here?
We can directly pass it. The rest of the code is copying data into
`buf` so I went with that way as it matches the existing code. I feel
it also makes it clear that it's just a const input and we aren't
editing it in crypto_shash_update().
>
> > if (ret)
> > goto out;
> > @@ -378,6 +378,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
> > ret = crypto_shash_update(shash, ctrl->hostnqn, strlen(ctrl->hostnqn));
> > if (ret)
> > goto out;
> > + memset(buf, 0, sizeof(buf));
> > ret = crypto_shash_update(shash, buf, 1);
>
> just have a
>
> sttic const u8 zero = 0;
>
> and use that here instead of the memset?
We don't actually need this memset at all, it's a mistake from my
rebase, I'll drop it.
Do you still want me to just directly pass req->sq->sc_c in?
Alistair
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-06 3:01 ` Alistair Francis
@ 2025-11-06 11:15 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2025-11-06 11:15 UTC (permalink / raw)
To: Alistair Francis
Cc: Christoph Hellwig, hare, kbusch, axboe, sagi, kch, linux-nvme,
linux-kernel, Alistair Francis
On Thu, Nov 06, 2025 at 01:01:51PM +1000, Alistair Francis wrote:
> We don't actually need this memset at all, it's a mistake from my
> rebase, I'll drop it.
>
> Do you still want me to just directly pass req->sq->sc_c in?
I think that's much cleaner. OTOH we might as well just merge your fix
ASAP and clean this entire mess up later, so given that everything
looks correct:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-04 23:14 [PATCH v2] nvmet-auth: update sc_c in target host hash calculation alistair23
2025-11-05 7:29 ` Hannes Reinecke
2025-11-05 13:20 ` Christoph Hellwig
@ 2025-11-06 13:05 ` Martin George
2025-11-06 13:09 ` Christoph Hellwig
2025-11-06 23:18 ` Alistair Francis
2 siblings, 2 replies; 9+ messages in thread
From: Martin George @ 2025-11-06 13:05 UTC (permalink / raw)
To: alistair23, hare, kbusch, axboe, hch, sagi, kch, linux-nvme
Cc: linux-kernel, Alistair Francis
On Wed, 2025-11-05 at 09:14 +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
>
> Commit 7e091add9c43 "nvme-auth: update sc_c in host response" added
> the sc_c variable to the dhchap queue context structure which is
> appropriately set during negotiate and then used in the host
> response.
>
> This breaks secure concat connections with a Linux target as the
> target
> code wasn't updated at the same time. This patch fixes this by adding
> a
> new sc_c variable to the host hash calculations.
>
> Fixes: 7e091add9c43 ("nvme-auth: update sc_c in host response")
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v2:
> - Rebase on v6.18-rc4
> - Add Fixes tag
>
> drivers/nvme/host/auth.c | 1 +
> drivers/nvme/target/auth.c | 5 +++--
> drivers/nvme/target/fabrics-cmd-auth.c | 1 +
> drivers/nvme/target/nvmet.h | 1 +
> 4 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index a01178caf15b..19980122d3d5 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -492,6 +492,7 @@ static int
> nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
> ret = crypto_shash_update(shash, buf, 2);
> if (ret)
> goto out;
> + memset(buf, 0, sizeof(buf));
> *buf = chap->sc_c;
> ret = crypto_shash_update(shash, buf, 1);
> if (ret)
This memset in host/auth.c doesn't seem to serve any purpose. Also
given your patch is intended to modify the target behavior for sc_c
handling, maybe you should restrict the patch to target side updates
alone.
All the memset cleanup in both the host/auth.c & target/auth.c should
ideally be done in a separate patch, and not part of this current
patch.
> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index 02c23998e13c..f54a1425262d 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -298,7 +298,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req,
> u8 *response,
> const char *hash_name;
> u8 *challenge = req->sq->dhchap_c1;
> struct nvme_dhchap_key *transformed_key;
> - u8 buf[4], sc_c = ctrl->concat ? 1 : 0;
> + u8 buf[4];
> int ret;
>
> hash_name = nvme_auth_hmac_name(ctrl->shash_id);
> @@ -367,7 +367,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req,
> u8 *response,
> ret = crypto_shash_update(shash, buf, 2);
> if (ret)
> goto out;
> - *buf = sc_c;
> + *buf = req->sq->sc_c;
> ret = crypto_shash_update(shash, buf, 1);
> if (ret)
> goto out;
> @@ -378,6 +378,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req,
> u8 *response,
> ret = crypto_shash_update(shash, ctrl->hostnqn, strlen(ctrl-
> >hostnqn));
> if (ret)
> goto out;
> + memset(buf, 0, sizeof(buf));
> ret = crypto_shash_update(shash, buf, 1);
> if (ret)
> goto out;
> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c
> b/drivers/nvme/target/fabrics-cmd-auth.c
> index 5d7d913927d8..16894302ebe1 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -43,6 +43,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req
> *req, void *d)
> data->auth_protocol[0].dhchap.halen,
> data->auth_protocol[0].dhchap.dhlen);
> req->sq->dhchap_tid = le16_to_cpu(data->t_id);
> + req->sq->sc_c = le16_to_cpu(data->sc_c);
Given sc_c is an unsigned 8bit int, is there really a need to make this
endian safe by calling le16_to_cpu()?
-Martin
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-06 13:05 ` Martin George
@ 2025-11-06 13:09 ` Christoph Hellwig
2025-11-06 23:18 ` Alistair Francis
1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2025-11-06 13:09 UTC (permalink / raw)
To: Martin George
Cc: alistair23, hare, kbusch, axboe, hch, sagi, kch, linux-nvme,
linux-kernel, Alistair Francis
On Thu, Nov 06, 2025 at 06:35:48PM +0530, Martin George wrote:
> > req->sq->dhchap_tid = le16_to_cpu(data->t_id);
> > + req->sq->sc_c = le16_to_cpu(data->sc_c);
>
> Given sc_c is an unsigned 8bit int, is there really a need to make this
> endian safe by calling le16_to_cpu()?
... calling le16_to_cpu on a u8 actually messed up the endianess (on
big endian systems anyway). Everyone please run sparse on your
submissions to catch this.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
2025-11-06 13:05 ` Martin George
2025-11-06 13:09 ` Christoph Hellwig
@ 2025-11-06 23:18 ` Alistair Francis
1 sibling, 0 replies; 9+ messages in thread
From: Alistair Francis @ 2025-11-06 23:18 UTC (permalink / raw)
To: Martin George
Cc: hare, kbusch, axboe, hch, sagi, kch, linux-nvme, linux-kernel,
Alistair Francis
On Thu, Nov 6, 2025 at 11:05 PM Martin George <martinus.gpy@gmail.com> wrote:
>
> On Wed, 2025-11-05 at 09:14 +1000, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > Commit 7e091add9c43 "nvme-auth: update sc_c in host response" added
> > the sc_c variable to the dhchap queue context structure which is
> > appropriately set during negotiate and then used in the host
> > response.
> >
> > This breaks secure concat connections with a Linux target as the
> > target
> > code wasn't updated at the same time. This patch fixes this by adding
> > a
> > new sc_c variable to the host hash calculations.
> >
> > Fixes: 7e091add9c43 ("nvme-auth: update sc_c in host response")
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > v2:
> > - Rebase on v6.18-rc4
> > - Add Fixes tag
> >
> > drivers/nvme/host/auth.c | 1 +
> > drivers/nvme/target/auth.c | 5 +++--
> > drivers/nvme/target/fabrics-cmd-auth.c | 1 +
> > drivers/nvme/target/nvmet.h | 1 +
> > 4 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> > index a01178caf15b..19980122d3d5 100644
> > --- a/drivers/nvme/host/auth.c
> > +++ b/drivers/nvme/host/auth.c
> > @@ -492,6 +492,7 @@ static int
> > nvme_auth_dhchap_setup_host_response(struct nvme_ctrl *ctrl,
> > ret = crypto_shash_update(shash, buf, 2);
> > if (ret)
> > goto out;
> > + memset(buf, 0, sizeof(buf));
> > *buf = chap->sc_c;
> > ret = crypto_shash_update(shash, buf, 1);
> > if (ret)
>
> This memset in host/auth.c doesn't seem to serve any purpose. Also
> given your patch is intended to modify the target behavior for sc_c
> handling, maybe you should restrict the patch to target side updates
> alone.
>
> All the memset cleanup in both the host/auth.c & target/auth.c should
> ideally be done in a separate patch, and not part of this current
> patch.
Yeah, I realised I don't actually need any of them. They have been removed in v3
>
> > diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> > index 02c23998e13c..f54a1425262d 100644
> > --- a/drivers/nvme/target/auth.c
> > +++ b/drivers/nvme/target/auth.c
> > @@ -298,7 +298,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req,
> > u8 *response,
> > const char *hash_name;
> > u8 *challenge = req->sq->dhchap_c1;
> > struct nvme_dhchap_key *transformed_key;
> > - u8 buf[4], sc_c = ctrl->concat ? 1 : 0;
> > + u8 buf[4];
> > int ret;
> >
> > hash_name = nvme_auth_hmac_name(ctrl->shash_id);
> > @@ -367,7 +367,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req,
> > u8 *response,
> > ret = crypto_shash_update(shash, buf, 2);
> > if (ret)
> > goto out;
> > - *buf = sc_c;
> > + *buf = req->sq->sc_c;
> > ret = crypto_shash_update(shash, buf, 1);
> > if (ret)
> > goto out;
> > @@ -378,6 +378,7 @@ int nvmet_auth_host_hash(struct nvmet_req *req,
> > u8 *response,
> > ret = crypto_shash_update(shash, ctrl->hostnqn, strlen(ctrl-
> > >hostnqn));
> > if (ret)
> > goto out;
> > + memset(buf, 0, sizeof(buf));
> > ret = crypto_shash_update(shash, buf, 1);
> > if (ret)
> > goto out;
> > diff --git a/drivers/nvme/target/fabrics-cmd-auth.c
> > b/drivers/nvme/target/fabrics-cmd-auth.c
> > index 5d7d913927d8..16894302ebe1 100644
> > --- a/drivers/nvme/target/fabrics-cmd-auth.c
> > +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> > @@ -43,6 +43,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req
> > *req, void *d)
> > data->auth_protocol[0].dhchap.halen,
> > data->auth_protocol[0].dhchap.dhlen);
> > req->sq->dhchap_tid = le16_to_cpu(data->t_id);
> > + req->sq->sc_c = le16_to_cpu(data->sc_c);
>
> Given sc_c is an unsigned 8bit int, is there really a need to make this
> endian safe by calling le16_to_cpu()?
No, it's not required. Fixed in v3.
Alistair
>
> -Martin
>
^ permalink raw reply [flat|nested] 9+ messages in thread