Re: [PATCH] nvme-tcp: send only permitted commands for secure concat

[Hannes Reinecke <hare@suse.de>]

On 9/11/25 15:07, Martin George wrote:
> On Wed, 2025-09-10 at 14:58 +0200, Hannes Reinecke wrote:
>> On 9/9/25 12:35, Martin George wrote:
>>> In addition to sending permitted commands such as connect/auth
>>> over the initial unencrypted admin connection as part of secure
>>> channel concatenation, the host also sends commands such as
>>> Property Get and Identify on the same. This is a spec violation
>>> leading to secure concat failures. Fix this by ensuring these
>>> additional commands are avoided on this connection.
>>>
>>> Fixes: 104d0e2f6222 ("nvme-fabrics: reset admin connection for
>>> secure concatenation")
>>> Signed-off-by: Martin George <marting@netapp.com>
>>> ---
>>>    drivers/nvme/host/tcp.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>> index c0fe8cfb7229..1413788ca7d5 100644
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>> @@ -2250,6 +2250,9 @@ static int
>>> nvme_tcp_configure_admin_queue(struct nvme_ctrl *ctrl, bool new)
>>>    	if (error)
>>>    		goto out_cleanup_tagset;
>>>    
>>> +	if (ctrl->opts->concat && !ctrl->tls_pskid)
>>> +		return 0;
>>> +
>>>    	error = nvme_enable_ctrl(ctrl);
>>>    	if (error)
>>>    		goto out_stop_queue;
>>
>> Hmm. Not sure. While section 8.3.4.3 'NVMe In-band Authentication'
>> states:
>>
>>    If one or more of the bits in the AUTHREQ field are set to ‘1’,
>> then
>>    the controller requires that the host authenticate on that queue in
>>    order to proceed with Fabrics, Admin, and I/O commands.
>>
>>   From which one could assume that none of the commands are allowed.
>> But it then goes on to state:
>>
>>    The state of an in-progress authentication transaction is soft-
>> state.
>>    If the subsequent command in an authentication transaction is not
>>    received by the controller within a timeout equal to:
>>     * the Keep Alive Timeout value (refer to Figure 546), if the Keep
>>       Alive Timer is enabled; or
>>     * the default Keep Alive Timeout value (i.e., two minutes), if the
>>       Keep Alive Timer is disabled;
>>
>> one can imply that KATO is running during authentication
>> transactions.
>> But that might require additional commands, so really I'm not sure.
>> Let me ask FMDS for clarification.
>>
> 
> This is specific to secure concat alone, and not otherwise.
> 
> NVMe base spec 2.1 section 8.3.4.1 Fabric Secure Channel states that
> "An NVM subsystem that requires use of a fabric secure channel (i.e.,
> as indicated by the TSC field in the associated Discovery Log Page
> Entry) shall not allow capsules to be transferred until a secure
> channel has been established for the NVMe Transport connection."
> 
> And again in section 8.3.4.3 Secure Channel Concatenation, "This PSK is
> generated by an authentication transaction on an Admin Queue over an
> unsecure channel. Once the authentication transaction is completed,
> that Admin Queue transport connection shall be disconnected by the
> host. The generated PSK may then be used to set up secure channels for
> subsequent Admin Queue(s) and I/O Queues."
> 
> So once authentication is completed on the unencrypted admin connection
> as part of secure concat, the host is expected to immediately
> disconnect from that and then proceed with setting up subsequent
> encrypted admin & I/O connections i.e. no reason for the host to
> continue with nvme_enable_ctrl() & nvme_init_ctrl_finish() on the
> initial unsecure connection as that's futile and doesn't serve any
> purpose. And that's what this patch is attempting to do above.
> 
Right you are.
I stand corrected.

Reviewed-by: Hannes Reinecke <hare@suse.de>

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