netdev - Re: [PATCH 11/18] nvme-fabrics: parse options 'keyring' and '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: <615aaad8-b216-ceaa-1553-d283a3c4531b@grimberg.me>
Date: Thu, 17 Aug 2023 13:41:02 +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'


>>> +        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...
>>
> They are not. But they might, as there's nothing in the code preventing 
> the user to specify 'keyring' or 'tls_key' several times.
> I can make it one-shot and error out if they are already populated, but
> really haven't seen the need.

OK, now I understand. its fine.

> 
>>> +            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?
>>
> We could; one would lose the distinction between 'not specified' and
> 'specified with the defaul keyring', but one could argue whether that
> brings any benefit.

Not sure I understand.
What exactly is the distinction and how is it manifested?