[PATCH 13/17] nvme-tcp: reset after recovery for secure concatenation

[Hannes Reinecke]

On 4/8/24 23:23, Sagi Grimberg wrote:
> 
> 
> On 08/04/2024 9:25, Hannes Reinecke wrote:
>> On 4/7/24 23:49, Sagi Grimberg wrote:
>>>
>>>
>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>> From: Hannes Reinecke <hare at suse.de>
>>>>
>>>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>>>> protocol after reset or recovery, but we need to start over
>>>> to establish a new TLS connection with the new keys.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>>>> ---
>>>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>>>   1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>> index 94152ded123a..3811ee9cd040 100644
>>>> --- a/drivers/nvme/host/tcp.c
>>>> +++ b/drivers/nvme/host/tcp.c
>>>> @@ -2219,6 +2219,22 @@ static void 
>>>> nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>>       }
>>>>   }
>>>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
>>>> +{
>>>> +    if (!ctrl->opts->concat)
>>>> +        return false;
>>>> +    /*
>>>> +     * If a key has been generated and TLS has not been enabled
>>>> +     * reset the queue to start TLS handshake.
>>>> +     */
>>>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>>>> +        dev_info(ctrl->device, "Reset to enable TLS with generated 
>>>> PSK\n");
>>>> +        nvme_reset_ctrl(ctrl);
>>>> +        return true;
>>>> +    }
>>>> +    return false;
>>>> +}
>>>> +
>>>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
>>>>   {
>>>>       if (!ctrl->opts->concat)
>>>> @@ -2321,6 +2337,9 @@ static void 
>>>> nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>           goto requeue;
>>>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>>>> +        return;
>>>> +
>>>>       dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
>>>>               ctrl->nr_reconnects);
>>>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>>>> work_struct *work)
>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>           goto out_fail;
>>>> +    nvme_tcp_reset_for_secure_concat(ctrl);
>>>
>>> This is really strange. I'd imagine that this would be needed only if 
>>> the derived psk expired no?
>>
>> You are absolutely correct. But once you reset the connection the 
>> derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
>> generated a new one.
>>
>> Remember: for secure concatenation we _always_ have a two-step 
>> connection setup. The initial connection runs for DH-HMAC-CHAP
>> to generate the PSK, and then a connection reset to start over
>> with TLS and the generated PSK.
>> So upon reset we have to invalidate the generated PSK, reset the
>> connection to run DH-HMAC-CHAP, and reset _again_ to start over
>> with the new PSK.
> 
> So what is the point of setting the derived psk with a timeout?
> 
> Seems rather silly doing that _every_ reset/reconnect... But ok...

Mandated by TP8018. Generated TLS PSK should be renewed after a
certain time. Makes sense in general, but still a pain.

Cheers,

Hannes