Re: [PATCH 12/16] nvme-tcp: request secure channel concatenation

[Hannes Reinecke <hare@suse.de>]

On 7/18/24 00:31, Sagi Grimberg wrote:
> 
> 
> On 17/07/2024 12:10, Hannes Reinecke wrote:
>> Add a fabrics option 'concat' to request secure channel concatenation.
>> When secure channel concatenation is enabled a 'generated PSK' is 
>> inserted
>> into the keyring such that it's available after reset.
>>
>> Signed-off-by: Hannes Reinecke <hare@kernel.org>
>> ---
>>   drivers/nvme/host/auth.c    | 105 ++++++++++++++++++++++++++++++++++--
>>   drivers/nvme/host/fabrics.c |  34 ++++++++++--
>>   drivers/nvme/host/fabrics.h |   3 ++
>>   drivers/nvme/host/sysfs.c   |   2 +-
>>   drivers/nvme/host/tcp.c     |  54 ++++++++++++++++---
>>   include/linux/nvme.h        |   7 +++
>>   6 files changed, 191 insertions(+), 14 deletions(-)
>>
[ .. ]
>> @@ -831,10 +911,21 @@ static void nvme_queue_auth_work(struct 
>> work_struct *work)
>>           if (ret)
>>               chap->error = ret;
>>       }
>> -    if (!ret) {
>> +    if (ret)
>> +        goto fail2;
>> +    if (chap->qid || !ctrl->opts->concat) {
> 
> Please add a comment here on why you are doing this.
> 
Okay.

>>           chap->error = 0;
>>           return;
>>       }
>> +    ret = nvme_auth_secure_concat(ctrl, chap);
>> +    if (ret) {
>> +        dev_warn(ctrl->device,
>> +             "%s: qid %d failed to enable secure concatenation\n",
>> +             __func__, chap->qid);
>> +        chap->error = ret;
>> +    } else
>> +        chap->error = 0;
>> +    return;
>>   fail2:
>>       if (chap->status == 0)
>> @@ -912,6 +1003,12 @@ static void nvme_ctrl_auth_work(struct 
>> work_struct *work)
>>                "qid 0: authentication failed\n");
>>           return;
>>       }
>> +    /*
>> +    * Only run authentication on the admin queue for
>> +    * secure concatenation
>> +     */
> 
> Seems like you got some whitespaces misalignment, I'd pass the patchset
> through checkpatch.pl
> 
Ok.

>> +    if (ctrl->opts->concat)
>> +        return;
>>       for (q = 1; q < ctrl->queue_count; q++) {
>>           ret = nvme_auth_negotiate(ctrl, q);
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index c62d8890f3a8..f83ac1292a3b 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -472,8 +472,9 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
>>       result = le32_to_cpu(res.u32);
>>       ctrl->cntlid = result & 0xFFFF;
>>       if (result & (NVME_CONNECT_AUTHREQ_ATR | 
>> NVME_CONNECT_AUTHREQ_ASCR)) {
>> -        /* Secure concatenation is not implemented */
>> -        if (result & NVME_CONNECT_AUTHREQ_ASCR) {
>> +        /* Check for secure concatenation */
>> +        if ((result & NVME_CONNECT_AUTHREQ_ASCR) &&
>> +            !ctrl->opts->concat) {
> 
> Shouldn't you check for the dhchap_secret here instead of concat?
> 
I _thought_ I had checked for valid option combinations during options
parsing, ie at this position we already know that we have a valid option
combination, and the dhchap_secret will always be set when 'concat is 
enabled.

>>               dev_warn(ctrl->device,
>>                    "qid 0: secure concatenation is not supported\n");
>>               ret = -EOPNOTSUPP;
>> @@ -550,7 +551,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, 
>> u16 qid)
>>           /* Secure concatenation is not implemented */
>>           if (result & NVME_CONNECT_AUTHREQ_ASCR) {
>>               dev_warn(ctrl->device,
>> -                 "qid 0: secure concatenation is not supported\n");
>> +                 "qid %d: secure concatenation is not supported\n", 
>> qid);
>>               ret = -EOPNOTSUPP;
>>               goto out_free_data;
>>           }
>> @@ -706,6 +707,7 @@ static const match_table_t opt_tokens = {
>>   #endif
>>   #ifdef CONFIG_NVME_TCP_TLS
>>       { NVMF_OPT_TLS,            "tls"            },
>> +    { NVMF_OPT_CONCAT,        "concat"        },
>>   #endif
>>       { NVMF_OPT_ERR,            NULL            }
>>   };
>> @@ -735,6 +737,7 @@ static int nvmf_parse_options(struct 
>> nvmf_ctrl_options *opts,
>>       opts->tls = false;
>>       opts->tls_key = NULL;
>>       opts->keyring = NULL;
>> +    opts->concat = false;
>>       options = o = kstrdup(buf, GFP_KERNEL);
>>       if (!options)
>> @@ -1053,6 +1056,14 @@ static int nvmf_parse_options(struct 
>> nvmf_ctrl_options *opts,
>>               }
>>               opts->tls = true;
>>               break;
>> +        case NVMF_OPT_CONCAT:
>> +            if (!IS_ENABLED(CONFIG_NVME_TCP_TLS)) {
>> +                pr_err("TLS is not supported\n");
>> +                ret = -EINVAL;
>> +                goto out;
>> +            }
>> +            opts->concat = true;
>> +            break;
>>           default:
>>               pr_warn("unknown parameter or missing value '%s' in ctrl 
>> creation request\n",
>>                   p);
>> @@ -1079,6 +1090,23 @@ static int nvmf_parse_options(struct 
>> nvmf_ctrl_options *opts,
>>               pr_warn("failfast tmo (%d) larger than controller loss 
>> tmo (%d)\n",
>>                   opts->fast_io_fail_tmo, ctrl_loss_tmo);
>>       }
>> +    if (opts->concat) {
>> +        if (opts->tls) {
>> +            pr_err("Secure concatenation over TLS is not supported\n");
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +        if (opts->tls_key) {
>> +            pr_err("Cannot specify a TLS key for secure 
>> concatenation\n");
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +        if (!opts->dhchap_secret) {
>> +            pr_err("Need to enable DH-CHAP for secure concatenation\n");
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +    }
>>       opts->host = nvmf_host_add(hostnqn, &hostid);
>>       if (IS_ERR(opts->host)) {
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index 21d75dc4a3a0..9cf5b020adba 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -66,6 +66,7 @@ enum {
>>       NVMF_OPT_TLS        = 1 << 25,
>>       NVMF_OPT_KEYRING    = 1 << 26,
>>       NVMF_OPT_TLS_KEY    = 1 << 27,
>> +    NVMF_OPT_CONCAT        = 1 << 28,
>>   };
>>   /**
>> @@ -101,6 +102,7 @@ enum {
>>    * @keyring:    Keyring to use for key lookups
>>    * @tls_key:    TLS key for encrypted connections (TCP)
>>    * @tls:        Start TLS encrypted connections (TCP)
>> + * @concat:     Enabled Secure channel concatenation (TCP)
>>    * @disable_sqflow: disable controller sq flow control
>>    * @hdr_digest: generate/verify header digest (TCP)
>>    * @data_digest: generate/verify data digest (TCP)
>> @@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
>>       struct key        *keyring;
>>       struct key        *tls_key;
>>       bool            tls;
>> +    bool            concat;
>>       bool            disable_sqflow;
>>       bool            hdr_digest;
>>       bool            data_digest;
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 462d71e2fbf8..5350eb87ec52 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -683,7 +683,7 @@ static ssize_t tls_configured_key_show(struct 
>> device *dev,
>>       struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>       struct key *key = ctrl->opts->tls_key;
>> -    if (!key)
>> +    if (!key || ctrl->opts->concat)
>>           return 0;
> 
> Same comment - move to visible check.
> 
Yeah, okay.

>>       return sysfs_emit(buf, "%08x\n", key_serial(key));
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 5885aa452aa1..d6c085ba0114 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -227,7 +227,7 @@ static inline bool nvme_tcp_tls_configured(struct 
>> nvme_ctrl *ctrl)
>>       if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
>>           return 0;
>> -    return ctrl->opts->tls;
>> +    return ctrl->opts->tls || ctrl->opts->concat;
> 
> I'm wandering if configured/enabled is the best naming...
> Maybe move it to a different check helper nvme_tcp_tls_concatenated() ?
> 
nvme_tcp_tls_configured() is a check to figure out whether TLS handshake 
should be attempted. So adding the check for secure concatenation here
seemed logical, and reduced code churn.

> btw, shouldn't tls + concat always be passed together? It is getting 
> confusing...
> 
--tls enables TLS _before_ the connect command is sent, --concat
enables TLS _after_ the connect command is sent.
The combination of '--tls' and '--concat' would set up a TLS connection,
and then run secure concatenation over TLS.
The spec does allow for that, but I haven't implemented it.

But that's not to say it will never be implemented, so I left this
option open for later and did keep both options separate.

>>   }
>>   static inline struct blk_mq_tags *nvme_tcp_tagset(struct 
>> nvme_tcp_queue *queue)
>> @@ -1942,7 +1942,7 @@ static int nvme_tcp_alloc_admin_queue(struct 
>> nvme_ctrl *ctrl)
>>       if (nvme_tcp_tls_configured(ctrl)) {
>>           if (ctrl->opts->tls_key)
>>               pskid = key_serial(ctrl->opts->tls_key);
>> -        else {
>> +        else if (ctrl->opts->tls) {
>>               pskid = nvme_tls_psk_default(ctrl->opts->keyring,
>>                                 ctrl->opts->host->nqn,
>>                                 ctrl->opts->subsysnqn);
>> @@ -1972,9 +1972,25 @@ static int __nvme_tcp_alloc_io_queues(struct 
>> nvme_ctrl *ctrl)
>>   {
>>       int i, ret;
>> -    if (nvme_tcp_tls_configured(ctrl) && !ctrl->tls_pskid) {
>> -        dev_err(ctrl->device, "no PSK negotiated\n");
>> -        return -ENOKEY;
>> +    if (nvme_tcp_tls_configured(ctrl)) {
>> +        if (ctrl->opts->concat) {
>> +            /*
>> +             * The generated PSK is stored in the
>> +             * fabric options
>> +             */
>> +            if (!ctrl->opts->tls_key) {
>> +                dev_err(ctrl->device, "no PSK generated\n");
>> +                return -ENOKEY;
>> +            }
>> +            if (ctrl->tls_pskid &&
>> +                ctrl->tls_pskid != key_serial(ctrl->opts->tls_key)) {
>> +                dev_err(ctrl->device, "Stale PSK id %08x\n", 
>> ctrl->tls_pskid);
>> +                ctrl->tls_pskid = 0;
>> +            }
>> +        } else if (!ctrl->tls_pskid) {
>> +            dev_err(ctrl->device, "no PSK negotiated\n");
>> +            return -ENOKEY;
>> +        }
>>       }
>>       for (i = 1; i < ctrl->queue_count; i++) {
>> @@ -2205,6 +2221,27 @@ static void nvme_tcp_reconnect_or_remove(struct 
>> nvme_ctrl *ctrl,
>>       }
>>   }
>> +static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
>> +{
>> +    if (!ctrl->opts->concat)
>> +        return;
>> +    /* No key generated, nothing to do */
>> +    if (!ctrl->opts->tls_key)
>> +        return;
>> +    /* TLS is not enabled, do not wipe the key */
>> +    if (!ctrl->tls_pskid)
>> +        return;
> 
> Gotta say I usually dislike functions that are called and either do
> or don't do stuff based on internals.
> 
> But if you must, please add _if_needed suffix to this function.
> 
I thought a helper would be good here, but I can easily split off
the checks into a different function (nvme_tcp_key_revoke_needed?).

>> +    /*
>> +     * Key generated, and TLS enabled:
>> +     * Revoke the generated key.
>> +     */
>> +    dev_dbg(ctrl->device, "Wipe generated TLS PSK %08x\n",
>> +        key_serial(ctrl->opts->tls_key));
>> +    key_revoke(ctrl->opts->tls_key);
>> +    key_put(ctrl->opts->tls_key);
>> +    ctrl->opts->tls_key = NULL;
>> +}
>> +
>>   static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>>   {
>>       struct nvmf_ctrl_options *opts = ctrl->opts;
>> @@ -2308,6 +2345,7 @@ static void nvme_tcp_error_recovery_work(struct 
>> work_struct *work)
>>                   struct nvme_tcp_ctrl, err_work);
>>       struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
>> +    nvme_tcp_revoke_generated_tls_key(ctrl);
>>       nvme_stop_keep_alive(ctrl);
>>       flush_work(&ctrl->async_event_work);
>>       nvme_tcp_teardown_io_queues(ctrl, false);
>> @@ -2348,6 +2386,7 @@ static void nvme_reset_ctrl_work(struct 
>> work_struct *work)
>>           container_of(work, struct nvme_ctrl, reset_work);
>>       int ret;
>> +    nvme_tcp_revoke_generated_tls_key(ctrl);
>>       nvme_stop_ctrl(ctrl);
>>       nvme_tcp_teardown_ctrl(ctrl, false);
>> @@ -2638,6 +2677,9 @@ static int nvme_tcp_get_address(struct nvme_ctrl 
>> *ctrl, char *buf, int size)
>>       len = nvmf_get_address(ctrl, buf, size);
>> +    if (ctrl->state != NVME_CTRL_LIVE)
>> +        return len;
>> +
> 
> This looks unrelated.
> 
Indeed. Will be removing it

>>       mutex_lock(&queue->queue_lock);
>>       if (!test_bit(NVME_TCP_Q_LIVE, &queue->flags))
>> @@ -2842,7 +2884,7 @@ static struct nvmf_transport_ops 
>> nvme_tcp_transport = {
>>                 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_KEYRING | NVMF_OPT_TLS_KEY,
>> +              NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
>>       .create_ctrl    = nvme_tcp_create_ctrl,
>>   };
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index c12a329dd463..ef85cf69cf99 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
>> @@ -1669,6 +1669,13 @@ enum {
>>       NVME_AUTH_DHGROUP_INVALID    = 0xff,
>>   };
>> +enum {
>> +    NVME_AUTH_SECP_NOSC        = 0x00,
>> +    NVME_AUTH_SECP_SC        = 0x01,
>> +    NVME_AUTH_SECP_NEWTLSPSK    = 0x02,
>> +    NVME_AUTH_SECP_REPLACETLSPSK    = 0x03,
>> +};
>> +
>>   union nvmf_auth_protocol {
>>       struct nvmf_auth_dhchap_protocol_descriptor dhchap;
>>   };
> 

Thanks for the review!

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich