| 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
| ||
|
Message-ID: <f250deb1-90d9-f9c4-667f-9e6ad580cb6b@grimberg.me>
Date: Thu, 17 Aug 2023 12:20:38 +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 11/18] nvme-fabrics: parse options 'keyring' and 'tls_key'
On 8/16/23 15:06, Hannes Reinecke wrote:
> Parse the fabrics options 'keyring' and 'tls_key' and store the
> referenced keys in the options structure.
>
> Signed-off-by: Hannes Reinecke <hare@...e.de>
> Reviewed-by: Sagi Grimberg <sagi@...mberg.me>
> ---
> drivers/nvme/host/fabrics.c | 52 ++++++++++++++++++++++++++++++++++++-
> drivers/nvme/host/fabrics.h | 6 +++++
> drivers/nvme/host/tcp.c | 14 +++++++---
> 3 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index ddad482c3537..e453c3871cb1 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -622,6 +622,23 @@ static struct nvmf_transport_ops *nvmf_lookup_transport(
> return NULL;
> }
>
> +static struct key *nvmf_parse_key(int key_id, char *key_type)
> +{
> + struct key *key;
> +
> + if (!IS_ENABLED(CONFIG_NVME_TCP_TLS)) {
> + pr_err("TLS is not supported\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + key = key_lookup(key_id);
> + if (IS_ERR(key))
> + pr_err("%s %08x not found\n", key_type, key_id);
> + else
> + pr_debug("Using %s %08x\n", key_type, key_id);
I think that the key_type string is unneeded as an argument solely for
the log print.
I think that you can move the error print to the call-site. Keep
the debug print here, but you don't need the type...
> + return key;
> +}
> +
> static const match_table_t opt_tokens = {
> { NVMF_OPT_TRANSPORT, "transport=%s" },
> { NVMF_OPT_TRADDR, "traddr=%s" },
> @@ -643,6 +660,10 @@ static const match_table_t opt_tokens = {
> { NVMF_OPT_NR_WRITE_QUEUES, "nr_write_queues=%d" },
> { NVMF_OPT_NR_POLL_QUEUES, "nr_poll_queues=%d" },
> { NVMF_OPT_TOS, "tos=%d" },
> +#ifdef CONFIG_NVME_TCP_TLS
> + { NVMF_OPT_KEYRING, "keyring=%d" },
> + { NVMF_OPT_TLS_KEY, "tls_key=%d" },
> +#endif
> { NVMF_OPT_FAIL_FAST_TMO, "fast_io_fail_tmo=%d" },
> { NVMF_OPT_DISCOVERY, "discovery" },
> { NVMF_OPT_DHCHAP_SECRET, "dhchap_secret=%s" },
> @@ -660,9 +681,10 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
> char *options, *o, *p;
> int token, ret = 0;
> size_t nqnlen = 0;
> - int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO;
> + int ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO, key_id;
> uuid_t hostid;
> char hostnqn[NVMF_NQN_SIZE];
> + struct key *key;
>
> /* Set defaults */
> opts->queue_size = NVMF_DEF_QUEUE_SIZE;
> @@ -928,6 +950,32 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
> }
> opts->tos = token;
> break;
> + case NVMF_OPT_KEYRING:
> + if (match_int(args, &key_id) || key_id <= 0) {
> + ret = -EINVAL;
> + goto out;
> + }
> + key = nvmf_parse_key(key_id, "Keyring");
> + if (IS_ERR(key)) {
> + ret = PTR_ERR(key);
> + goto out;
> + }
> + key_put(opts->keyring);
Don't understand how keyring/tls_key are pre-populated though...
> + opts->keyring = key;
> + break;
> + case NVMF_OPT_TLS_KEY:
> + if (match_int(args, &key_id) || key_id <= 0) {
> + ret = -EINVAL;
> + goto out;
> + }
> + key = nvmf_parse_key(key_id, "Key");
> + if (IS_ERR(key)) {
> + ret = PTR_ERR(key);
> + goto out;
> + }
> + key_put(opts->tls_key);
> + opts->tls_key = key;
> + break;
> case NVMF_OPT_DISCOVERY:
> opts->discovery_nqn = true;
> break;
> @@ -1168,6 +1216,8 @@ static int nvmf_check_allowed_opts(struct nvmf_ctrl_options *opts,
> void nvmf_free_options(struct nvmf_ctrl_options *opts)
> {
> nvmf_host_put(opts->host);
> + key_put(opts->keyring);
> + key_put(opts->tls_key);
> kfree(opts->transport);
> kfree(opts->traddr);
> kfree(opts->trsvcid);
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index dac17c3fee26..fbaee5a7be19 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -71,6 +71,8 @@ enum {
> NVMF_OPT_DHCHAP_SECRET = 1 << 23,
> NVMF_OPT_DHCHAP_CTRL_SECRET = 1 << 24,
> NVMF_OPT_TLS = 1 << 25,
> + NVMF_OPT_KEYRING = 1 << 26,
> + NVMF_OPT_TLS_KEY = 1 << 27,
> };
>
> /**
> @@ -103,6 +105,8 @@ enum {
> * @dhchap_secret: DH-HMAC-CHAP secret
> * @dhchap_ctrl_secret: DH-HMAC-CHAP controller secret for bi-directional
> * authentication
> + * @keyring: Keyring to use for key lookups
> + * @tls_key: TLS key for encrypted connections (TCP)
> * @tls: Start TLS encrypted connections (TCP)
> * @disable_sqflow: disable controller sq flow control
> * @hdr_digest: generate/verify header digest (TCP)
> @@ -130,6 +134,8 @@ struct nvmf_ctrl_options {
> struct nvmf_host *host;
> char *dhchap_secret;
> char *dhchap_ctrl_secret;
> + struct key *keyring;
> + struct key *tls_key;
> bool tls;
> bool disable_sqflow;
> bool hdr_digest;
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index ef9cf8c7a113..f48797fcc4ee 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1589,6 +1589,8 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
>
> dev_dbg(nctrl->device, "queue %d: start TLS with key %x\n",
> qid, pskid);
> + if (nctrl->opts->keyring)
> + keyring = key_serial(nctrl->opts->keyring);
Maybe populate opts->keyring with nvme_keyring_id() to begin
with and then you don't need this?
> memset(&args, 0, sizeof(args));
> args.ta_sock = queue->sock;
> args.ta_done = nvme_tcp_tls_done;
> @@ -1914,9 +1916,12 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
> key_serial_t pskid = 0;
>
> if (ctrl->opts->tls) {
> - pskid = nvme_tls_psk_default(NULL,
> - ctrl->opts->host->nqn,
> - ctrl->opts->subsysnqn);
> + if (ctrl->opts->tls_key)
> + pskid = key_serial(ctrl->opts->tls_key);
> + else
> + pskid = nvme_tls_psk_default(ctrl->opts->keyring,
> + ctrl->opts->host->nqn,
> + ctrl->opts->subsysnqn);
> if (!pskid) {
> dev_err(ctrl->device, "no valid PSK found\n");
> ret = -ENOKEY;
> @@ -2776,7 +2781,8 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
> NVMF_OPT_HOST_TRADDR | NVMF_OPT_CTRL_LOSS_TMO |
> NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
> NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
> - NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS,
> + NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
> + NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
> .create_ctrl = nvme_tcp_create_ctrl,
> };
>
Powered by blists - more mailing lists