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

[Sagi Grimberg]

On 21/04/2024 17:37, Hannes Reinecke wrote:
> On 4/21/24 13:22, Sagi Grimberg wrote:
>>
>>
>> On 09/04/2024 1:10, Hannes Reinecke wrote:
>>> 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.
>>
>> I understand that psk can expire, I don't understand why we need to 
>> reset every
>> single time. What if I'm doing the loop: while true; do nvme reset 
>> /dev/nvme0; done?
>>
>> Will it do 2 resets every time?
>
> For secure concatenation: yes.
>
> Secure concatenation consists on running DH-HMAC-CHAP, generate a TLS 
> PSK from the generated credentials, and then start TLS with the 
> generated TLS PSK. This is run for every connection attempt, so each
> connection attempt gets a new TLS PSK; after the connection is reset
> the old PSK _cannot_ be reused, and we need to clear them up via
> some sort of garbage collection. By adding an expiration time to the
> key we achieve exactly this.
> And that is why we need to reset twice; we need to run the DH-HMAC-CHAP
> protocol _after_ recovery, and use the generated keys to establish a
> TLS connection. And as the TLS connection is established before the
> initial connect is sent we need to reset the connection after the
> DH-HMAC-CHAP protocol is run (as this is run _after_ the connect 
> command).

Got it.