Re: [PATCH 2/9] nvme-keyring: add 'dhchap' key type

[Hannes Reinecke <hare@suse.de>]

On 11/26/25 08:46, Sagi Grimberg wrote:
> 
> 
> On 28/05/2025 17:05, Hannes Reinecke wrote:
>> Add a 'dhchap' keytype to store DH-HMAC-CHAP secret keys.
>> Keys are stored with a 'user-type' compatible payload, such
>> that one can use 'user_read()' to access the raw contents
>> and the 'read()' callback to get the base64-encoded key
>> data in the DH-HMAC-CHAP secret representation.
>>
>> Signed-off-by: Hannes Reinecke <hare@kernel.org>
>> ---
>>   drivers/nvme/common/keyring.c | 266 ++++++++++++++++++++++++++++++++++
>>   include/linux/nvme-keyring.h  |  22 ++-
>>   2 files changed, 287 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/ 
>> keyring.c
>> index 32d16c53133b..a58c93c6d495 100644
>> --- a/drivers/nvme/common/keyring.c
>> +++ b/drivers/nvme/common/keyring.c
>> @@ -4,6 +4,9 @@
>>    */
>>   #include <linux/module.h>
>> +#include <linux/crc32.h>
>> +#include <linux/base64.h>
>> +#include <linux/unaligned.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/key-type.h>
>>   #include <keys/user-type.h>
>> @@ -252,6 +255,262 @@ key_serial_t nvme_tls_psk_default(struct key 
>> *keyring,
>>   }
>>   EXPORT_SYMBOL_GPL(nvme_tls_psk_default);
>> +static void nvme_dhchap_psk_describe(const struct key *key, struct 
>> seq_file *m)
>> +{
>> +    seq_puts(m, key->description);
>> +    seq_printf(m, ": %u", key->datalen);
>> +}
>> +
>> +static bool nvme_dhchap_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;
>> +    }
>> +    if (!match_data->raw_data) {
>> +        pr_debug("%s: no match data\n", __func__);
>> +        return false;
>> +    }
>> +    match_id = match_data->raw_data;
>> +    match_len = strlen(match_id);
>> +    pr_debug("%s: match '%s' '%s' len %zd\n",
>> +         __func__, match_id, key->description, match_len);
>> +
>> +    return !memcmp(key->description, match_id, match_len);
>> +}
>> +
>> +static int nvme_dhchap_psk_match_preparse(struct key_match_data 
>> *match_data)
>> +{
>> +    match_data->lookup_type = KEYRING_SEARCH_LOOKUP_ITERATE;
>> +    match_data->cmp = nvme_dhchap_psk_match;
>> +    return 0;
>> +}
>> +
>> +/**
>> + * nvme_dhchap_psk_preparse - prepare DH-HMAC-CHAP key data
>> + * @prep: preparsed payload of the key data
>> + *
>> + * Decode the DH-HMAC-CHAP key data passed in in @prep and
>> + * store the resulting binary data. The binary data includes
>> + * space for the CRC, the version, and the hmac identifier,
>> + * but the data length is just the key data without the CRC.
>> + * This allows the user to read the key data via the
>> + * 'user_read()' function, but also to export the full
>> + * key data including CRC via the 'read' callback from the key type.
>> + */
>> +static int nvme_dhchap_psk_preparse(struct key_preparsed_payload *prep)
>> +{
>> +    struct user_key_payload *upayload;
>> +    size_t datalen = prep->datalen, keylen;
>> +    int ret;
>> +    u32 crc;
>> +    u8 version, hmac;
>> +
>> +    if (!prep->data) {
>> +        pr_debug("%s: Empty data", __func__);
>> +        prep->payload.data[0] = NULL;
>> +        prep->quotalen = 0;
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (sscanf(prep->data, "DHHC-%01hhu:%02hhu:%*s", &version, 
>> &hmac) != 2) {
>> +        pr_debug("%s: invalid key data '%s'\n", __func__,
>> +             (char *)prep->data);
>> +        prep->payload.data[0] = NULL;
>> +        prep->quotalen = 0;
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* skip header and final ':' character */
>> +    datalen -= 11;
>> +
>> +    /* keylen is the key length, 4 bytes CRC, 1 byte version, and 1 
>> byte HMAC identifier */
>> +    switch (datalen) {
>> +    case 48:
>> +        keylen = 38;
>> +        break;
>> +    case 72:
>> +        keylen = 54;
>> +        break;
>> +    case 92:
>> +        keylen = 70;
>> +        break;
>> +    default:
> 
> This feels like random arithmetic Hannes. Really really really hard to 
> review this code
> and understand if it is correct without trying to decipher the bits/ 
> bytes of the payload.
> 
> Can we introduce well documented macros/helpers to make this code more 
> explainable?
> 
Sure, will do.

>> +        pr_debug("%s: Invalid data length %lu\n", __func__, datalen);
>> +        prep->payload.data[0] = NULL;
>> +        prep->quotalen = 0;
>> +        return -EINVAL;
>> +    }
>> +
>> +    upayload = kzalloc(sizeof(*upayload) + keylen, GFP_KERNEL);
>> +    if (!upayload) {
>> +        prep->payload.data[0] = NULL;
>> +        prep->quotalen = 0;
>> +        return -ENOMEM;
>> +    }
>> +
>> +    /* decode the data */
>> +    prep->quotalen = keylen;
>> +    prep->payload.data[0] = upayload;
>> +    ret = base64_decode(prep->data + 10, datalen, upayload->data);
>> +    if (ret < 0) {
>> +        pr_debug("%s: Failed to decode key %s\n",
>> +             __func__, (char *)prep->data + 10);
>> +        return ret;
>> +    }
>> +    ret -= 4;
>> +    crc = ~crc32(~0, upayload->data, ret);
>> +    if (get_unaligned_le32(upayload->data + ret) != crc) {
>> +        pr_debug("%s: CRC mismatch for key\n", __func__);
>> +        /* CRC mismatch */
>> +        return -EKEYREJECTED;
>> +    }
>> +    upayload->data[ret + 4] = version;
>> +    upayload->data[ret + 5] = hmac;
> 
> Again some more inline random arithmetic.
> 
I described it briefly in the function description, but will
be updating it to give a detailed layout of the keys.

>> +    upayload->datalen = ret;
>> +    return 0;
>> +}
>> +
>> +static long nvme_dhchap_psk_read(const struct key *key, char *buffer, 
>> size_t buflen)
>> +{
>> +    const struct user_key_payload *upayload;
>> +    size_t datalen, keylen;
>> +    u8 version, hmac;
>> +    long ret;
>> +
>> +    upayload = user_key_payload_locked(key);
>> +    switch (upayload->datalen) {
>> +        case 32:
>> +        keylen = 59;
> 
> Some more random arithmetic...
> 
See above, will be moved into a helper function.

>> +        break;
>> +        case 48:
>> +        keylen = 83;
>> +        break;
>> +        case 64:
>> +        keylen = 103;
>> +        break;
>> +        default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!buffer || buflen == 0)
>> +        return keylen;
>> +
>> +    if (buflen < keylen)
>> +        return -EINVAL;
>> +
>> +    memset(buffer, 0, buflen);
>> +    version = upayload->data[upayload->datalen + 4];
>> +    hmac = upayload->data[upayload->datalen + 5];
>> +    ret = sprintf(buffer, "DHHC-%01hhu:%02hhu:", version, hmac);
>> +    if (ret < 0)
>> +        return -ENOKEY;
>> +    /* Include the trailing CRC */
>> +    datalen = upayload->datalen + 4;
>> +    ret += base64_encode(upayload->data, datalen, buffer + ret);
>> +    buffer[ret] = ':';
>> +    ret++;
>> +    return ret;
>> +}
>> +
>> +static struct key_type nvme_dhchap_psk_key_type = {
>> +    .name           = "dhchap",
>> +    .flags          = KEY_TYPE_NET_DOMAIN,
>> +    .preparse       = nvme_dhchap_psk_preparse,
>> +    .free_preparse  = user_free_preparse,
>> +    .match_preparse = nvme_dhchap_psk_match_preparse,
>> +    .instantiate    = generic_key_instantiate,
>> +    .revoke         = user_revoke,
>> +    .destroy        = user_destroy,
>> +    .describe       = nvme_dhchap_psk_describe,
>> +    .read           = nvme_dhchap_psk_read,
>> +};
>> +
>> +struct key *nvme_dhchap_psk_refresh(struct key *keyring,
>> +        const u8 *data, size_t data_len)
> 
> Is there a call-site to this? I don't see one...
> 
Hmm. I thought there was, but I'll check.
Will be moving it into the patch requiring it.

>> +{
>> +    key_perm_t keyperm =
>> +        KEY_POS_SEARCH | KEY_POS_VIEW | KEY_POS_READ |
>> +        KEY_POS_WRITE | KEY_POS_LINK | KEY_POS_SETATTR |
>> +        KEY_USR_SEARCH | KEY_USR_VIEW | KEY_USR_READ;
>> +    char *identity;
>> +    key_ref_t keyref;
>> +    key_serial_t keyring_id;
>> +    struct key *key;
>> +    uuid_t key_uuid;
>> +
>> +    if (data == NULL || !data_len)
>> +        return ERR_PTR(-EINVAL);
>> +
>> +    generate_random_uuid(key_uuid.b);
>> +    identity = kasprintf(GFP_KERNEL, "%pU", &key_uuid);
>> +    if (!identity)
>> +        return ERR_PTR(-ENOMEM);
>> +    if (!keyring)
>> +        keyring = nvme_keyring;
>> +    keyring_id = key_serial(keyring);
>> +
>> +    pr_debug("keyring %x generate dhchap psk '%s'\n",
>> +         keyring_id, identity);
>> +    keyref = key_create(make_key_ref(keyring, true),
>> +                "dhchap", identity, data, data_len,
>> +                keyperm, KEY_ALLOC_NOT_IN_QUOTA |
>> +                KEY_ALLOC_BUILT_IN |
>> +                KEY_ALLOC_BYPASS_RESTRICTION);
>> +    if (IS_ERR(keyref)) {
>> +        pr_warn("refresh dhchap psk '%s' failed, error %ld\n",
>> +            identity, PTR_ERR(keyref));
>> +        kfree(identity);
>> +        return ERR_PTR(-ENOKEY);
>> +    }
>> +    key = key_ref_to_ptr(keyref);
>> +    pr_debug("generated dhchap psk %08x\n", key_serial(key));
>> +    kfree(identity);
>> +    return key;
>> +}
>> +EXPORT_SYMBOL_GPL(nvme_dhchap_psk_refresh);
>> +
>> +struct key *nvme_dhchap_psk_lookup(struct key *keyring, const char 
>> *identity)
> 
> Same...
> 
See above. Will be moving it into the patch requiring it.

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