1. Patchew
  2. linux
  3. View series

[PATCH v2] nvmet-auth: update sc_c in target host hash calculation

alistair23@gmail.com posted 1 patch 1 month, 2 weeks ago
There is a newer version of this series
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(-)
[PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by alistair23@gmail.com 1 month, 2 weeks ago 
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;
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;
-- 
2.51.1
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Martin George 1 month, 1 week ago 
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
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Alistair Francis 1 month, 1 week ago 
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
>
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Christoph Hellwig 1 month, 1 week ago 
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.
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Christoph Hellwig 1 month, 1 week ago 
>  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?
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Alistair Francis 1 month, 1 week ago 
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
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Christoph Hellwig 1 month, 1 week ago 
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>
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Hannes Reinecke 1 month, 2 weeks ago 
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
Re: [PATCH v2] nvmet-auth: update sc_c in target host hash calculation
Posted by Alistair Francis 1 month, 1 week ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.