| 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
| ||
|
Message-ID: <6d8e91d9-8ae2-43e3-bf6f-4ccb063ef115@amd.com>
Date: Fri, 9 May 2025 15:00:59 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Kurt Borja <kuurtb@...il.com>, Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Thomas Weißschuh <linux@...ssschuh.net>,
Joshua Grisham <josh@...huagrisham.com>,
Mark Pearson <mpearson-lenovo@...ebb.ca>, Armin Wolf <W_Armin@....de>
Cc: Antheas Kapenekakis <lkml@...heas.dev>,
"Derek J. Clark" <derekjohn.clark@...il.com>,
Prasanth Ksr <prasanth.ksr@...l.com>, Jorge Lopez <jorge.lopez2@...com>,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
Dell.Client.Kernel@...l.com
Subject: Re: [PATCH RFC 2/5] platform/x86: firmware_attributes_class: Add a
high level API
On 5/9/2025 2:56 PM, Kurt Borja wrote:
> Hi Mario,
>
> On Fri May 9, 2025 at 12:58 PM -03, Mario Limonciello wrote:
>> On 5/9/2025 2:48 AM, Kurt Borja wrote:
>>> Add an attribute configuration mechanism through the newly introduced
>>> `struct fwat_dev_config`, which makes use of other documented structs
>>> and callbacks.
>>>
>>> This API aims to be simple, yet flexible. In order to accomplish this,
>>> the following features were taken into account:
>>>
>>> * Ability to statically define attributes
>>> * Custom read/write callbacks for each attribute type
>>> * Ability to map attributes to numbers in order to differentiate them in
>>> callbacks (`aux` number)
>>> * Ability to reuse read/write callbacks in different attributes
>>> * Ability to reuse property selection in different attributes
>>> * Optional visibility callback for dynamic attribute visibility
>>>
>>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>>> ---
>>> drivers/platform/x86/firmware_attributes_class.c | 249 ++++++++++++++++++++++-
>>> drivers/platform/x86/firmware_attributes_class.h | 228 +++++++++++++++++++++
>>> 2 files changed, 474 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/platform/x86/firmware_attributes_class.c
>>> index 58ab1495ba3bd449cfe17de2827a57a0c5937788..7cfb0f49f235728c7450a82a7e9d00b8963d3dea 100644
>>> --- a/drivers/platform/x86/firmware_attributes_class.c
>>> +++ b/drivers/platform/x86/firmware_attributes_class.c
>>> @@ -7,14 +7,233 @@
>>> #include <linux/kobject.h>
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> +#include <linux/sysfs.h>
>>> #include <linux/types.h>
>>> #include "firmware_attributes_class.h"
>>>
>>> +#define to_fwat_attribute_ext(_a) container_of_const(_a, struct fwat_attribute_ext, attr)
>>> +
>>> +struct fwat_attribute_ext {
>>> + struct fwat_attribute attr;
>>> + enum fwat_property prop;
>>> + const struct fwat_attr_config *config;
>>> +};
>>> +
>>> const struct class firmware_attributes_class = {
>>> .name = "firmware-attributes",
>>> };
>>> EXPORT_SYMBOL_GPL(firmware_attributes_class);
>>>
>>> +static const char * const fwat_type_labels[] = {
>>> + [fwat_type_integer] = "integer",
>>> + [fwat_type_string] = "string",
>>> + [fwat_type_enumeration] = "enumeration",
>>> +};
>>> +
>>> +static const char * const fwat_prop_labels[] = {
>>> + [FWAT_PROP_DISPLAY_NAME] = "display_name",
>>> + [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code",
>>> + [FWAT_PROP_DEFAULT] = "default",
>>> +
>>> + [FWAT_INT_PROP_MIN] = "min_value",
>>> + [FWAT_INT_PROP_MAX] = "max_value",
>>> + [FWAT_INT_PROP_INCREMENT] = "scalar_increment",
>>> +
>>> + [FWAT_STR_PROP_MIN] = "min_length",
>>> + [FWAT_STR_PROP_MAX] = "max_length",
>>> +
>>> + [FWAT_ENUM_PROP_POSSIBLE_VALUES] = "possible_values",
>>> +};
>>> +
>>> +static ssize_t
>>> +fwat_type_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> +
>>> + return sysfs_emit(buf, "%s\n", fwat_type_labels[config->type]);
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_property_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> +
>>> + if (!config->ops->prop_read)
>>> + return -EOPNOTSUPP;
>>> +
>>> + return config->ops->prop_read(dev, config->aux, ext->prop, buf);
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_current_value_show(struct device *dev, const struct fwat_attribute *attr, char *buf)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> + const char *str;
>>> + long int_val;
>>> + int ret;
>>> +
>>> + switch (config->type) {
>>> + case fwat_type_integer:
>>> + ret = config->ops->integer_read(dev, config->aux, &int_val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sysfs_emit(buf, "%ld\n", int_val);
>>> + case fwat_type_string:
>>> + ret = config->ops->string_read(dev, config->aux, &str);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sysfs_emit(buf, "%s\n", str);
>>> + case fwat_type_enumeration:
>>> + ret = config->ops->enumeration_read(dev, config->aux, &str);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return sysfs_emit(buf, "%s\n", str);
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +}
>>> +
>>> +static ssize_t
>>> +fwat_current_value_store(struct device *dev, const struct fwat_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + const struct fwat_attribute_ext *ext = to_fwat_attribute_ext(attr);
>>> + const struct fwat_attr_config *config = ext->config;
>>> + long int_val;
>>> + int ret;
>>> +
>>> + switch (config->type) {
>>> + case fwat_type_integer:
>>> + ret = kstrtol(buf, 0, &int_val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = config->ops->integer_write(dev, config->aux, int_val);
>>> + break;
>>> + case fwat_type_string:
>>> + ret = config->ops->string_write(dev, config->aux, buf);
>>> + break;
>>> + case fwat_type_enumeration:
>>> + ret = config->ops->enumeration_write(dev, config->aux, buf);
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + return ret ? ret : count;
>>> +}
>>> +
>>> +static struct attribute *
>>> +fwat_alloc_attr(struct device *dev, const struct fwat_attr_config *config,
>>> + const char *attr_name, umode_t mode, enum fwat_property prop,
>>> + ssize_t (*show)(struct device *dev, const struct fwat_attribute *attr,
>>> + char *buf),
>>> + ssize_t (*store)(struct device *dev, const struct fwat_attribute *attr,
>>> + const char *buf, size_t count))
>>> +{
>>> + struct fwat_attribute_ext *fattr;
>>> +
>>> + fattr = devm_kzalloc(dev, sizeof(*fattr), GFP_KERNEL);
>>> + if (!fattr)
>>> + return NULL;
>>> +
>>> + fattr->attr.attr.name = attr_name;
>>> + fattr->attr.attr.mode = mode;
>>> + fattr->attr.show = show;
>>> + fattr->attr.store = store;
>>> + fattr->prop = prop;
>>> + fattr->config = config;
>>> + sysfs_attr_init(&fattr->attr.attr);
>>> +
>>> + return &fattr->attr.attr;
>>> +}
>>> +
>>> +static struct attribute **
>>> +fwat_create_attrs(struct device *dev, const struct fwat_attr_config *config)
>>> +{
>>> + struct attribute **attrs;
>>> + enum fwat_property prop;
>>> + unsigned int index = 0;
>>> +
>>> + attrs = devm_kcalloc(dev, config->num_props + 3, sizeof(*attrs), GFP_KERNEL);
>>> + if (!attrs)
>>> + return NULL;
>>> +
>>> + /*
>>> + * Create optional attributes
>>> + */
>> Just a nit here; this probably doesn't need to be a multiline comment.
>> a single line like this is fine:
>>
>> /* optional attributes */
>>
>>> + for (; index < config->num_props; index++) {
>>> + prop = config->props[index];
>>> + attrs[index] = fwat_alloc_attr(dev, config, fwat_prop_labels[prop],
>>> + 0444, prop, fwat_property_show, NULL);
>>> + }
>>> +
>>> + /*
>>> + * Create mandatory attributes
>>> + */
>>
>> Same as above
>
> Ack for these two.
>
>>
>>> + attrs[index++] = fwat_alloc_attr(dev, config, "type", 0444, 0, fwat_type_show, NULL);
>>> + attrs[index++] = fwat_alloc_attr(dev, config, "current_value", 0644, 0,
>>
>> Is this permission right? Some attributes could be considered more
>> sensitive can't they?
>
> You are right. I assumed most drivers would want 0644 but think-lmi and
> dell sysman use 0600.
>
> I can add a mode_t mask to fwat_attr_config and use that for
> current_value, while other properties would be masked by it, i.e.
> `0444 & config->mode`
>
> What do you think?
Yeah; current_value is the only sensitive one.
Powered by blists - more mailing lists