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 for Android: free password hash cracker in your pocket

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

Message-ID: <94fc9502-c743-4f6c-a082-40e326085426@suse.de>
Date: Tue, 12 Sep 2023 13:48:42 +0200
From: Hannes Reinecke <hare@...e.de>
To: Sagi Grimberg <sagi@...mberg.me>, 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 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.

Cheers,

Hannes