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
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]>
---
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]>
---
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
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]>
---
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
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
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.
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 <[email protected]>
>
> ---
> 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
[email protected] +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
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
[email protected] +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