Re: [PATCH 02/18] nvme-keyring: define a 'psk' keytype

Sagi Grimberg
2023-03-22T08:49:04Z

>> On 3/21/23 14:43, Hannes Reinecke wrote:
>>> Define a 'psk' keytype to hold the NVMe TLS PSKs.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/nvme/common/keyring.c | 96 +++++++++++++++++++++++++++++++++++
>>>   include/linux/nvme-keyring.h  |  8 +++
>>>   2 files changed, 104 insertions(+)
>>>
>>> diff --git a/drivers/nvme/common/keyring.c
>>> b/drivers/nvme/common/keyring.c
>>> index 3a6e8a0b38e2..6cbb9d66e0f6 100644
>>> --- a/drivers/nvme/common/keyring.c
>>> +++ b/drivers/nvme/common/keyring.c
>>> @@ -11,6 +11,96 @@
>>>   static struct key *nvme_keyring;
>>> +key_serial_t nvme_keyring_id(void)
>>> +{
>>> +    return nvme_keyring->serial;
>>> +}
>>> +EXPORT_SYMBOL_GPL(nvme_keyring_id);
>>> +
>>> +static void nvme_tls_psk_describe(const struct key *key, struct
>>> seq_file *m)
>>> +{
>>> +    seq_puts(m, key->description);
>>> +    seq_printf(m, ": %u", key->datalen);
>>> +}
>>> +
>>> +static bool nvme_tls_psk_match(const struct key *key,
>>> +                   const struct key_match_data *match_data)
>>> +{
>>> +    const char *match_id;
>>> +    size_t match_len;
>>> +
>>> +    if (!key->description) {
>>> +        pr_debug("%s: no key description\n", __func__);
>>> +        return false;
>>> +    }
>>> +    match_len = strlen(key->description);
>>> +    pr_debug("%s: id %s len %zd\n", __func__, key->description,
>>> match_len);
>>> +
>>> +    if (!match_data->raw_data) {
>>> +        pr_debug("%s: no match data\n", __func__);
>>> +        return false;
>>> +    }
>>> +    match_id = match_data->raw_data;
>>> +    pr_debug("%s: match '%s' '%s' len %lu\n",
>>> +         __func__, match_id, key->description, match_len);
>>> +    return !memcmp(key->description, match_id, match_len);
>>> +}
>>> +
>>> +static int nvme_tls_psk_match_preparse(struct key_match_data
>>> *match_data)
>>> +{
>>> +    match_data->lookup_type = KEYRING_SEARCH_LOOKUP_ITERATE;
>>> +    match_data->cmp = nvme_tls_psk_match;
>>> +    return 0;
>>> +}
>>> +
>>> +static struct key_type nvme_tls_psk_key_type = {
>>> +    .name           = "psk",
>>> +    .flags          = KEY_TYPE_NET_DOMAIN,
>>> +    .preparse       = user_preparse,
>>> +    .free_preparse  = user_free_preparse,
>>> +    .match_preparse = nvme_tls_psk_match_preparse,
>>> +    .instantiate    = generic_key_instantiate,
>>> +    .revoke         = user_revoke,
>>> +    .destroy        = user_destroy,
>>> +    .describe       = nvme_tls_psk_describe,
>>> +    .read           = user_read,
>>> +};
>>> +
>>
>> Hannes, can you please provide a documentation section
>> to this function? most importantly 'generated' argument.
>>
> Sure. There are two types of PSK types specified in the NVMe TCP spec;
> a 'retained' one which has to be provided by the admin, and a
> 'generated' one which is derived from the shared key material from
> DH-HMAC-CHAP when doing a secure channel concatenation.
> And each type has two possible hash algorithms (SHA-256 and SHA-384),
> resulting in 4 possible PSKs.
>
> And indeed, the secure channel concatenation bits are missing as we're
> still hashing out details at the fmds group.
> I do have a patchset for that, though, but decided not to include it in
> this submission as it'll increase the patchset even more.
> Can do if you like ...

I'd prefer to split the secure channel concatenation out of this.
But it would be good to give some context in the code.

Also the bool is confusing, can you make this an enumeration that
translates to 'G' or 'R' and pass that in?

>
> But will be updating the documentation.
>
>>> +struct key *nvme_tls_psk_lookup(key_ref_t keyring,
>>> +        const char *hostnqn, const char *subnqn,
>>> +        int hmac, bool generated)
>>> +{
>>> +    char *identity;
>>> +    size_t identity_len = (NVMF_NQN_SIZE) * 2 + 11;
>>> +    key_ref_t keyref;
>>> +    key_serial_t keyring_id;
>>> +
>>> +    identity = kzalloc(identity_len, GFP_KERNEL);
>>> +    if (!identity)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    snprintf(identity, identity_len, "NVMe0%c%02d %s %s",
>>> +         generated ? 'G' : 'R', hmac, hostnqn, subnqn);
>>
>> Is that a format that is expected from userspace to produce?
>>
> Yes. See
> NVMe-TCP 1.0a section 3.6.1.3 "TLS PSK and PSK Identity Derivation"
> for details.
>

Yes, I see it now. Thanks.