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

[Martin George <martinus.gpy@gmail.com>]

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