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

[Hannes Reinecke <hare@suse.de>]

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@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@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).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich