[PATCH v2 0/3] nvme-tcp: always set valid seq_num in dhchap reply

[PATCH v2 0/3] nvme-tcp: always set valid seq_num in dhchap reply

[Mark O'Donovan]

The first patch is a small unrelated fix which makes it clear that the
response data section of the Success1 message is not optional.

The second patch removes use of the host sequence number (S2) as an
indicator of bi-directional authentication.

The third patch causes us to always set the host sequence number (S2)
to a non-zero value instead of the 0 value reserved for the secure
channel feature.

Mark O'Donovan (3):
  nvme-auth: auth success1 msg always includes resp
  nvme-auth: add flag for bi-directional auth
  nvme-auth: always set valid seq_num in dhchap reply

 drivers/nvme/host/auth.c | 13 ++++++-------
 include/linux/nvme.h     |  2 +-
 2 files changed, 7 insertions(+), 8 deletions(-)


base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
-- 
2.39.2

[PATCH v2 1/3] nvme-auth: auth success1 msg always includes resp

[Mark O'Donovan]

In cases where RVALID is false, the response is still transmitted,
but is cleared to zero.

Relevant extract from the spec:
Response R2, if valid (i.e., if the RVALID field is set to 01h),
cleared to 0h otherwise

Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

---
v1: used incorrect prefix nvme-tcp
v2: rebase on latest git

 drivers/nvme/host/auth.c | 5 +----
 include/linux/nvme.h     | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 064592a5d546..e0fc33d41baa 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -339,10 +339,7 @@ static int nvme_auth_process_dhchap_success1(struct nvme_ctrl *ctrl,
 		struct nvme_dhchap_queue_context *chap)
 {
 	struct nvmf_auth_dhchap_success1_data *data = chap->buf;
-	size_t size = sizeof(*data);
-
-	if (chap->s2)
-		size += chap->hash_len;
+	size_t size = sizeof(*data) + chap->hash_len;
 
 	if (size > CHAP_BUF_SIZE) {
 		chap->status = NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 26dd3f859d9d..8d8df9c15b74 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1722,7 +1722,7 @@ struct nvmf_auth_dhchap_success1_data {
 	__u8		rsvd2;
 	__u8		rvalid;
 	__u8		rsvd3[7];
-	/* 'hl' bytes of response value if 'rvalid' is set */
+	/* 'hl' bytes of response value */
 	__u8		rval[];
 };
 
-- 
2.39.2

Re: [PATCH v2 1/3] nvme-auth: auth success1 msg always includes resp

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 2/3] nvme-auth: add flag for bi-directional auth

[Mark O'Donovan]

Introduces an explicit variable for bi-directional auth.
The currently used variable chap->s2 is incorrectly zeroed for
uni-directional auth. That will be fixed in the next patch so this
needs to change to avoid sending unexpected success2 messages

Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
---
 drivers/nvme/host/auth.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index e0fc33d41baa..8558a02865ac 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -28,6 +28,7 @@ struct nvme_dhchap_queue_context {
 	int error;
 	u32 s1;
 	u32 s2;
+	bool bi_directional;
 	u16 transaction;
 	u8 status;
 	u8 dhgroup_id;
@@ -312,6 +313,7 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
 	data->dhvlen = cpu_to_le16(chap->host_key_len);
 	memcpy(data->rval, chap->response, chap->hash_len);
 	if (ctrl->ctrl_key) {
+		chap->bi_directional = true;
 		get_random_bytes(chap->c2, chap->hash_len);
 		data->cvalid = 1;
 		chap->s2 = nvme_auth_get_seqnum();
@@ -660,6 +662,7 @@ static void nvme_auth_reset_dhchap(struct nvme_dhchap_queue_context *chap)
 	chap->error = 0;
 	chap->s1 = 0;
 	chap->s2 = 0;
+	chap->bi_directional = false;
 	chap->transaction = 0;
 	memset(chap->c1, 0, sizeof(chap->c1));
 	memset(chap->c2, 0, sizeof(chap->c2));
@@ -822,7 +825,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
 		goto fail2;
 	}
 
-	if (chap->s2) {
+	if (chap->bi_directional) {
 		/* DH-HMAC-CHAP Step 5: send success2 */
 		dev_dbg(ctrl->device, "%s: qid %d send success2\n",
 			__func__, chap->qid);
-- 
2.39.2

Re: [PATCH v2 2/3] nvme-auth: add flag for bi-directional auth

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/3] nvme-auth: add flag for bi-directional auth

[Hannes Reinecke]

On 10/23/23 16:00, Mark O'Donovan wrote:
> Introduces an explicit variable for bi-directional auth.
> The currently used variable chap->s2 is incorrectly zeroed for
> uni-directional auth. That will be fixed in the next patch so this
> needs to change to avoid sending unexpected success2 messages
> 
> Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
> ---
>   drivers/nvme/host/auth.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

[PATCH v2 3/3] nvme-auth: always set valid seq_num in dhchap reply

[Mark O'Donovan]

Currently a seqnum of zero is sent during uni-directional
authentication. The zero value is reserved for the secure channel
feature which is not yet implemented.

Relevant extract from the spec:
The value 0h is used to indicate that bidirectional authentication
is not performed, but a challenge value C2 is carried in order to
generate a pre-shared key (PSK) for subsequent establishment of a
secure channel

Signed-off-by: Mark O'Donovan <shiftee@posteo.net>

---
v1: used incorrect prefix nvme-tcp
v2: added spec extract to commit message

 drivers/nvme/host/auth.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 8558a02865ac..7f6b2e99a78c 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -316,15 +316,14 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
 		chap->bi_directional = true;
 		get_random_bytes(chap->c2, chap->hash_len);
 		data->cvalid = 1;
-		chap->s2 = nvme_auth_get_seqnum();
 		memcpy(data->rval + chap->hash_len, chap->c2,
 		       chap->hash_len);
 		dev_dbg(ctrl->device, "%s: qid %d ctrl challenge %*ph\n",
 			__func__, chap->qid, (int)chap->hash_len, chap->c2);
 	} else {
 		memset(chap->c2, 0, chap->hash_len);
-		chap->s2 = 0;
 	}
+	chap->s2 = nvme_auth_get_seqnum();
 	data->seqnum = cpu_to_le32(chap->s2);
 	if (chap->host_key_len) {
 		dev_dbg(ctrl->device, "%s: qid %d host public key %*ph\n",
-- 
2.39.2

Re: [PATCH v2 3/3] nvme-auth: always set valid seq_num in dhchap reply

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/3] nvme-auth: always set valid seq_num in dhchap reply

[Hannes Reinecke]

On 10/23/23 16:00, Mark O'Donovan wrote:
> Currently a seqnum of zero is sent during uni-directional
> authentication. The zero value is reserved for the secure channel
> feature which is not yet implemented.
> 
> Relevant extract from the spec:
> The value 0h is used to indicate that bidirectional authentication
> is not performed, but a challenge value C2 is carried in order to
> generate a pre-shared key (PSK) for subsequent establishment of a
> secure channel
> 
> Signed-off-by: Mark O'Donovan <shiftee@posteo.net>
> 
> ---
> v1: used incorrect prefix nvme-tcp
> v2: added spec extract to commit message
> 
>   drivers/nvme/host/auth.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 8558a02865ac..7f6b2e99a78c 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -316,15 +316,14 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
>   		chap->bi_directional = true;
>   		get_random_bytes(chap->c2, chap->hash_len);
>   		data->cvalid = 1;
> -		chap->s2 = nvme_auth_get_seqnum();
>   		memcpy(data->rval + chap->hash_len, chap->c2,
>   		       chap->hash_len);
>   		dev_dbg(ctrl->device, "%s: qid %d ctrl challenge %*ph\n",
>   			__func__, chap->qid, (int)chap->hash_len, chap->c2);
>   	} else {
>   		memset(chap->c2, 0, chap->hash_len);
> -		chap->s2 = 0;
>   	}
> +	chap->s2 = nvme_auth_get_seqnum();
>   	data->seqnum = cpu_to_le32(chap->s2);
>   	if (chap->host_key_len) {
>   		dev_dbg(ctrl->device, "%s: qid %d host public key %*ph\n",

I guess you'll need to fix up nvmet, too, as this currently ignores 's2' 
when 'cvalid' is false.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH v2 0/3] nvme-tcp: always set valid seq_num in dhchap reply

[Keith Busch]

On Mon, Oct 23, 2023 at 02:00:00PM +0000, Mark O'Donovan wrote:
> The first patch is a small unrelated fix which makes it clear that the
> response data section of the Success1 message is not optional.
> 
> The second patch removes use of the host sequence number (S2) as an
> indicator of bi-directional authentication.
> 
> The third patch causes us to always set the host sequence number (S2)
> to a non-zero value instead of the 0 value reserved for the secure
> channel feature.

Should these go in now for 6.6? We're pretty close to the end. If for
6.7, there is a merge conflict that I think would be easiest resolved if
I wait until the block tree resyncs after the 6.7 merge window opens.

Re: [PATCH v2 0/3] nvme-tcp: always set valid seq_num in dhchap reply

[Hannes Reinecke]

On 10/24/23 18:40, Keith Busch wrote:
> On Mon, Oct 23, 2023 at 02:00:00PM +0000, Mark O'Donovan wrote:
>> The first patch is a small unrelated fix which makes it clear that the
>> response data section of the Success1 message is not optional.
>>
>> The second patch removes use of the host sequence number (S2) as an
>> indicator of bi-directional authentication.
>>
>> The third patch causes us to always set the host sequence number (S2)
>> to a non-zero value instead of the 0 value reserved for the secure
>> channel feature.
> 
> Should these go in now for 6.6? We're pretty close to the end. If for
> 6.7, there is a merge conflict that I think would be easiest resolved if
> I wait until the block tree resyncs after the 6.7 merge window opens.
> 
I'd suggest to wait, as we'll need to fixup nvmet, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman