netdev - Re: [PATCH 18/18] nvmet-tcp: peek icreq before starting TLS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e743f924-7421-3f06-c916-075d79975b26@grimberg.me>
Date: Tue, 12 Sep 2023 15:55:34 +0300
From: Sagi Grimberg <sagi@...mberg.me>
To: Hannes Reinecke <hare@...e.de>, Christoph Hellwig <hch@....de>
Cc: Keith Busch <kbusch@...nel.org>, linux-nvme@...ts.infradead.org,
 Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH 18/18] nvmet-tcp: peek icreq before starting TLS



On 9/12/23 14:48, Hannes Reinecke wrote:
> On 9/12/23 13:32, Sagi Grimberg wrote:
>>
>>
>> On 8/24/23 17:39, Hannes Reinecke wrote:
>>> Incoming connection might be either 'normal' NVMe-TCP connections
>>> starting with icreq or TLS handshakes. To ensure that 'normal'
>>> connections can still be handled we need to peek the first packet
>>> and only start TLS handshake if it's not an icreq.
>>> With that we can lift the restriction to always set TREQ to
>>> 'required' when TLS1.3 is enabled.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@...e.de>
>>> ---
>>>   drivers/nvme/target/configfs.c | 25 +++++++++++---
>>>   drivers/nvme/target/nvmet.h    |  5 +++
>>>   drivers/nvme/target/tcp.c      | 61 +++++++++++++++++++++++++++++++---
>>>   3 files changed, 82 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/nvme/target/configfs.c 
>>> b/drivers/nvme/target/configfs.c
>>> index b780ce049163..9eed6e6765ea 100644
>>> --- a/drivers/nvme/target/configfs.c
>>> +++ b/drivers/nvme/target/configfs.c
>>> @@ -198,6 +198,20 @@ static ssize_t nvmet_addr_treq_store(struct 
>>> config_item *item,
>>>       return -EINVAL;
>>>   found:
>>> +    if (port->disc_addr.trtype == NVMF_TRTYPE_TCP &&
>>> +        port->disc_addr.tsas.tcp.sectype == NVMF_TCP_SECTYPE_TLS13) {
>>> +        switch (nvmet_addr_treq[i].type) {
>>> +        case NVMF_TREQ_NOT_SPECIFIED:
>>> +            pr_debug("treq '%s' not allowed for TLS1.3\n",
>>> +                 nvmet_addr_treq[i].name);
>>> +            return -EINVAL;
>>> +        case NVMF_TREQ_NOT_REQUIRED:
>>> +            pr_warn("Allow non-TLS connections while TLS1.3 is 
>>> enabled\n");
>>> +            break;
>>> +        default:
>>> +            break;
>>> +        }
>>> +    }
>>>       treq |= nvmet_addr_treq[i].type;
>>>       port->disc_addr.treq = treq;
>>>       return count;
>>> @@ -410,12 +424,15 @@ static ssize_t nvmet_addr_tsas_store(struct 
>>> config_item *item,
>>>       nvmet_port_init_tsas_tcp(port, sectype);
>>>       /*
>>> -     * The TLS implementation currently does not support
>>> -     * secure concatenation, so TREQ is always set to 'required'
>>> -     * if TLS is enabled.
>>> +     * If TLS is enabled TREQ should be set to 'required' per default
>>>        */
>>>       if (sectype == NVMF_TCP_SECTYPE_TLS13) {
>>> -        treq |= NVMF_TREQ_REQUIRED;
>>> +        u8 sc = nvmet_port_disc_addr_treq_secure_channel(port);
>>> +
>>> +        if (sc == NVMF_TREQ_NOT_SPECIFIED)
>>> +            treq |= NVMF_TREQ_REQUIRED;
>>> +        else
>>> +            treq |= sc;
>>>       } else {
>>>           treq |= NVMF_TREQ_NOT_SPECIFIED;
>>>       }
>>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>>> index e35a03260f45..3e179019ca7c 100644
>>> --- a/drivers/nvme/target/nvmet.h
>>> +++ b/drivers/nvme/target/nvmet.h
>>> @@ -184,6 +184,11 @@ static inline u8 
>>> nvmet_port_disc_addr_treq_secure_channel(struct nvmet_port *por
>>>       return (port->disc_addr.treq & NVME_TREQ_SECURE_CHANNEL_MASK);
>>>   }
>>> +static inline bool nvmet_port_secure_channel_required(struct 
>>> nvmet_port *port)
>>> +{
>>> +    return nvmet_port_disc_addr_treq_secure_channel(port) == 
>>> NVMF_TREQ_REQUIRED;
>>> +}
>>> +
>>>   struct nvmet_ctrl {
>>>       struct nvmet_subsys    *subsys;
>>>       struct nvmet_sq        **sqs;
>>> diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
>>> index 67fffa2e1e4a..5c1518a8bded 100644
>>> --- a/drivers/nvme/target/tcp.c
>>> +++ b/drivers/nvme/target/tcp.c
>>> @@ -1730,6 +1730,54 @@ static int nvmet_tcp_set_queue_sock(struct 
>>> nvmet_tcp_queue *queue)
>>>   }
>>>   #ifdef CONFIG_NVME_TARGET_TCP_TLS
>>
>> You need a stub for the other side of the ifdef?
> 
> No. The new function is completely encapsulated by 
> CONFIG_NVME_TARGET_TCP_TLS, so no need to add a stub.

Ok.