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.
v1: original submission
v2: rebased + added dedicated variable for bi-directional auth
v3: nvme-target code now reads seq-num
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 ++++++-------
drivers/nvme/target/fabrics-cmd-auth.c | 2 +-
include/linux/nvme.h | 2 +-
3 files changed, 8 insertions(+), 9 deletions(-)
base-commit: 4f82870119a46b0d04d91ef4697ac4977a255a9d
--
2.39.2
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 <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
v1: used incorrect prefix nvme-tcp
v2: added spec extract to commit message
v3: read the seq-num in nvme-target code
drivers/nvme/host/auth.c | 3 +--
drivers/nvme/target/fabrics-cmd-auth.c | 2 +-
2 files changed, 2 insertions(+), 3 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",
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 1d9854484e2e..eb7785be0ca7 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -163,11 +163,11 @@ static u16 nvmet_auth_reply(struct nvmet_req *req, void *d)
pr_debug("%s: ctrl %d qid %d challenge %*ph\n",
__func__, ctrl->cntlid, req->sq->qid, data->hl,
req->sq->dhchap_c2);
- req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
} else {
req->sq->authenticated = true;
req->sq->dhchap_c2 = NULL;
}
+ req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
return 0;
}
--
2.39.2
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 <[email protected]>
Reviewed-by: Hannes Reinecke <[email protected]>
Reviewed-by: Sagi Grimberg <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
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
On 10/25/23 12:51, 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 <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> ---
> drivers/nvme/host/auth.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
On 10/25/23 12:51, 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 <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
>
> ---
> v1: used incorrect prefix nvme-tcp
> v2: added spec extract to commit message
> v3: read the seq-num in nvme-target code
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
On Wed, Oct 25, 2023 at 10:51:22AM +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.
Thanks, queued up for nvme-6.7.