| Message ID | 20230824143925.9098-19-hare@suse.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | nvme: In-kernel TLS support for TCP | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
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@suse.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?
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@suse.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
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@suse.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.
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 +static int nvmet_tcp_try_peek_pdu(struct nvmet_tcp_queue *queue) +{ + struct nvme_tcp_hdr *hdr = &queue->pdu.cmd.hdr; + int len, ret; + struct kvec iov = { + .iov_base = (u8 *)&queue->pdu + queue->offset, + .iov_len = sizeof(struct nvme_tcp_hdr), + }; + char cbuf[CMSG_LEN(sizeof(char))] = {}; + struct msghdr msg = { + .msg_control = cbuf, + .msg_controllen = sizeof(cbuf), + .msg_flags = MSG_PEEK, + }; + + if (nvmet_port_secure_channel_required(queue->port->nport)) + return 0; + + len = kernel_recvmsg(queue->sock, &msg, &iov, 1, + iov.iov_len, msg.msg_flags); + if (unlikely(len < 0)) { + pr_debug("queue %d: peek error %d\n", + queue->idx, len); + return len; + } + + ret = nvmet_tcp_tls_record_ok(queue, &msg, cbuf); + if (ret < 0) + return ret; + + if (len < sizeof(struct nvme_tcp_hdr)) { + pr_debug("queue %d: short read, %d bytes missing\n", + queue->idx, (int)iov.iov_len - len); + return -EAGAIN; + } + pr_debug("queue %d: hdr type %d hlen %d plen %d size %d\n", + queue->idx, hdr->type, hdr->hlen, hdr->plen, + (int)sizeof(struct nvme_tcp_icreq_pdu)); + if (hdr->type == nvme_tcp_icreq && + hdr->hlen == sizeof(struct nvme_tcp_icreq_pdu) && + hdr->plen == (__le32)sizeof(struct nvme_tcp_icreq_pdu)) { + pr_debug("queue %d: icreq detected\n", + queue->idx); + return len; + } + return 0; +} + static void nvmet_tcp_tls_handshake_done(void *data, int status, key_serial_t peerid) { @@ -1876,11 +1924,14 @@ static void nvmet_tcp_alloc_queue(struct nvmet_tcp_port *port, sk->sk_user_data = NULL; sk->sk_data_ready = port->data_ready; read_unlock_bh(&sk->sk_callback_lock); - if (!nvmet_tcp_tls_handshake(queue)) - return; - - /* TLS handshake failed, terminate the connection */ - goto out_destroy_sq; + if (!nvmet_tcp_try_peek_pdu(queue)) { + if (!nvmet_tcp_tls_handshake(queue)) + return; + /* TLS handshake failed, terminate the connection */ + goto out_destroy_sq; + } + /* Not a TLS connection, continue with normal processing */ + queue->state = NVMET_TCP_Q_CONNECTING; } #endif
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@suse.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(-)