[PATCH] nvme: add sysfs attribute 'tls_keyring'

[PATCH] nvme: add sysfs attribute 'tls_keyring'

[Hannes Reinecke]

Add a sysfs attribute 'tls_keyring' to hold the contents for the
'keyring' fabrics option. This option is only meaningful if the
'tls' option has been specified; if the 'tls_key' option is specified
the key is looked up directly and the keyring setting is ignored.
So blank out the keyring value when 'tls_key' is specified.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/sysfs.c | 55 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 6c7f1d5c056f..7b65ff7426c9 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -5,11 +5,12 @@
  * Copyright (c) 2011-2014, Intel Corporation.
  */
 
-#include <linux/nvme-auth.h>
-
 #include "nvme.h"
 #include "fabrics.h"
 
+#include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
+
 static ssize_t nvme_sysfs_reset(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -677,6 +678,52 @@ static ssize_t tls_key_show(struct device *dev,
 	return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
 }
 static DEVICE_ATTR_RO(tls_key);
+
+static ssize_t tls_keyring_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	struct key *keyring;
+
+	if (!ctrl->tls_key)
+		return 0;
+	/* Do not display the keyring for user-selected keys */
+	if (ctrl->opts->tls_key)
+		return 0;
+	keyring = ctrl->opts->keyring;
+	if (!keyring) {
+		keyring = key_lookup(nvme_keyring_id());
+		if (!keyring)
+			return -EINVAL;
+	}
+	return sysfs_emit(buf, "%s", keyring->description);
+}
+
+static ssize_t tls_keyring_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	int err;
+	u32 key_id;
+	struct key *keyring;
+
+	if (!strlen(buf))
+		return -EINVAL;
+	err = kstrtou32(buf, 0, &key_id);
+	if (err)
+		return err;
+	keyring = key_lookup(key_id);
+	if (IS_ERR(keyring)) {
+		pr_err("key %08x not found\n", key_id);
+		return -EINVAL;
+	}
+	if (ctrl->opts->keyring)
+		key_put(ctrl->opts->keyring);
+	ctrl->opts->keyring = keyring;
+	return 0;
+}
+static DEVICE_ATTR(tls_keyring, S_IRUGO | S_IWUSR,
+		tls_keyring_show, tls_keyring_store);
 #endif
 
 static struct attribute *nvme_dev_attrs[] = {
@@ -708,6 +755,7 @@ static struct attribute *nvme_dev_attrs[] = {
 #endif
 #ifdef CONFIG_NVME_TCP_TLS
 	&dev_attr_tls_key.attr,
+	&dev_attr_tls_keyring.attr,
 #endif
 	&dev_attr_adm_passthru_err_log_enabled.attr,
 	NULL
@@ -743,6 +791,9 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
 	if (a == &dev_attr_tls_key.attr &&
 	    (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
 		return 0;
+	if (a == &dev_attr_tls_keyring.attr &&
+	    (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
+		return 0;
 #endif
 
 	return a->mode;
-- 
2.35.3

Re: [PATCH] nvme: add sysfs attribute 'tls_keyring'

[Sagi Grimberg]

On 08/03/2024 14:10, Hannes Reinecke wrote:
> Add a sysfs attribute 'tls_keyring' to hold the contents for the
> 'keyring' fabrics option. This option is only meaningful if the
> 'tls' option has been specified; if the 'tls_key' option is specified
> the key is looked up directly and the keyring setting is ignored.
> So blank out the keyring value when 'tls_key' is specified.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/sysfs.c | 55 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 6c7f1d5c056f..7b65ff7426c9 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -5,11 +5,12 @@
>    * Copyright (c) 2011-2014, Intel Corporation.
>    */
>   
> -#include <linux/nvme-auth.h>
> -
>   #include "nvme.h"
>   #include "fabrics.h"
>   
> +#include <linux/nvme-auth.h>
> +#include <linux/nvme-keyring.h>
> +
>   static ssize_t nvme_sysfs_reset(struct device *dev,
>   				struct device_attribute *attr, const char *buf,
>   				size_t count)
> @@ -677,6 +678,52 @@ static ssize_t tls_key_show(struct device *dev,
>   	return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
>   }
>   static DEVICE_ATTR_RO(tls_key);
> +
> +static ssize_t tls_keyring_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +	struct key *keyring;
> +
> +	if (!ctrl->tls_key)
> +		return 0;
> +	/* Do not display the keyring for user-selected keys */
> +	if (ctrl->opts->tls_key)
> +		return 0;
> +	keyring = ctrl->opts->keyring;
> +	if (!keyring) {
> +		keyring = key_lookup(nvme_keyring_id());
> +		if (!keyring)
> +			return -EINVAL;

Weird if this failed...

> +	}
> +	return sysfs_emit(buf, "%s", keyring->description);
> +}
> +
> +static ssize_t tls_keyring_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +	int err;
> +	u32 key_id;
> +	struct key *keyring;
> +
> +	if (!strlen(buf))
> +		return -EINVAL;
> +	err = kstrtou32(buf, 0, &key_id);
> +	if (err)
> +		return err;
> +	keyring = key_lookup(key_id);
> +	if (IS_ERR(keyring)) {
> +		pr_err("key %08x not found\n", key_id);
> +		return -EINVAL;
> +	}
> +	if (ctrl->opts->keyring)
> +		key_put(ctrl->opts->keyring);
> +	ctrl->opts->keyring = keyring;
> +	return 0;
> +}

Why should there be a _store() for this?

> +static DEVICE_ATTR(tls_keyring, S_IRUGO | S_IWUSR,
> +		tls_keyring_show, tls_keyring_store);
>   #endif
>   
>   static struct attribute *nvme_dev_attrs[] = {
> @@ -708,6 +755,7 @@ static struct attribute *nvme_dev_attrs[] = {
>   #endif
>   #ifdef CONFIG_NVME_TCP_TLS
>   	&dev_attr_tls_key.attr,
> +	&dev_attr_tls_keyring.attr,
>   #endif
>   	&dev_attr_adm_passthru_err_log_enabled.attr,
>   	NULL
> @@ -743,6 +791,9 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
>   	if (a == &dev_attr_tls_key.attr &&
>   	    (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
>   		return 0;
> +	if (a == &dev_attr_tls_keyring.attr &&
> +	    (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
> +		return 0;

Wouldn't it make better sense to expose it only if tls was requested?

Re: [PATCH] nvme: add sysfs attribute 'tls_keyring'

[Hannes Reinecke]

On 3/8/24 15:01, Sagi Grimberg wrote:
> 
> 
> On 08/03/2024 14:10, Hannes Reinecke wrote:
>> Add a sysfs attribute 'tls_keyring' to hold the contents for the
>> 'keyring' fabrics option. This option is only meaningful if the
>> 'tls' option has been specified; if the 'tls_key' option is specified
>> the key is looked up directly and the keyring setting is ignored.
>> So blank out the keyring value when 'tls_key' is specified.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/sysfs.c | 55 +++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 6c7f1d5c056f..7b65ff7426c9 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -5,11 +5,12 @@
>>    * Copyright (c) 2011-2014, Intel Corporation.
>>    */
>> -#include <linux/nvme-auth.h>
>> -
>>   #include "nvme.h"
>>   #include "fabrics.h"
>> +#include <linux/nvme-auth.h>
>> +#include <linux/nvme-keyring.h>
>> +
>>   static ssize_t nvme_sysfs_reset(struct device *dev,
>>                   struct device_attribute *attr, const char *buf,
>>                   size_t count)
>> @@ -677,6 +678,52 @@ static ssize_t tls_key_show(struct device *dev,
>>       return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
>>   }
>>   static DEVICE_ATTR_RO(tls_key);
>> +
>> +static ssize_t tls_keyring_show(struct device *dev,
>> +                struct device_attribute *attr, char *buf)
>> +{
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> +    struct key *keyring;
>> +
>> +    if (!ctrl->tls_key)
>> +        return 0;
>> +    /* Do not display the keyring for user-selected keys */
>> +    if (ctrl->opts->tls_key)
>> +        return 0;
>> +    keyring = ctrl->opts->keyring;
>> +    if (!keyring) {
>> +        keyring = key_lookup(nvme_keyring_id());
>> +        if (!keyring)
>> +            return -EINVAL;
> 
> Weird if this failed...
> 
Yeah. But feels even weirder to _not_ check it.
WARN_ON?

>> +    }
>> +    return sysfs_emit(buf, "%s", keyring->description);
>> +}
>> +
>> +static ssize_t tls_keyring_store(struct device *dev,
>> +        struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> +    int err;
>> +    u32 key_id;
>> +    struct key *keyring;
>> +
>> +    if (!strlen(buf))
>> +        return -EINVAL;
>> +    err = kstrtou32(buf, 0, &key_id);
>> +    if (err)
>> +        return err;
>> +    keyring = key_lookup(key_id);
>> +    if (IS_ERR(keyring)) {
>> +        pr_err("key %08x not found\n", key_id);
>> +        return -EINVAL;
>> +    }
>> +    if (ctrl->opts->keyring)
>> +        key_put(ctrl->opts->keyring);
>> +    ctrl->opts->keyring = keyring;
>> +    return 0;
>> +}
> 
> Why should there be a _store() for this?
> 
Technically one can change the keyring. But I can drop it for now,
and revisit the issue once I get around to fully test key revokation
and key rotation.

>> +static DEVICE_ATTR(tls_keyring, S_IRUGO | S_IWUSR,
>> +        tls_keyring_show, tls_keyring_store);
>>   #endif
>>   static struct attribute *nvme_dev_attrs[] = {
>> @@ -708,6 +755,7 @@ static struct attribute *nvme_dev_attrs[] = {
>>   #endif
>>   #ifdef CONFIG_NVME_TCP_TLS
>>       &dev_attr_tls_key.attr,
>> +    &dev_attr_tls_keyring.attr,
>>   #endif
>>       &dev_attr_adm_passthru_err_log_enabled.attr,
>>       NULL
>> @@ -743,6 +791,9 @@ static umode_t nvme_dev_attrs_are_visible(struct 
>> kobject *kobj,
>>       if (a == &dev_attr_tls_key.attr &&
>>           (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
>>           return 0;
>> +    if (a == &dev_attr_tls_keyring.attr &&
>> +        (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
>> +        return 0;
> 
> Wouldn't it make better sense to expose it only if tls was requested?

Sort of. But we never did for the 'tls_key' attribute, too, which would
have the same restriction. So I didn't do it here.

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

Re: [PATCH] nvme: add sysfs attribute 'tls_keyring'

[Sagi Grimberg]

On 08/03/2024 16:06, Hannes Reinecke wrote:
> On 3/8/24 15:01, Sagi Grimberg wrote:
>>
>>
>> On 08/03/2024 14:10, Hannes Reinecke wrote:
>>> Add a sysfs attribute 'tls_keyring' to hold the contents for the
>>> 'keyring' fabrics option. This option is only meaningful if the
>>> 'tls' option has been specified; if the 'tls_key' option is specified
>>> the key is looked up directly and the keyring setting is ignored.
>>> So blank out the keyring value when 'tls_key' is specified.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/nvme/host/sysfs.c | 55 
>>> +++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 53 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>>> index 6c7f1d5c056f..7b65ff7426c9 100644
>>> --- a/drivers/nvme/host/sysfs.c
>>> +++ b/drivers/nvme/host/sysfs.c
>>> @@ -5,11 +5,12 @@
>>>    * Copyright (c) 2011-2014, Intel Corporation.
>>>    */
>>> -#include <linux/nvme-auth.h>
>>> -
>>>   #include "nvme.h"
>>>   #include "fabrics.h"
>>> +#include <linux/nvme-auth.h>
>>> +#include <linux/nvme-keyring.h>
>>> +
>>>   static ssize_t nvme_sysfs_reset(struct device *dev,
>>>                   struct device_attribute *attr, const char *buf,
>>>                   size_t count)
>>> @@ -677,6 +678,52 @@ static ssize_t tls_key_show(struct device *dev,
>>>       return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
>>>   }
>>>   static DEVICE_ATTR_RO(tls_key);
>>> +
>>> +static ssize_t tls_keyring_show(struct device *dev,
>>> +                struct device_attribute *attr, char *buf)
>>> +{
>>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>> +    struct key *keyring;
>>> +
>>> +    if (!ctrl->tls_key)
>>> +        return 0;
>>> +    /* Do not display the keyring for user-selected keys */
>>> +    if (ctrl->opts->tls_key)
>>> +        return 0;
>>> +    keyring = ctrl->opts->keyring;
>>> +    if (!keyring) {
>>> +        keyring = key_lookup(nvme_keyring_id());
>>> +        if (!keyring)
>>> +            return -EINVAL;
>>
>> Weird if this failed...
>>
> Yeah. But feels even weirder to _not_ check it.
> WARN_ON?

WARN_ON is better I guess as this is really unexpected.

>
>>> +    }
>>> +    return sysfs_emit(buf, "%s", keyring->description);
>>> +}
>>> +
>>> +static ssize_t tls_keyring_store(struct device *dev,
>>> +        struct device_attribute *attr, const char *buf, size_t count)
>>> +{
>>> +    struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>>> +    int err;
>>> +    u32 key_id;
>>> +    struct key *keyring;
>>> +
>>> +    if (!strlen(buf))
>>> +        return -EINVAL;
>>> +    err = kstrtou32(buf, 0, &key_id);
>>> +    if (err)
>>> +        return err;
>>> +    keyring = key_lookup(key_id);
>>> +    if (IS_ERR(keyring)) {
>>> +        pr_err("key %08x not found\n", key_id);
>>> +        return -EINVAL;
>>> +    }
>>> +    if (ctrl->opts->keyring)
>>> +        key_put(ctrl->opts->keyring);
>>> +    ctrl->opts->keyring = keyring;
>>> +    return 0;
>>> +}
>>
>> Why should there be a _store() for this?
>>
> Technically one can change the keyring. But I can drop it for now,
> and revisit the issue once I get around to fully test key revokation
> and key rotation.

Still I don't see a sane use-case where this would be used.

>
>>> +static DEVICE_ATTR(tls_keyring, S_IRUGO | S_IWUSR,
>>> +        tls_keyring_show, tls_keyring_store);
>>>   #endif
>>>   static struct attribute *nvme_dev_attrs[] = {
>>> @@ -708,6 +755,7 @@ static struct attribute *nvme_dev_attrs[] = {
>>>   #endif
>>>   #ifdef CONFIG_NVME_TCP_TLS
>>>       &dev_attr_tls_key.attr,
>>> +    &dev_attr_tls_keyring.attr,
>>>   #endif
>>>       &dev_attr_adm_passthru_err_log_enabled.attr,
>>>       NULL
>>> @@ -743,6 +791,9 @@ static umode_t nvme_dev_attrs_are_visible(struct 
>>> kobject *kobj,
>>>       if (a == &dev_attr_tls_key.attr &&
>>>           (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
>>>           return 0;
>>> +    if (a == &dev_attr_tls_keyring.attr &&
>>> +        (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
>>> +        return 0;
>>
>> Wouldn't it make better sense to expose it only if tls was requested?
>
> Sort of. But we never did for the 'tls_key' attribute, too, which would
> have the same restriction. So I didn't do it here.

OK, not a must. Can be added later...