1. Patchew
  2. linux
  3. View series

[PATCH v2] nvme-auth: Include SC_C in RVAL controller hash

alistair23@gmail.com posted 1 patch 2 months ago
There is a newer version of this series
drivers/nvme/host/auth.c   | 3 ++-
drivers/nvme/target/auth.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
[PATCH v2] nvme-auth: Include SC_C in RVAL controller hash
Posted by alistair23@gmail.com 2 months ago 
From: Alistair Francis <alistair.francis@wdc.com>

Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
included in the Response Value (RVAL) hash and SC_C should be included.
Currently we are hardcoding 0 instead of using the correct SC_C value.

Update the host and target code to use the SC_C when calculating the
RVAL instead of using 0.

Fixes: f50fff73d620 ("nvme: implement In-Band authentication")
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
v2:
 - Rebase using new nvme_auth_hmac_update() functions

 drivers/nvme/host/auth.c   | 3 ++-
 drivers/nvme/target/auth.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index bbedbe181c8a6..63f543e809985 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -535,11 +535,12 @@ static int nvme_auth_dhchap_setup_ctrl_response(struct nvme_ctrl *ctrl,
 	put_unaligned_le16(chap->transaction, buf);
 	nvme_auth_hmac_update(&hmac, buf, 2);
 
-	memset(buf, 0, 4);
+	*buf = chap->sc_c;
 	nvme_auth_hmac_update(&hmac, buf, 1);
 	nvme_auth_hmac_update(&hmac, "Controller", 10);
 	nvme_auth_hmac_update(&hmac, ctrl->opts->subsysnqn,
 			      strlen(ctrl->opts->subsysnqn));
+	memset(buf, 0, 4);
 	nvme_auth_hmac_update(&hmac, buf, 1);
 	nvme_auth_hmac_update(&hmac, ctrl->opts->host->nqn,
 			      strlen(ctrl->opts->host->nqn));
diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index b34610e2f19d4..f032855b21477 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -402,11 +402,12 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
 	put_unaligned_le16(req->sq->dhchap_tid, buf);
 	nvme_auth_hmac_update(&hmac, buf, 2);
 
-	memset(buf, 0, 4);
+	*buf = req->sq->sc_c;
 	nvme_auth_hmac_update(&hmac, buf, 1);
 	nvme_auth_hmac_update(&hmac, "Controller", 10);
 	nvme_auth_hmac_update(&hmac, ctrl->subsys->subsysnqn,
 			      strlen(ctrl->subsys->subsysnqn));
+	memset(buf, 0, 4);
 	nvme_auth_hmac_update(&hmac, buf, 1);
 	nvme_auth_hmac_update(&hmac, ctrl->hostnqn, strlen(ctrl->hostnqn));
 	nvme_auth_hmac_final(&hmac, response);
-- 
2.53.0
Re: [PATCH v2] nvme-auth: Include SC_C in RVAL controller hash
Posted by Hannes Reinecke 2 months ago 
On 4/16/26 01:08, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
> included in the Response Value (RVAL) hash and SC_C should be included.
> Currently we are hardcoding 0 instead of using the correct SC_C value.
> 
> Update the host and target code to use the SC_C when calculating the
> RVAL instead of using 0.
> 
> Fixes: f50fff73d620 ("nvme: implement In-Band authentication")
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v2:
>   - Rebase using new nvme_auth_hmac_update() functions
> 
>   drivers/nvme/host/auth.c   | 3 ++-
>   drivers/nvme/target/auth.c | 3 ++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
I would argue that this is a fix to secure concatenation, as the
original implementation (ie the quoted commit) did not support
secure concatenation (which sets 'sc_c' to a non-zero value).

Can you adapt the 'Fixes' line?

> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index bbedbe181c8a6..63f543e809985 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -535,11 +535,12 @@ static int nvme_auth_dhchap_setup_ctrl_response(struct nvme_ctrl *ctrl,
>   	put_unaligned_le16(chap->transaction, buf);
>   	nvme_auth_hmac_update(&hmac, buf, 2);
>   
> -	memset(buf, 0, 4);
> +	*buf = chap->sc_c;
>   	nvme_auth_hmac_update(&hmac, buf, 1);
>   	nvme_auth_hmac_update(&hmac, "Controller", 10);
>   	nvme_auth_hmac_update(&hmac, ctrl->opts->subsysnqn,
>   			      strlen(ctrl->opts->subsysnqn));
> +	memset(buf, 0, 4);
>   	nvme_auth_hmac_update(&hmac, buf, 1);
>   	nvme_auth_hmac_update(&hmac, ctrl->opts->host->nqn,
>   			      strlen(ctrl->opts->host->nqn));
> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index b34610e2f19d4..f032855b21477 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -402,11 +402,12 @@ int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
>   	put_unaligned_le16(req->sq->dhchap_tid, buf);
>   	nvme_auth_hmac_update(&hmac, buf, 2);
>   
> -	memset(buf, 0, 4);
> +	*buf = req->sq->sc_c;
>   	nvme_auth_hmac_update(&hmac, buf, 1);
>   	nvme_auth_hmac_update(&hmac, "Controller", 10);
>   	nvme_auth_hmac_update(&hmac, ctrl->subsys->subsysnqn,
>   			      strlen(ctrl->subsys->subsysnqn));
> +	memset(buf, 0, 4);
>   	nvme_auth_hmac_update(&hmac, buf, 1);
>   	nvme_auth_hmac_update(&hmac, ctrl->hostnqn, strlen(ctrl->hostnqn));
>   	nvme_auth_hmac_final(&hmac, response);

Otherwise looks good.

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] nvme-auth: Include SC_C in RVAL controller hash
Posted by Christoph Hellwig 2 months ago 
On Thu, Apr 16, 2026 at 09:08:24AM +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
> included in the Response Value (RVAL) hash and SC_C should be included.
> Currently we are hardcoding 0 instead of using the correct SC_C value.
> 
> Update the host and target code to use the SC_C when calculating the
> RVAL instead of using 0.

This looks correct.  But I guess this breaks existing implementations
in the wild now?
Re: [PATCH v2] nvme-auth: Include SC_C in RVAL controller hash
Posted by Alistair Francis 2 months ago 
On Thu, Apr 16, 2026 at 3:16 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Apr 16, 2026 at 09:08:24AM +1000, alistair23@gmail.com wrote:
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
> > included in the Response Value (RVAL) hash and SC_C should be included.
> > Currently we are hardcoding 0 instead of using the correct SC_C value.
> >
> > Update the host and target code to use the SC_C when calculating the
> > RVAL instead of using 0.
>
> This looks correct.  But I guess this breaks existing implementations
> in the wild now?

It would break an implementation that is using non zero sc_c and
updates one of the Linux target or Linux host but not the other.

Note that similar changes have been made recently to "HostHost" and
didn't seem to break everything

7e091add9c43 nvme-auth: update sc_c in host response
159de7a825ae nvmet-auth: update sc_c in target host hash calculation

Alistair
Re: [PATCH v2] nvme-auth: Include SC_C in RVAL controller hash
Posted by Sagi Grimberg 1 month, 3 weeks ago 

On 16/04/2026 8:25, Alistair Francis wrote:
> On Thu, Apr 16, 2026 at 3:16 PM Christoph Hellwig <hch@lst.de> wrote:
>> On Thu, Apr 16, 2026 at 09:08:24AM +1000, alistair23@gmail.com wrote:
>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>
>>> Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
>>> included in the Response Value (RVAL) hash and SC_C should be included.
>>> Currently we are hardcoding 0 instead of using the correct SC_C value.
>>>
>>> Update the host and target code to use the SC_C when calculating the
>>> RVAL instead of using 0.
>> This looks correct.  But I guess this breaks existing implementations
>> in the wild now?
> It would break an implementation that is using non zero sc_c and
> updates one of the Linux target or Linux host but not the other.
>
> Note that similar changes have been made recently to "HostHost" and
> didn't seem to break everything
>
> 7e091add9c43 nvme-auth: update sc_c in host response
> 159de7a825ae nvmet-auth: update sc_c in target host hash calculation

Still doesn't mean that it does not break folks.

I don't see how we get around not breaking other than introducing some
compat mode under some sysctl (yukk).

Perhaps secure-concatenation is new enough that the breakage surface
is very small.

> Alistair
Re: [PATCH v2] nvme-auth: Include SC_C in RVAL controller hash
Posted by Alistair Francis 1 month, 2 weeks ago 
On Sat, Apr 25, 2026 at 8:58 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
>
> On 16/04/2026 8:25, Alistair Francis wrote:
> > On Thu, Apr 16, 2026 at 3:16 PM Christoph Hellwig <hch@lst.de> wrote:
> >> On Thu, Apr 16, 2026 at 09:08:24AM +1000, alistair23@gmail.com wrote:
> >>> From: Alistair Francis <alistair.francis@wdc.com>
> >>>
> >>> Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
> >>> included in the Response Value (RVAL) hash and SC_C should be included.
> >>> Currently we are hardcoding 0 instead of using the correct SC_C value.
> >>>
> >>> Update the host and target code to use the SC_C when calculating the
> >>> RVAL instead of using 0.
> >> This looks correct.  But I guess this breaks existing implementations
> >> in the wild now?
> > It would break an implementation that is using non zero sc_c and
> > updates one of the Linux target or Linux host but not the other.
> >
> > Note that similar changes have been made recently to "HostHost" and
> > didn't seem to break everything
> >
> > 7e091add9c43 nvme-auth: update sc_c in host response
> > 159de7a825ae nvmet-auth: update sc_c in target host hash calculation
>
> Still doesn't mean that it does not break folks.

The current implementation breaks all cases of secure concat with a
spec compliant implementation though, which does seem worse.

>
> I don't see how we get around not breaking other than introducing some
> compat mode under some sysctl (yukk).

Ewww...

>
> Perhaps secure-concatenation is new enough that the breakage surface
> is very small.

I suspect the user base right now is small to zero, especially
considering that this won't work with any spec compliant
implementation and no one else has noticed.

Alistair
Re: [PATCH v2] nvme-auth: Include SC_C in RVAL controller hash
Posted by Hannes Reinecke 1 month, 2 weeks ago 
On 4/27/26 01:22, Alistair Francis wrote:
> On Sat, Apr 25, 2026 at 8:58 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>>
>>
>>
>> On 16/04/2026 8:25, Alistair Francis wrote:
>>> On Thu, Apr 16, 2026 at 3:16 PM Christoph Hellwig <hch@lst.de> wrote:
>>>> On Thu, Apr 16, 2026 at 09:08:24AM +1000, alistair23@gmail.com wrote:
>>>>> From: Alistair Francis <alistair.francis@wdc.com>
>>>>>
>>>>> Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
>>>>> included in the Response Value (RVAL) hash and SC_C should be included.
>>>>> Currently we are hardcoding 0 instead of using the correct SC_C value.
>>>>>
>>>>> Update the host and target code to use the SC_C when calculating the
>>>>> RVAL instead of using 0.
>>>> This looks correct.  But I guess this breaks existing implementations
>>>> in the wild now?
>>> It would break an implementation that is using non zero sc_c and
>>> updates one of the Linux target or Linux host but not the other.
>>>
>>> Note that similar changes have been made recently to "HostHost" and
>>> didn't seem to break everything
>>>
>>> 7e091add9c43 nvme-auth: update sc_c in host response
>>> 159de7a825ae nvmet-auth: update sc_c in target host hash calculation
>>
>> Still doesn't mean that it does not break folks.
> 
> The current implementation breaks all cases of secure concat with a
> spec compliant implementation though, which does seem worse.
> 
>>
>> I don't see how we get around not breaking other than introducing some
>> compat mode under some sysctl (yukk).
> 
> Ewww...
> 
>>
>> Perhaps secure-concatenation is new enough that the breakage surface
>> is very small.
> 
> I suspect the user base right now is small to zero, especially
> considering that this won't work with any spec compliant
> implementation and no one else has noticed.
> 
Precisely. Secure concatenation is still pretty new, and there are
very few implementations out there.
So I'm fine with not having a 'compat' setting.

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] nvme-auth: Include SC_C in RVAL controller hash
Posted by Chris Leech 2 months ago 
On Thu, Apr 16, 2026 at 09:08:24AM +1000, alistair23@gmail.com wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Section 8.3.4.5.5 of the NVMe Base Specification 2.1 describes what is
> included in the Response Value (RVAL) hash and SC_C should be included.
> Currently we are hardcoding 0 instead of using the correct SC_C value.
> 
> Update the host and target code to use the SC_C when calculating the
> RVAL instead of using 0.
> 
> Fixes: f50fff73d620 ("nvme: implement In-Band authentication")
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> v2:
>  - Rebase using new nvme_auth_hmac_update() functions
> 
>  drivers/nvme/host/auth.c   | 3 ++-
>  drivers/nvme/target/auth.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)

Thanks, this was a good catch.  Fix looks good to me.

Reviewed-by: Chris Leech <cleech@redhat.com>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.