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: <b545b658-0771-6c53-fc9d-a69e452909c1@grimberg.me>
Date: Tue, 12 Sep 2023 14:32:10 +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 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?