| Message ID | 20210509015708.112766-3-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events | expand |
Hi, On 5/9/21 3:57 AM, Mark Pearson wrote: > For Lenovo platforms that support a WMI interface to the BIOS add > support, using the firmware-attributes class, to allow users to access > and modify various BIOS related settings. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> > --- > Changes in V2: > - Improved error handling > - updated to use kobj's better (thanks Hans for pointers). Makes code > simpler and cleaner > - Removed some dead code from previous implementation that I'd missed > - Moved think-lmi.h into platform/x86 directory > - Use firmware_attributes_class helper > - Corrected some bad indexing when storing an attribute > - improved encoding options to limit selection > - Added details on encoding and kbdlang to > sysfs-class-firmware-attributes documentation Thank you, overall this looks pretty good, a small review remarks inline. > > .../testing/sysfs-class-firmware-attributes | 13 +- > drivers/platform/x86/Kconfig | 12 + > drivers/platform/x86/Makefile | 1 + > drivers/platform/x86/think-lmi.c | 890 ++++++++++++++++++ > drivers/platform/x86/think-lmi.h | 73 ++ > 5 files changed, 988 insertions(+), 1 deletion(-) > create mode 100644 drivers/platform/x86/think-lmi.c > create mode 100644 drivers/platform/x86/think-lmi.h > > diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes > index 8ea59fea4..31d1becfa 100644 > --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes > +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes > @@ -185,6 +185,17 @@ Description: > A write only value that when used in tandem with > current_password will reset a system or admin password. > > + encoding: > + For platforms that require it (currently Lenovo) the > + encoding method that is used. This can be either "ascii" > + or "scancode". Default is set to "ascii" > + > + kbdlang: > + For platforms that require it (currently Lenovo only) the > + keyboard language method that is used. This is generally a > + two char code (e.g. "us", "fr", "gr") and may vary per platform. > + Default is set to "us" > + I should have caught this during my review of v1, these are Lenovo specific, so please prefix them with lenovo_ (the same is already done for dell specific sysfs attributes) and move them to a separate "Lenovo specific class extensions" part of the doc. > Note, password management is session specific. If Admin password is set, > same password must be written into current_password file (required for > password-validation) and must be cleared once the session is over. > @@ -197,7 +208,7 @@ Description: > Drivers may emit a CHANGE uevent when a password is set or unset > userspace may check it again. > > - On Dell systems, if Admin password is set, then all BIOS attributes > + On Dell and Lenovo systems, if Admin password is set, then all BIOS attributes > require password validation. > > What: /sys/class/firmware-attributes/*/attributes/pending_reboot > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index b0e1e5f65..0511c54fc 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -639,6 +639,18 @@ config THINKPAD_ACPI_HOTKEY_POLL > If you are not sure, say Y here. The driver enables polling only if > it is strictly necessary to do so. > > +config THINKPAD_LMI > + tristate "Lenovo WMI-based systems management driver" > + default m default n (or no default at all) please. > + depends on ACPI_WMI > + select FW_ATTR_CLASS > + help > + This driver allows changing BIOS settings on Lenovo machines whose > + BIOS support the WMI interface. > + > + To compile this driver as a module, choose M here: the module will > + be called think-lmi. > + > config INTEL_ATOMISP2_LED > tristate "Intel AtomISP2 camera LED driver" > depends on GPIOLIB && LEDS_GPIO > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 147573f69..cab19672e 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -64,6 +64,7 @@ obj-$(CONFIG_IBM_RTL) += ibm_rtl.o > obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o > obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o > obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o > +obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o > > # Intel > obj-$(CONFIG_INTEL_ATOMISP2_LED) += intel_atomisp2_led.o > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > new file mode 100644 > index 000000000..2fa989e98 > --- /dev/null > +++ b/drivers/platform/x86/think-lmi.c > @@ -0,0 +1,890 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * think-lmi.c - Think LMI BIOS configuration driver > + * > + * Copyright(C) 2019-2021 Lenovo > + * > + * Original code from Thinkpad-wmi project https://github.com/iksaif/thinkpad-wmi > + * Copyright(C) 2017 Corentin Chary <corentin.chary@gmail.com> > + * Distributed under the GPL-2.0 license > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/acpi.h> > +#include <linux/fs.h> > +#include <linux/wmi.h> > +#include "firmware_attributes_class.h" > +#include "think-lmi.h" > + > +/* > + * Name: > + * Lenovo_BiosSetting > + * Description: > + * Get item name and settings for current LMI instance. > + * Type: > + * Query > + * Returns: > + * "Item,Value" > + * Example: > + * "WakeOnLAN,Enable" > + */ > +#define LENOVO_BIOS_SETTING_GUID "51F5230E-9677-46CD-A1CF-C0B23EE34DB7" > + > +/* > + * Name: > + * Lenovo_SetBiosSetting > + * Description: > + * Change the BIOS setting to the desired value using the Lenovo_SetBiosSetting > + * class. To save the settings, use the Lenovo_SaveBiosSetting class. > + * BIOS settings and values are case sensitive. > + * After making changes to the BIOS settings, you must reboot the computer > + * before the changes will take effect. > + * Type: > + * Method > + * Arguments: > + * "Item,Value,Password,Encoding,KbdLang;" > + * Example: > + * "WakeOnLAN,Disable,pswd,ascii,us;" > + */ > +#define LENOVO_SET_BIOS_SETTINGS_GUID "98479A64-33F5-4E33-A707-8E251EBBC3A1" > + > +/* > + * Name: > + * Lenovo_SaveBiosSettings > + * Description: > + * Save any pending changes in settings. > + * Type: > + * Method > + * Arguments: > + * "Password,Encoding,KbdLang;" > + * Example: > + * "pswd,ascii,us;" > + */ > +#define LENOVO_SAVE_BIOS_SETTINGS_GUID "6A4B54EF-A5ED-4D33-9455-B0D9B48DF4B3" > + > +/* > + * Name: > + * Lenovo_BiosPasswordSettings > + * Description: > + * Return BIOS Password settings > + * Type: > + * Query > + * Returns: > + * PasswordMode, PasswordState, MinLength, MaxLength, > + * SupportedEncoding, SupportedKeyboard > + */ > +#define LENOVO_BIOS_PASSWORD_SETTINGS_GUID "8ADB159E-1E32-455C-BC93-308A7ED98246" > + > +/* > + * Name: > + * Lenovo_SetBiosPassword > + * Description: > + * Change a specific password. > + * - BIOS settings cannot be changed at the same boot as power-on > + * passwords (POP) and hard disk passwords (HDP). If you want to change > + * BIOS settings and POP or HDP, you must reboot the system after changing > + * one of them. > + * - A password cannot be set using this method when one does not already > + * exist. Passwords can only be updated or cleared. > + * Type: > + * Method > + * Arguments: > + * "PasswordType,CurrentPassword,NewPassword,Encoding,KbdLang;" > + * Example: > + * "pop,oldpop,newpop,ascii,us;” > + */ > +#define LENOVO_SET_BIOS_PASSWORD_GUID "2651D9FD-911C-4B69-B94E-D0DED5963BD7" > + > +/* > + * Name: > + * Lenovo_GetBiosSelections > + * Description: > + * Return a list of valid settings for a given item. > + * Type: > + * Method > + * Arguments: > + * "Item" > + * Returns: > + * "Value1,Value2,Value3,..." > + * Example: > + * -> "FlashOverLAN" > + * <- "Enabled,Disabled" > + */ > +#define LENOVO_GET_BIOS_SELECTIONS_GUID "7364651A-132F-4FE7-ADAA-40C6C7EE2E3B" > + > +#define TLMI_POP_PWD (1 << 0) > +#define TLMI_PAP_PWD (1 << 1) > + > +static const char * const encoding_options[] = { > + [TLMI_ENCODING_ASCII] = "ascii", > + [TLMI_ENCODING_SCANCODE] = "scancode", > +}; > +static struct think_lmi tlmi_priv; > +struct class *fw_attr_class; > + > +/* ------ Utility functions ------------*/ > +/* Convert BIOS WMI error string to suitable error code */ > +static int tlmi_errstr_to_err(const char *errstr) > +{ > + if (!strcmp(errstr, "Success")) > + /* Operation completed successfully */ > + return 0; > + if (!strcmp(errstr, "Not Supported")) > + /* The feature is not supported on this system */ > + return -EOPNOTSUPP; > + if (!strcmp(errstr, "Invalid")) > + /* The item or value provided is not valid parameter */ > + return -EINVAL; > + if (!strcmp(errstr, "Access Denied")) > + /* > + * The change could not be made due to an authentication problem. > + * If a supervisor password exists, the correct supervisor password > + * must be provided. > + */ > + return -EPERM; > + if (!strcmp(errstr, "System Busy")) > + /* > + * BIOS changes have already been made that need to be committed. > + 238G* Reboot the system and try again. > + */ > + return -EBUSY; > + > + pr_debug("Unknown error string: '%s'", errstr); > + return -EINVAL; > +} > + > +/* Extract error string from WMI return buffer */ > +static int tlmi_extract_error(const struct acpi_buffer *output) > +{ > + const union acpi_object *obj; > + int ret; > + > + obj = output->pointer; > + if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { > + kfree(obj); > + return -EIO; > + } > + > + ret = tlmi_errstr_to_err(obj->string.pointer); > + kfree(obj); > + return ret; > +} > + > +/* Utility function to execute WMI call to BIOS */ > +static int tlmi_simple_call(const char *guid, const char *arg) > +{ > + const struct acpi_buffer input = { strlen(arg), (char *)arg }; > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + acpi_status status; > + int i, err; > + > + /* > + * duplicated call required to match bios workaround for behavior > + * seen when WMI accessed via scripting on other OS > + */ > + for (i = 0; i < 2; i++) { > + /* (re)initialize output buffer to default state */ > + output.length = ACPI_ALLOCATE_BUFFER; > + output.pointer = NULL; > + > + status = wmi_evaluate_method(guid, 0, 0, &input, &output); > + if (ACPI_FAILURE(status)) { > + kfree(output.pointer); > + return -EIO; > + } > + err = tlmi_extract_error(&output); > + if (err) > + return err; > + } > + return 0; > +} > + > +/* Extract output string from WMI return buffer */ > +static int tlmi_extract_output_string(const struct acpi_buffer *output, > + char **string) > +{ > + const union acpi_object *obj; > + > + obj = output->pointer; > + if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { > + kfree(obj); > + return -EIO; > + } > + > + *string = kstrdup(obj->string.pointer, GFP_KERNEL); > + kfree(obj); > + return *string ? 0 : -ENOMEM; > +} > + > +/* ------ Core interface functions ------------*/ > + > +/* Get password settings from BIOS */ > +static int tlmi_get_pwd_settings(struct tlmi_pwdcfg *pwdcfg) > +{ > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + const union acpi_object *obj; > + acpi_status status; > + > + if (!tlmi_priv.can_get_password_settings) > + return -EOPNOTSUPP; > + > + status = wmi_query_block(LENOVO_BIOS_PASSWORD_SETTINGS_GUID, 0, > + &output); > + if (ACPI_FAILURE(status)) > + return -EIO; > + > + obj = output.pointer; > + if (!obj || obj->type != ACPI_TYPE_BUFFER || !obj->buffer.pointer) { > + kfree(obj); > + return -EIO; > + } > + /* > + * The size of thinkpad_wmi_pcfg on ThinkStation is larger than ThinkPad. > + * To make the driver compatible on different brands, we permit it to get > + * the data in below case. > + */ > + if (obj->buffer.length < sizeof(struct tlmi_pwdcfg)) { > + pr_warn("Unknown pwdcfg buffer length %d\n", obj->buffer.length); > + kfree(obj); > + return -EIO; > + } > + memcpy(pwdcfg, obj->buffer.pointer, sizeof(struct tlmi_pwdcfg)); > + kfree(obj); > + return 0; > +} > + > +static int tlmi_save_bios_settings(const char *password) > +{ > + return tlmi_simple_call(LENOVO_SAVE_BIOS_SETTINGS_GUID, > + password); > +} > + > +static int tlmi_setting(int item, char **value, const char *guid_string) > +{ > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + acpi_status status; > + > + status = wmi_query_block(guid_string, item, &output); > + if (ACPI_FAILURE(status)) { > + kfree(output.pointer); > + return -EIO; > + } > + > + return tlmi_extract_output_string(&output, value); > +} > + > +static int tlmi_get_bios_selections(const char *item, char **value) > +{ > + const struct acpi_buffer input = { strlen(item), (char *)item }; > + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > + acpi_status status; > + > + status = wmi_evaluate_method(LENOVO_GET_BIOS_SELECTIONS_GUID, > + 0, 0, &input, &output); > + > + if (ACPI_FAILURE(status)) { > + kfree(output.pointer); > + return -EIO; > + } > + > + return tlmi_extract_output_string(&output, value); > +} > + > +/* ---- Authentication sysfs --------------------------------------------------------- */ > +static ssize_t is_enabled_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + > + if (setting->valid) > + return sysfs_emit(buf, "1\n"); > + else > + return sysfs_emit(buf, "0\n"); Maybe use: sysfs_emit(buf, "%d\n", settings->valid); instead? > +} > + > +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); > + > +static ssize_t current_password_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + int length; > + > + length = strlen(buf); > + if (buf[length-1] == '\n') > + length--; > + > + if (length >= TLMI_PWD_MAXLEN) > + return -EINVAL; > + > + memcpy(setting->password, buf, length); > + setting->password[length] = '\0'; > + return count; > +} > + > +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password); > + > +static ssize_t new_password_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + char *p, *new_pwd; > + char *auth_str; > + int ret = 0, len; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EPERM; > + > + if (!tlmi_priv.can_set_bios_password) > + return -EOPNOTSUPP; > + > + new_pwd = kstrdup(buf, GFP_KERNEL); > + if (!new_pwd) > + return -ENOMEM; > + > + p = strchr(new_pwd, '\n'); > + if (p) > + *p = '\0'; > + > + if (strlen(new_pwd) > setting->maxlen) { > + ret = -EINVAL; > + goto out; > + } > + > + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ > + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) > + + strlen(setting->kbdlang) + 3 /* type */ > + + strlen(new_pwd) + 6 /* punctuation and terminator*/; > + auth_str = kzalloc(len, GFP_KERNEL); > + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", > + setting->pwd_type, setting->password, new_pwd, > + encoding_options[setting->encoding], setting->kbdlang); > + ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str); So I guess on success any subsequent calls need to use the new password, so the user is expected to write the new password to the current_password file after changing the password this way? I just checked the dell-wmi-sysman code and that does this: /* clear current_password here and use user input from wmi_priv.current_password */ if (!ret) memset(current_password, 0, MAX_BUFF); Where current_password points to either the user or admin cached password, depending on which one is being changed. So that seems to work the same way as what you are doing here (the user needs to write the new password to current_password after changing it through the new_password sysfs attribute). Can you add a patch to the patch-set documenting this in Documentation/ABI/testing/sysfs-class-firmware-attributes ? Also you may want to consider clearing out the old cached password on success like the Dell code is doing. > + kfree(auth_str); > +out: > + kfree(new_pwd); > + return ret ? ret : count; > +} > + > +static struct kobj_attribute auth_new_password = __ATTR_WO(new_password); > + > +static ssize_t min_password_length_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + > + return sysfs_emit(buf, "%d\n", setting->minlen); > +} > + > +static struct kobj_attribute auth_min_pass_length = __ATTR_RO(min_password_length); > + > +static ssize_t max_password_length_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + > + return sysfs_emit(buf, "%d\n", setting->maxlen); > +} > +static struct kobj_attribute auth_max_pass_length = __ATTR_RO(max_password_length); > + > +static ssize_t mechanism_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "password\n"); > +} > +static struct kobj_attribute auth_mechanism = __ATTR_RO(mechanism); > + > +static ssize_t encoding_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + > + return sysfs_emit(buf, "%s\n", encoding_options[setting->encoding]); > +} > + > +static ssize_t encoding_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + int i; > + > + /* Scan for a matching profile */ > + i = sysfs_match_string(encoding_options, buf); > + if (i < 0) > + return -EINVAL; > + > + setting->encoding = i; > + return count; > +} > + > +static struct kobj_attribute auth_encoding = __ATTR_RW(encoding); > + > +static ssize_t kbdlang_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + > + return sysfs_emit(buf, "%s\n", setting->kbdlang); > +} > + > +static ssize_t kbdlang_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + int length; > + > + length = strlen(buf); > + if (buf[length-1] == '\n') > + length--; > + if (!length || (length >= TLMI_LANG_MAXLEN)) > + return -EINVAL; > + > + memcpy(setting->kbdlang, buf, length); > + setting->kbdlang[length] = '\0'; > + return count; > +} > + > +static struct kobj_attribute auth_kbdlang = __ATTR_RW(kbdlang); > + > +{ > + if (strcmp(kobj->name, "Admin") == 0) > + return sprintf(buf, "bios-admin\n"); > + else if (strcmp(kobj->name, "System") == 0) > + return sprintf(buf, "power-on\n"); > + return -EIO; > +} IMHO it would be nice to add a "const char *role" to struct tlmi_pwd_setting and then change this to: > +static ssize_t encoding_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + > + return sysfs_emit(buf, "%s\n", setting->role); > +} That would make this function consistent with the other password related functions. > +static struct kobj_attribute auth_role = __ATTR_RO(role); > + > +static struct attribute *auth_attrs[] = { > + &auth_is_pass_set.attr, > + &auth_min_pass_length.attr, > + &auth_max_pass_length.attr, > + &auth_current_password.attr, > + &auth_new_password.attr, > + &auth_role.attr, > + &auth_mechanism.attr, > + &auth_encoding.attr, > + &auth_kbdlang.attr, > + NULL, > +}; > + > +static const struct attribute_group auth_attr_group = { > + .attrs = auth_attrs, > +}; > + > +/* ---- Attributes sysfs --------------------------------------------------------- */ > +static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); > + > + return sysfs_emit(buf, "%s\n", setting->display_name); > +} > + > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > +{ > + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); > + char *item; > + int ret; > + > + ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID); > + if (ret) > + return ret; > + > + ret = sysfs_emit(buf, "%s\n", item); > + kfree(item); > + return ret; > +} > + > +static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > +{ > + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); > + > + if (!tlmi_priv.can_get_bios_selections) > + return -EOPNOTSUPP; > + > + return sysfs_emit(buf, "%s\n", setting->possible_values); > +} > + > +static ssize_t current_value_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); > + int alloc_len, auth_len = 0; > + int str_ix = 0; > + char *auth_str = NULL; > + char *set_str, *new_setting, *p; > + int ret; > + > + if (!tlmi_priv.can_set_bios_settings) > + return -EOPNOTSUPP; > + > + new_setting = kstrdup(buf, GFP_KERNEL); > + if (!new_setting) > + return -ENOMEM; > + p = strchr(new_setting, '\n'); > + if (p) > + *p = '\0'; > + > + alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting); > + > + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) { > + auth_len += strlen(tlmi_priv.pwd_admin->password) + 1 > + + strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1 > + + strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1; > + auth_str = kmalloc(auth_len, GFP_KERNEL); > + if (!auth_str) { > + ret = -ENOMEM; > + goto out; You end-up kfree-ing set_str here without it being initialized (compiler warning?) Please just initialize all 3 strings which you kfree at the end to NULL, I know this is not necessary for at least the new_setting string but it makes it easier for someone reading the code to verify that the kfree() does not happens on an uninitialized pointer. > + } > + > + sprintf(auth_str, "%s,%s,%s;", > + tlmi_priv.pwd_admin->password, > + encoding_options[tlmi_priv.pwd_admin->encoding], > + tlmi_priv.pwd_admin->kbdlang); > + alloc_len += auth_len; > + } > + > + set_str = kmalloc(alloc_len, GFP_KERNEL); > + if (!set_str) { > + ret = -ENOMEM; > + goto out; > + } > + > + str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting); > + > + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) Maybe use: if (auth_str) here? This way if the condition of the above if block ever changes, you can't end up passing a NULL auth_Str to the sprintf. > + sprintf(set_str + str_ix, ",%s", auth_str); > + else > + sprintf(set_str + str_ix, ";"); > + > + ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str); > + if (ret) > + goto out; > + > + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) > + ret = tlmi_save_bios_settings(auth_str); > + else > + ret = tlmi_save_bios_settings(""); > + if (ret) > + goto out; > + > +out: > + kfree(auth_str); > + kfree(set_str); > + kfree(new_setting); > + return ret ? ret : count; > +} > + > +static struct kobj_attribute attr_displ_name = > + __ATTR_RO(display_name); > + > +static struct kobj_attribute attr_possible_values = > + __ATTR_RO(possible_values); > + > +static struct kobj_attribute attr_current_val = > + __ATTR_RW_MODE(current_value, 0600); > + > +static struct attribute *tlmi_attrs[] = { > + &attr_displ_name.attr, > + &attr_current_val.attr, > + &attr_possible_values.attr, > + NULL, > +}; > + > +static const struct attribute_group tlmi_attr_group = { > + .attrs = tlmi_attrs, > +}; > + > +static ssize_t tlmi_attr_show(struct kobject *kobj, struct attribute *attr, > + char *buf) > +{ > + struct kobj_attribute *kattr; > + ssize_t ret = -EIO; > + > + kattr = container_of(attr, struct kobj_attribute, attr); > + if (kattr->show) > + ret = kattr->show(kobj, kattr, buf); > + return ret; > +} > + > +static ssize_t tlmi_attr_store(struct kobject *kobj, struct attribute *attr, > + const char *buf, size_t count) > +{ > + struct kobj_attribute *kattr; > + ssize_t ret = -EIO; > + > + kattr = container_of(attr, struct kobj_attribute, attr); > + if (kattr->store) > + ret = kattr->store(kobj, kattr, buf, count); > + return ret; > +} > + > +static const struct sysfs_ops tlmi_kobj_sysfs_ops = { > + .show = tlmi_attr_show, > + .store = tlmi_attr_store, > +}; > + > +static struct kobj_type attr_name_ktype = { > + .sysfs_ops = &tlmi_kobj_sysfs_ops, > +}; You need to re-add a release function here, sorry I was not clear about that in my review of v1. Since you do a kobject_put and not a direct kfree() on cleanup, you need to define a release function to do the actual free once the refcount drops to 0. And since you have 2 kind of structs embedding the kobjects you will need 2 release functions: static void tmli_attr_setting_release(struct kobject *kobj) { struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); kfree(setting); } static void tmli_pwd_setting_release(struct kobject *kobj) { struct tlmi_pwd_setting *pwd = container_of(kobj, struct tlmi_pwd_setting, kobj); kfree(pwd); } And then have 2 kobject-types: static struct kobj_type tmli_attr_setting_ktype = { .release = tmli_attr_setting_release, .sysfs_ops = &tlmi_kobj_sysfs_ops, }; static struct kobj_type tmli_pwd_setting_ktype = { .release = tmli_pwd_setting_release, .sysfs_ops = &tlmi_kobj_sysfs_ops, }; And use the right type when creating the objects. > + > +/* ---- Initialisation --------------------------------------------------------- */ > +static void tlmi_release_attr(void) > +{ > + int i; > + > + /* Attribute structures */ > + for (i = 0; i < TLMI_SETTINGS_COUNT; i++) { > + > + if (tlmi_priv.setting[i]) { > + kfree(tlmi_priv.setting[i]->possible_values); > + sysfs_remove_group(&tlmi_priv.setting[i]->kobj, &tlmi_attr_group); > + kobject_put(&tlmi_priv.setting[i]->kobj); > + } > + } > + kset_unregister(tlmi_priv.attribute_kset); > + > + /* Authentication structures */ > + sysfs_remove_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group); > + kobject_put(&tlmi_priv.pwd_admin->kobj); > + sysfs_remove_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group); > + kobject_put(&tlmi_priv.pwd_power->kobj); > + kset_unregister(tlmi_priv.authentication_kset); > +} > + > +static int tlmi_sysfs_init(void) > +{ > + int i, ret; > + > + ret = fw_attributes_class_register(&fw_attr_class); > + if (ret) > + return ret; > + > + tlmi_priv.class_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), > + NULL, "%s", "thinklmi"); > + if (IS_ERR(tlmi_priv.class_dev)) { > + ret = PTR_ERR(tlmi_priv.class_dev); > + goto fail_class_created; > + } > + > + tlmi_priv.attribute_kset = kset_create_and_add("attributes", NULL, > + &tlmi_priv.class_dev->kobj); > + if (!tlmi_priv.attribute_kset) { > + ret = -ENOMEM; > + goto fail_device_created; > + } > + > + for (i = 0; i < TLMI_SETTINGS_COUNT; i++) { > + /*Check if index is a valid setting - skip if it isn't */ > + if (!tlmi_priv.setting[i]) > + continue; > + > + /* build attribute */ > + tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset; > + ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &attr_name_ktype, > + NULL, "%s", tlmi_priv.setting[i]->display_name); > + if (ret) > + goto fail_create_attr; > + > + ret = sysfs_create_group(&tlmi_priv.setting[i]->kobj, &tlmi_attr_group); > + if (ret) > + goto fail_create_attr; > + } > + > + /* Create authentication entries */ > + tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL, > + &tlmi_priv.class_dev->kobj); > + if (!tlmi_priv.authentication_kset) { > + ret = -ENOMEM; > + goto fail_create_attr; > + } > + tlmi_priv.pwd_admin->kobj.kset = tlmi_priv.authentication_kset; > + ret = kobject_init_and_add(&tlmi_priv.pwd_admin->kobj, &attr_name_ktype, > + NULL, "%s", "Admin"); > + if (ret) > + goto fail_create_attr; > + > + ret = sysfs_create_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group); > + if (ret) > + goto fail_create_attr; > + > + tlmi_priv.pwd_power->kobj.kset = tlmi_priv.authentication_kset; > + ret = kobject_init_and_add(&tlmi_priv.pwd_power->kobj, &attr_name_ktype, > + NULL, "%s", "System"); > + if (ret) > + goto fail_create_attr; > + > + ret = sysfs_create_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group); > + if (ret) > + goto fail_create_attr; > + > + return ret; > + > +fail_create_attr: > + tlmi_release_attr(); > +fail_device_created: > + device_destroy(fw_attr_class, MKDEV(0, 0)); > +fail_class_created: > + fw_attributes_class_remove(); > + return ret; > +} > + > +/* ---- Base Driver -------------------------------------------------------- */ > +static int tlmi_analyze(void) > +{ > + struct tlmi_pwdcfg pwdcfg; > + acpi_status status; > + int i = 0; > + int ret; > + > + if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) && > + wmi_has_guid(LENOVO_SAVE_BIOS_SETTINGS_GUID)) > + tlmi_priv.can_set_bios_settings = true; > + > + if (wmi_has_guid(LENOVO_GET_BIOS_SELECTIONS_GUID)) > + tlmi_priv.can_get_bios_selections = true; > + > + if (wmi_has_guid(LENOVO_SET_BIOS_PASSWORD_GUID)) > + tlmi_priv.can_set_bios_password = true; > + > + if (wmi_has_guid(LENOVO_BIOS_PASSWORD_SETTINGS_GUID)) > + tlmi_priv.can_get_password_settings = true; > + > + /* > + * Try to find the number of valid settings of this machine > + * and use it to create sysfs attributes > + */ > + for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) { > + char *item = NULL; > + int spleng = 0; > + int num = 0; > + char *p; > + struct tlmi_attr_setting *setting; > + > + tlmi_priv.setting[i] = NULL; > + status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); > + if (ACPI_FAILURE(status)) > + break; > + if (!item) > + break; > + if (!*item) > + continue; > + > + /* It is not allowed to have '/' for file name. Convert it into '\'. */ > + spleng = strlen(item); > + for (num = 0; num < spleng; num++) { > + if (item[num] == '/') > + item[num] = '\\'; > + } > + > + /* Remove the value part */ > + p = strchr(item, ','); > + if (p) > + *p = '\0'; > + > + /* Create a setting entry */ > + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); > + if (!setting) { > + ret = -ENOMEM; > + goto fail_clear_attr; > + } > + setting->index = i; > + strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN); > + /* If BIOS selections supported, load those */ > + if (tlmi_priv.can_get_bios_selections) { > + ret = tlmi_get_bios_selections(setting->display_name, > + &setting->possible_values); > + if (ret || !setting->possible_values) > + pr_info("Error retrieving possible values for %d : %s\n", > + i, setting->display_name); > + } > + tlmi_priv.setting[i] = setting; > + tlmi_priv.settings_count++; > + kfree(item); > + } > + > + /* Create password setting structure */ > + ret = tlmi_get_pwd_settings(&pwdcfg); > + if (ret) > + goto fail_clear_attr; > + > + tlmi_priv.pwd_admin = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL); > + if (!tlmi_priv.pwd_admin) { > + ret = -ENOMEM; > + goto fail_clear_attr; > + } > + sprintf(tlmi_priv.pwd_admin->display_name, "admin"); > + sprintf(tlmi_priv.pwd_admin->kbdlang, "us"); > + tlmi_priv.pwd_admin->encoding = TLMI_ENCODING_ASCII; > + sprintf(tlmi_priv.pwd_admin->pwd_type, "pap"); > + tlmi_priv.pwd_admin->minlen = pwdcfg.min_length; > + tlmi_priv.pwd_admin->maxlen = pwdcfg.max_length; > + if (pwdcfg.password_state & TLMI_PAP_PWD) > + tlmi_priv.pwd_admin->valid = true; > + > + tlmi_priv.pwd_power = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL); > + if (!tlmi_priv.pwd_power) { > + ret = -ENOMEM; > + goto fail_clear_attr; > + } > + sprintf(tlmi_priv.pwd_power->display_name, "power-on"); > + sprintf(tlmi_priv.pwd_power->kbdlang, "us"); > + tlmi_priv.pwd_power->encoding = TLMI_ENCODING_ASCII; > + sprintf(tlmi_priv.pwd_admin->pwd_type, "pop"); > + tlmi_priv.pwd_power->minlen = pwdcfg.min_length; > + tlmi_priv.pwd_power->maxlen = pwdcfg.max_length; > + > + if (pwdcfg.password_state & TLMI_POP_PWD) > + tlmi_priv.pwd_power->valid = true; > + > + return 0; > + > +fail_clear_attr: > + for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) > + kfree(tlmi_priv.setting[i]); > + return ret; > +} > + > +static void tlmi_remove(struct wmi_device *wdev) > +{ > + tlmi_release_attr(); > + device_destroy(fw_attr_class, MKDEV(0, 0)); > + fw_attributes_class_remove(); > +} > + > +static int tlmi_probe(struct wmi_device *wdev, const void *context) > +{ > + tlmi_analyze(); > + return tlmi_sysfs_init(); > +} > + > +static const struct wmi_device_id tlmi_id_table[] = { > + { .guid_string = LENOVO_BIOS_SETTING_GUID }, > + { } > +}; > + > +static struct wmi_driver tlmi_driver = { > + .driver = { > + .name = "think-lmi", > + }, > + .id_table = tlmi_id_table, > + .probe = tlmi_probe, > + .remove = tlmi_remove, > +}; > + > +MODULE_AUTHOR("Sugumaran L <slacshiminar@lenovo.com>"); > +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); > +MODULE_AUTHOR("Corentin Chary <corentin.chary@gmail.com>"); > +MODULE_DESCRIPTION("ThinkLMI Driver"); > +MODULE_LICENSE("GPL"); > + > +module_wmi_driver(tlmi_driver); > diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h > new file mode 100644 > index 000000000..2c1484304 > --- /dev/null > +++ b/drivers/platform/x86/think-lmi.h > @@ -0,0 +1,73 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > + > +#ifndef _THINK_LMI_H_ > +#define _THINK_LMI_H_ > + > +#define TLMI_SETTINGS_COUNT 256 > +#define TLMI_SETTINGS_MAXLEN 512 > +#define TLMI_PWD_MAXLEN 128 > +#define TLMI_PWDTYPE_MAXLEN 64 > +#define TLMI_ENC_MAXLEN 64 > +#define TLMI_LANG_MAXLEN 4 > +#define TLMI_PWDTYPE_LEN 4 > +/* > + * Longest string should be in the set command: allow size of BIOS > + * option and choice > + */ > +#define TLMI_GETSET_MAXLEN (TLMI_SETTINGS_MAXLEN + TLMI_SETTINGS_MAXLEN) > + > +enum encoding_option { > + TLMI_ENCODING_ASCII, > + TLMI_ENCODING_SCANCODE, > +}; > + > +/* password configuration details */ > +struct tlmi_pwdcfg { > + uint32_t password_mode; > + uint32_t password_state; > + uint32_t min_length; > + uint32_t max_length; > + uint32_t supported_encodings; > + uint32_t supported_keyboard; > +}; > + > +/* password setting details */ > +struct tlmi_pwd_setting { > + struct kobject kobj; > + bool valid; > + char display_name[TLMI_PWDTYPE_MAXLEN]; > + char password[TLMI_PWD_MAXLEN]; > + char pwd_type[TLMI_PWDTYPE_LEN]; Note you can just make this a "const char *" and init it like this: pwd_type = "pop"; The "pop" string will be part of the const data section so its address will still be valid after the function has exited. > + int minlen; > + int maxlen; > + enum encoding_option encoding; > + char kbdlang[TLMI_LANG_MAXLEN]; > +}; > + > +/* Attribute setting details */ > +struct tlmi_attr_setting { > + struct kobject kobj; > + int index; > + char display_name[TLMI_SETTINGS_MAXLEN]; > + char *possible_values; > +}; > + > +struct think_lmi { > + struct wmi_device *wmi_device; > + > + int settings_count; > + bool can_set_bios_settings; > + bool can_get_bios_selections; > + bool can_set_bios_password; > + bool can_get_password_settings; > + > + struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT]; > + struct device *class_dev; > + struct kset *attribute_kset; > + struct kset *authentication_kset; > + struct tlmi_pwd_setting *pwd_admin; > + struct tlmi_pwd_setting *pwd_power; > +}; > + > +#endif /* !_THINK_LMI_H_ */ > + > Regards, Hans
Thanks for the review Hans, On 2021-05-19 1:06 p.m., Hans de Goede wrote: > Hi, > > On 5/9/21 3:57 AM, Mark Pearson wrote: <snip> >> >> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes >> index 8ea59fea4..31d1becfa 100644 >> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes >> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes >> @@ -185,6 +185,17 @@ Description: >> A write only value that when used in tandem with >> current_password will reset a system or admin password. >> >> + encoding: >> + For platforms that require it (currently Lenovo) the >> + encoding method that is used. This can be either "ascii" >> + or "scancode". Default is set to "ascii" >> + >> + kbdlang: >> + For platforms that require it (currently Lenovo only) the >> + keyboard language method that is used. This is generally a >> + two char code (e.g. "us", "fr", "gr") and may vary per platform. >> + Default is set to "us" >> + > > I should have caught this during my review of v1, these are Lenovo specific, so please prefix > them with lenovo_ (the same is already done for dell specific sysfs attributes) and move them > to a separate "Lenovo specific class extensions" part of the doc. > No problem <snip> >> >> What: /sys/class/firmware-attributes/*/attributes/pending_reboot >> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >> index b0e1e5f65..0511c54fc 100644 >> --- a/drivers/platform/x86/Kconfig >> +++ b/drivers/platform/x86/Kconfig >> @@ -639,6 +639,18 @@ config THINKPAD_ACPI_HOTKEY_POLL >> If you are not sure, say Y here. The driver enables polling only if >> it is strictly necessary to do so. >> >> +config THINKPAD_LMI >> + tristate "Lenovo WMI-based systems management driver" >> + default m > > default n (or no default at all) please. Ack <snip> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> new file mode 100644 >> index 000000000..2fa989e98 >> --- /dev/null >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -0,0 +1,890 @@ <snip> >> + >> +/* ---- Authentication sysfs --------------------------------------------------------- */ >> +static ssize_t is_enabled_show(struct kobject *kobj, struct kobj_attribute *attr, >> + char *buf) >> +{ >> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >> + >> + if (setting->valid) >> + return sysfs_emit(buf, "1\n"); >> + else >> + return sysfs_emit(buf, "0\n"); > > Maybe use: > > sysfs_emit(buf, "%d\n", settings->valid); > > instead? Definitely :) > > >> +} >> + >> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); >> + >> +static ssize_t current_password_store(struct kobject *kobj, >> + struct kobj_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >> + int length; >> + >> + length = strlen(buf); >> + if (buf[length-1] == '\n') >> + length--; >> + >> + if (length >= TLMI_PWD_MAXLEN) >> + return -EINVAL; >> + >> + memcpy(setting->password, buf, length); >> + setting->password[length] = '\0'; >> + return count; >> +} >> + >> +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password); >> + >> +static ssize_t new_password_store(struct kobject *kobj, >> + struct kobj_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >> + char *p, *new_pwd; >> + char *auth_str; >> + int ret = 0, len; >> + >> + if (!capable(CAP_SYS_ADMIN)) >> + return -EPERM; >> + >> + if (!tlmi_priv.can_set_bios_password) >> + return -EOPNOTSUPP; >> + >> + new_pwd = kstrdup(buf, GFP_KERNEL); >> + if (!new_pwd) >> + return -ENOMEM; >> + >> + p = strchr(new_pwd, '\n'); >> + if (p) >> + *p = '\0'; >> + >> + if (strlen(new_pwd) > setting->maxlen) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ >> + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) >> + + strlen(setting->kbdlang) + 3 /* type */ >> + + strlen(new_pwd) + 6 /* punctuation and terminator*/; >> + auth_str = kzalloc(len, GFP_KERNEL); >> + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", >> + setting->pwd_type, setting->password, new_pwd, >> + encoding_options[setting->encoding], setting->kbdlang); >> + ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str); > > So I guess on success any subsequent calls need to use the new password, > so the user is expected to write the new password to the current_password > file after changing the password this way? > > I just checked the dell-wmi-sysman code and that does this: > > /* clear current_password here and use user input from wmi_priv.current_password */ > if (!ret) > memset(current_password, 0, MAX_BUFF); > > Where current_password points to either the user or admin cached password, > depending on which one is being changed. > > So that seems to work the same way as what you are doing here (the user needs to > write the new password to current_password after changing it through the > new_password sysfs attribute). Can you add a patch to the patch-set documenting > this in Documentation/ABI/testing/sysfs-class-firmware-attributes ? > > Also you may want to consider clearing out the old cached password on success > like the Dell code is doing. > Would you have any objections/concerns if, on a successful password update, this function just updates the stored password setting to the new password. Seems nicer from a user point of view then having to go and feed the current_password field again. e.g: strncpy(new_pwd, setting->password, TLMI_PWD_MAXLEN) I know it would make Dell and Lenovo operate differently (I can add that detail to the documentation) but it just feels like a nicer design. <snip> >> + >> +static struct kobj_attribute auth_kbdlang = __ATTR_RW(kbdlang); >> + >> +{ >> + if (strcmp(kobj->name, "Admin") == 0) >> + return sprintf(buf, "bios-admin\n"); >> + else if (strcmp(kobj->name, "System") == 0) >> + return sprintf(buf, "power-on\n"); >> + return -EIO; >> +} > > IMHO it would be nice to add a "const char *role" to struct tlmi_pwd_setting and > then change this to: > >> +static ssize_t encoding_show(struct kobject *kobj, struct kobj_attribute *attr, >> + char *buf) >> +{ >> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >> + >> + return sysfs_emit(buf, "%s\n", setting->role); >> +} > > That would make this function consistent with the other password related functions. Agreed - I'll make that change <snip> >> +static ssize_t current_value_store(struct kobject *kobj, >> + struct kobj_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); >> + int alloc_len, auth_len = 0; >> + int str_ix = 0; >> + char *auth_str = NULL; >> + char *set_str, *new_setting, *p; >> + int ret; >> + >> + if (!tlmi_priv.can_set_bios_settings) >> + return -EOPNOTSUPP; >> + >> + new_setting = kstrdup(buf, GFP_KERNEL); >> + if (!new_setting) >> + return -ENOMEM; >> + p = strchr(new_setting, '\n'); >> + if (p) >> + *p = '\0'; >> + >> + alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting); >> + >> + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) { >> + auth_len += strlen(tlmi_priv.pwd_admin->password) + 1 >> + + strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1 >> + + strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1; >> + auth_str = kmalloc(auth_len, GFP_KERNEL); >> + if (!auth_str) { >> + ret = -ENOMEM; >> + goto out; > > You end-up kfree-ing set_str here without it being initialized (compiler warning?) > Please just initialize all 3 strings which you kfree at the end to NULL, I know > this is not necessary for at least the new_setting string but it makes it easier > for someone reading the code to verify that the kfree() does not happens on > an uninitialized pointer. Agreed. Thanks. > >> + } >> + >> + sprintf(auth_str, "%s,%s,%s;", >> + tlmi_priv.pwd_admin->password, >> + encoding_options[tlmi_priv.pwd_admin->encoding], >> + tlmi_priv.pwd_admin->kbdlang); >> + alloc_len += auth_len; >> + } >> + >> + set_str = kmalloc(alloc_len, GFP_KERNEL); >> + if (!set_str) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting); >> + >> + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) > > Maybe use: > > if (auth_str) > > here? This way if the condition of the above if block ever changes, you can't > end up passing a NULL auth_Str to the sprintf. Agreed - makes sense <snip> >> + >> +static const struct sysfs_ops tlmi_kobj_sysfs_ops = { >> + .show = tlmi_attr_show, >> + .store = tlmi_attr_store, >> +}; >> + >> +static struct kobj_type attr_name_ktype = { >> + .sysfs_ops = &tlmi_kobj_sysfs_ops, >> +}; > > You need to re-add a release function here, sorry I was not clear > about that in my review of v1. Since you do a kobject_put and not a > direct kfree() on cleanup, you need to define a release function to > do the actual free once the refcount drops to 0. > > And since you have 2 kind of structs embedding the kobjects you will > need 2 release functions: > > static void tmli_attr_setting_release(struct kobject *kobj) > { > struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); > > kfree(setting); > } > > static void tmli_pwd_setting_release(struct kobject *kobj) > { > struct tlmi_pwd_setting *pwd = container_of(kobj, struct tlmi_pwd_setting, kobj); > > kfree(pwd); > } > > And then have 2 kobject-types: > > static struct kobj_type tmli_attr_setting_ktype = { > .release = tmli_attr_setting_release, > .sysfs_ops = &tlmi_kobj_sysfs_ops, > }; > > static struct kobj_type tmli_pwd_setting_ktype = { > .release = tmli_pwd_setting_release, > .sysfs_ops = &tlmi_kobj_sysfs_ops, > }; > > And use the right type when creating the objects. > Ah - got it. I had a feeling I was missing something here. Thanks! <snip> >> + >> +module_wmi_driver(tlmi_driver); >> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h >> new file mode 100644 >> index 000000000..2c1484304 >> --- /dev/null >> +++ b/drivers/platform/x86/think-lmi.h >> @@ -0,0 +1,73 @@ <snip>>> + >> +/* password setting details */ >> +struct tlmi_pwd_setting { >> + struct kobject kobj; >> + bool valid; >> + char display_name[TLMI_PWDTYPE_MAXLEN]; >> + char password[TLMI_PWD_MAXLEN]; >> + char pwd_type[TLMI_PWDTYPE_LEN]; > > Note you can just make this a "const char *" and init it like this: > > pwd_type = "pop"; > > The "pop" string will be part of the const data section so its > address will still be valid after the function has exited. > Ack
Hi Mark, On 5/20/21 7:18 PM, Mark Pearson wrote: > Thanks for the review Hans, You're welcome. > On 2021-05-19 1:06 p.m., Hans de Goede wrote: >> Hi, >> >> On 5/9/21 3:57 AM, Mark Pearson wrote: > <snip> >>> >>> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes >>> index 8ea59fea4..31d1becfa 100644 >>> --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes >>> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes >>> @@ -185,6 +185,17 @@ Description: >>> A write only value that when used in tandem with >>> current_password will reset a system or admin password. >>> >>> + encoding: >>> + For platforms that require it (currently Lenovo) the >>> + encoding method that is used. This can be either "ascii" >>> + or "scancode". Default is set to "ascii" >>> + >>> + kbdlang: >>> + For platforms that require it (currently Lenovo only) the >>> + keyboard language method that is used. This is generally a >>> + two char code (e.g. "us", "fr", "gr") and may vary per platform. >>> + Default is set to "us" >>> + >> >> I should have caught this during my review of v1, these are Lenovo specific, so please prefix >> them with lenovo_ (the same is already done for dell specific sysfs attributes) and move them >> to a separate "Lenovo specific class extensions" part of the doc. >> > No problem > > <snip> > >>> >>> What: /sys/class/firmware-attributes/*/attributes/pending_reboot >>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >>> index b0e1e5f65..0511c54fc 100644 >>> --- a/drivers/platform/x86/Kconfig >>> +++ b/drivers/platform/x86/Kconfig >>> @@ -639,6 +639,18 @@ config THINKPAD_ACPI_HOTKEY_POLL >>> If you are not sure, say Y here. The driver enables polling only if >>> it is strictly necessary to do so. >>> >>> +config THINKPAD_LMI >>> + tristate "Lenovo WMI-based systems management driver" >>> + default m >> >> default n (or no default at all) please. > Ack > > <snip> >>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >>> new file mode 100644 >>> index 000000000..2fa989e98 >>> --- /dev/null >>> +++ b/drivers/platform/x86/think-lmi.c >>> @@ -0,0 +1,890 @@ > <snip> >>> + >>> +/* ---- Authentication sysfs --------------------------------------------------------- */ >>> +static ssize_t is_enabled_show(struct kobject *kobj, struct kobj_attribute *attr, >>> + char *buf) >>> +{ >>> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >>> + >>> + if (setting->valid) >>> + return sysfs_emit(buf, "1\n"); >>> + else >>> + return sysfs_emit(buf, "0\n"); >> >> Maybe use: >> >> sysfs_emit(buf, "%d\n", settings->valid); >> >> instead? > Definitely :) > >> >> >>> +} >>> + >>> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); >>> + >>> +static ssize_t current_password_store(struct kobject *kobj, >>> + struct kobj_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >>> + int length; >>> + >>> + length = strlen(buf); >>> + if (buf[length-1] == '\n') >>> + length--; >>> + >>> + if (length >= TLMI_PWD_MAXLEN) >>> + return -EINVAL; >>> + >>> + memcpy(setting->password, buf, length); >>> + setting->password[length] = '\0'; >>> + return count; >>> +} >>> + >>> +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password); >>> + >>> +static ssize_t new_password_store(struct kobject *kobj, >>> + struct kobj_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >>> + char *p, *new_pwd; >>> + char *auth_str; >>> + int ret = 0, len; >>> + >>> + if (!capable(CAP_SYS_ADMIN)) >>> + return -EPERM; >>> + >>> + if (!tlmi_priv.can_set_bios_password) >>> + return -EOPNOTSUPP; >>> + >>> + new_pwd = kstrdup(buf, GFP_KERNEL); >>> + if (!new_pwd) >>> + return -ENOMEM; >>> + >>> + p = strchr(new_pwd, '\n'); >>> + if (p) >>> + *p = '\0'; >>> + >>> + if (strlen(new_pwd) > setting->maxlen) { >>> + ret = -EINVAL; >>> + goto out; >>> + } >>> + >>> + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ >>> + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) >>> + + strlen(setting->kbdlang) + 3 /* type */ >>> + + strlen(new_pwd) + 6 /* punctuation and terminator*/; >>> + auth_str = kzalloc(len, GFP_KERNEL); >>> + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", >>> + setting->pwd_type, setting->password, new_pwd, >>> + encoding_options[setting->encoding], setting->kbdlang); >>> + ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str); >> >> So I guess on success any subsequent calls need to use the new password, >> so the user is expected to write the new password to the current_password >> file after changing the password this way? >> >> I just checked the dell-wmi-sysman code and that does this: >> >> /* clear current_password here and use user input from wmi_priv.current_password */ >> if (!ret) >> memset(current_password, 0, MAX_BUFF); >> >> Where current_password points to either the user or admin cached password, >> depending on which one is being changed. >> >> So that seems to work the same way as what you are doing here (the user needs to >> write the new password to current_password after changing it through the >> new_password sysfs attribute). Can you add a patch to the patch-set documenting >> this in Documentation/ABI/testing/sysfs-class-firmware-attributes ? >> >> Also you may want to consider clearing out the old cached password on success >> like the Dell code is doing. >> > Would you have any objections/concerns if, on a successful password > update, this function just updates the stored password setting to the > new password. > Seems nicer from a user point of view then having to go and feed the > current_password field again. > e.g: strncpy(new_pwd, setting->password, TLMI_PWD_MAXLEN) Please use strscpy, strncpy has horrible syntax and using it is easy to get wrong. e.g. the above example is wrong if strlen(new_pwd) >= TLMI_PWD_MAXLEN the resulting setting->password will not be 0 terminated (and you seem to have swapped src and dst, but that is unrelated to strncpy being a sucky function). This also make me realize that the code has 2 max-pwd-lengths: setting->maxlen TLMI_PWD_MAXLEN I think you should put a: if (WARN_ON(pwdcfg.max_length > TLMI_PWD_MAXLEN)) pwdcfg.max_length = TLMI_PWD_MAXLEN; in tlmi_analyze() so that we get a kernel-backtrace (and thus bugreports if this condition ever becomes true. And then use pwdcfg.max_length everywhere (e.g. also for the check in current_password_store()). Also the length checks in current_password_store() and new_password_store() should probably also check against settings->minlen ? > I know it would make Dell and Lenovo operate differently (I can add that > detail to the documentation) but it just feels like a nicer design. That works for me. Perhaps you can also do a (compile tested only) RFC patch for the Dell code to do the same thing (replace the memset 0 with the strscpy) to see if the Dell folks are ok with also doing things this way ? Regards, Hans
Hi, On 5/21/21 10:10 AM, Hans de Goede wrote: <snip> Some minor corrections to my last email: > This also make me realize that the code has 2 max-pwd-lengths: > > setting->maxlen > TLMI_PWD_MAXLEN > > I think you should put a: > > if (WARN_ON(pwdcfg.max_length > TLMI_PWD_MAXLEN)) > pwdcfg.max_length = TLMI_PWD_MAXLEN; This needs to be: if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_MAXEN)) pwdcfg.max_length = TLMI_PWD_MAXLEN - 1; To account for the terminating 0 (maybe bump TLMI_PWD_MAXEN with 1 and rename it to TLMI_PWD_BUFSIZE?) > in tlmi_analyze() so that we get a kernel-backtrace (and thus bugreports > if this condition ever becomes true. > > And then use pwdcfg.max_length everywhere (e.g. also for the check in > current_password_store()). > > Also the length checks in current_password_store() and new_password_store() > should probably also check against settings->minlen ? I just realized that current_password_store() should also allow a length of 0 to clear the password when the user is done making changes, so the check in current_password_store() should be something like this: /* len == 0 is allowed to clear the password */ if (len != 0 && (len < setting->minlen || len > setting->maxlen)) return -EINVAL; Regards, Gans
Thanks Hans On 2021-05-21 4:10 a.m., Hans de Goede wrote: > Hi Mark, > > On 5/20/21 7:18 PM, Mark Pearson wrote: <snip> >>>> + + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' >>>> */ + len = strlen(setting->password) + >>>> strlen(encoding_options[setting->encoding]) + + >>>> strlen(setting->kbdlang) + 3 /* type */ + + strlen(new_pwd) + >>>> 6 /* punctuation and terminator*/; + auth_str = kzalloc(len, >>>> GFP_KERNEL); + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", + >>>> setting->pwd_type, setting->password, new_pwd, + >>>> encoding_options[setting->encoding], setting->kbdlang); + ret = >>>> tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str); >>> >>> So I guess on success any subsequent calls need to use the new >>> password, so the user is expected to write the new password to >>> the current_password file after changing the password this way? >>> >>> I just checked the dell-wmi-sysman code and that does this: >>> >>> /* clear current_password here and use user input from >>> wmi_priv.current_password */ if (!ret) memset(current_password, >>> 0, MAX_BUFF); >>> >>> Where current_password points to either the user or admin cached >>> password, depending on which one is being changed. >>> >>> So that seems to work the same way as what you are doing here >>> (the user needs to write the new password to current_password >>> after changing it through the new_password sysfs attribute). Can >>> you add a patch to the patch-set documenting this in >>> Documentation/ABI/testing/sysfs-class-firmware-attributes ? >>> >>> Also you may want to consider clearing out the old cached >>> password on success like the Dell code is doing. >>> >> Would you have any objections/concerns if, on a successful >> password update, this function just updates the stored password >> setting to the new password. Seems nicer from a user point of view >> then having to go and feed the current_password field again. e.g: >> strncpy(new_pwd, setting->password, TLMI_PWD_MAXLEN) > > Please use strscpy, strncpy has horrible syntax and using it is easy > to get wrong. > > e.g. the above example is wrong if strlen(new_pwd) >= > TLMI_PWD_MAXLEN the resulting setting->password will not be 0 > terminated (and you seem to have swapped src and dst, but that is > unrelated to strncpy being a sucky function). Yeah, sorry, typed as an example only, and will use strscpy > > This also make me realize that the code has 2 max-pwd-lengths: > > setting->maxlen TLMI_PWD_MAXLEN > > I think you should put a: > > if (WARN_ON(pwdcfg.max_length > TLMI_PWD_MAXLEN)) pwdcfg.max_length = > TLMI_PWD_MAXLEN; > > in tlmi_analyze() so that we get a kernel-backtrace (and thus > bugreports if this condition ever becomes true. Sounds good > > And then use pwdcfg.max_length everywhere (e.g. also for the check > in current_password_store()). > > Also the length checks in current_password_store() and > new_password_store() should probably also check against > settings->minlen ? Agreed - will add > > >> I know it would make Dell and Lenovo operate differently (I can add >> that detail to the documentation) but it just feels like a nicer >> design. > > That works for me. Perhaps you can also do a (compile tested only) > RFC patch for the Dell code to do the same thing (replace the memset > 0 with the strscpy) to see if the Dell folks are ok with also doing > things this way ? > I'm not hugely comfortable with that. If for some reason it broke things for Dell customers I wouldn't want to be responsible :) I'd rather they made the changes and were able to test it - I know that's what I'd prefer if it was the other way around. Apologies if I'm being over cautious! I've added the new Dell kernel group email to the thread so they're aware :) Mark
Hi, On 5/21/21 5:55 PM, Mark Pearson wrote: <snip> >>> I know it would make Dell and Lenovo operate differently (I can add >>> that detail to the documentation) but it just feels like a nicer >>> design. >> >> That works for me. Perhaps you can also do a (compile tested only) >> RFC patch for the Dell code to do the same thing (replace the memset >> 0 with the strscpy) to see if the Dell folks are ok with also doing >> things this way ? >> > I'm not hugely comfortable with that. If for some reason it broke things > for Dell customers I wouldn't want to be responsible :) Right, that is why I suggested making it a RFC patch and I would certainly not apply that patch without it being tested by Dell first. The idea behind the patch is for it to be a way to get a discussion about this started. In my experience patches tend to get more of a reaction then hypothetical discussions about changes :) > I'd rather they > made the changes and were able to test it - I know that's what I'd > prefer if it was the other way around. Apologies if I'm being over cautious! If you don't feel comfortable doing this, that is fine, lets wait what the Dell folks have to say; and if they don't respond I might do a RFC myself. > I've added the new Dell kernel group email to the thread so they're aware :) Thanks. Regards, Hans
On 2021-05-21 12:55 p.m., Hans de Goede wrote: > Hi, > > On 5/21/21 5:55 PM, Mark Pearson wrote: > > <snip> > >>>> I know it would make Dell and Lenovo operate differently (I can add >>>> that detail to the documentation) but it just feels like a nicer >>>> design. >>> >>> That works for me. Perhaps you can also do a (compile tested only) >>> RFC patch for the Dell code to do the same thing (replace the memset >>> 0 with the strscpy) to see if the Dell folks are ok with also doing >>> things this way ? >>> >> I'm not hugely comfortable with that. If for some reason it broke things >> for Dell customers I wouldn't want to be responsible :) > > Right, that is why I suggested making it a RFC patch and I would > certainly not apply that patch without it being tested by Dell first. > > The idea behind the patch is for it to be a way to get a discussion > about this started. In my experience patches tend to get more of > a reaction then hypothetical discussions about changes :) > >> I'd rather they >> made the changes and were able to test it - I know that's what I'd >> prefer if it was the other way around. Apologies if I'm being over cautious! > > If you don't feel comfortable doing this, that is fine, lets wait what > the Dell folks have to say; and if they don't respond I might do a RFC > myself. > Ah - I'd misunderstood that point. I have no issues with that :) I won't update the document and publish a RFC instead as the last patch in the series Mark
On Sun, May 9, 2021 at 5:02 AM Mark Pearson <markpearson@lenovo.com> wrote: > > For Lenovo platforms that support a WMI interface to the BIOS add > support, using the firmware-attributes class, to allow users to access > and modify various BIOS related settings. Few comments below. ... > +/* Convert BIOS WMI error string to suitable error code */ Can it be rather the mapping structure? In that case you move string literals, error codes and comments to its entries. Here will be something like for-loop. ... > +static int tlmi_extract_error(const struct acpi_buffer *output) > +{ > + const union acpi_object *obj; > + int ret; > + > + obj = output->pointer; > + if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { I would split !obj check, but I don't know if it is even a useful check. Can it be the case? > + kfree(obj); Basically you double check for NULL (see above) Same for other similar places in the code. > + return -EIO; > + } > + > + ret = tlmi_errstr_to_err(obj->string.pointer); > + kfree(obj); > + return ret; What I really do not like here is that you are freeing something out of the scope. Please, free it where it was acquired. > +} ... > + /* > + * duplicated call required to match bios workaround for behavior > + * seen when WMI accessed via scripting on other OS /* * Multi-line comments * should have this kind of * style. Pay attention to the details. */ > + */ ... > + *string = kstrdup(obj->string.pointer, GFP_KERNEL); > + kfree(obj); > + return *string ? 0 : -ENOMEM; This breaks the principle "don't touch the output in error case". ... > + if (setting->valid) > + return sysfs_emit(buf, "1\n"); > + else > + return sysfs_emit(buf, "0\n"); sysfs_emit(buf, "%d\n", !!setting->valid); ? ... > +static ssize_t current_password_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > + int length; > + length = strlen(buf); > + if (buf[length-1] == '\n') > + length--; This will prevent you from using \n in the password. Why? > + if (length >= TLMI_PWD_MAXLEN) > + return -EINVAL; I guess password *string* length is wrong per se. count seems the proper one which one should use. > + > + memcpy(setting->password, buf, length); > + setting->password[length] = '\0'; Why is the password a *string*? From where that assumption comes from? > + return count; > +} > + > +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password); ... > + p = strchr(new_pwd, '\n'); > + if (p) > + *p = '\0'; strtrim(). But see above. > + if (strlen(new_pwd) > setting->maxlen) { > + ret = -EINVAL; > + goto out; > + } ... > + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ > + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) > + + strlen(setting->kbdlang) + 3 /* type */ > + + strlen(new_pwd) + 6 /* punctuation and terminator*/; > + auth_str = kzalloc(len, GFP_KERNEL); > + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", > + setting->pwd_type, setting->password, new_pwd, > + encoding_options[setting->encoding], setting->kbdlang); NIH of kasprintf() ... > + return ret ? ret : count; return ret ?: count; is shorter. ... > + if (strcmp(kobj->name, "Admin") == 0) > + return sprintf(buf, "bios-admin\n"); > + else if (strcmp(kobj->name, "System") == 0) Redundant 'else' > + return sprintf(buf, "power-on\n"); > + return -EIO; ... > +static struct attribute *auth_attrs[] = { > + &auth_is_pass_set.attr, > + &auth_min_pass_length.attr, > + &auth_max_pass_length.attr, > + &auth_current_password.attr, > + &auth_new_password.attr, > + &auth_role.attr, > + &auth_mechanism.attr, > + &auth_encoding.attr, > + &auth_kbdlang.attr, > + NULL, The terminator line doesn't need a comma. > +}; ... > + new_setting = kstrdup(buf, GFP_KERNEL); strtrim(buf) ? > + if (!new_setting) > + return -ENOMEM; > + p = strchr(new_setting, '\n'); > + if (p) > + *p = '\0'; See above. > + alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting); > + > + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) { > + auth_len += strlen(tlmi_priv.pwd_admin->password) + 1 > + + strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1 > + + strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1; > + auth_str = kmalloc(auth_len, GFP_KERNEL); > + if (!auth_str) { > + ret = -ENOMEM; > + goto out; > + } > + > + sprintf(auth_str, "%s,%s,%s;", > + tlmi_priv.pwd_admin->password, > + encoding_options[tlmi_priv.pwd_admin->encoding], > + tlmi_priv.pwd_admin->kbdlang); > + alloc_len += auth_len; HIN of kasprintf() > + } > + > + set_str = kmalloc(alloc_len, GFP_KERNEL); > + if (!set_str) { > + ret = -ENOMEM; > + goto out; > + } > + > + str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting); Ditto, > + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) > + sprintf(set_str + str_ix, ",%s", auth_str); > + else > + sprintf(set_str + str_ix, ";"); > + > + ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str); > + if (ret) > + goto out; > + > + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) > + ret = tlmi_save_bios_settings(auth_str); > + else > + ret = tlmi_save_bios_settings(""); > + if (ret) > + goto out; Useless. > +out: > + kfree(auth_str); > + kfree(set_str); > + kfree(new_setting); > + return ret ? ret : count; Can be shorter. > +} > +static struct kobj_attribute attr_displ_name = > + __ATTR_RO(display_name); One line (and so on for all of them). ... > +static struct attribute *tlmi_attrs[] = { > + &attr_displ_name.attr, > + &attr_current_val.attr, > + &attr_possible_values.attr, > + NULL, No comma. > +}; ... > + struct kobj_attribute *kattr; > + ssize_t ret = -EIO; Useless. Use direct return. > + > + kattr = container_of(attr, struct kobj_attribute, attr); > + if (kattr->show) > + ret = kattr->show(kobj, kattr, buf); > + return ret; ... > + struct kobj_attribute *kattr; > + ssize_t ret = -EIO; > + > + kattr = container_of(attr, struct kobj_attribute, attr); > + if (kattr->store) > + ret = kattr->store(kobj, kattr, buf, count); > + return ret; As above. ... > + for (i = 0; i < TLMI_SETTINGS_COUNT; i++) { > + /*Check if index is a valid setting - skip if it isn't */ Missed space. > + if (!tlmi_priv.setting[i]) > + continue; > + > + /* build attribute */ > + tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset; > + ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &attr_name_ktype, > + NULL, "%s", tlmi_priv.setting[i]->display_name); > + if (ret) > + goto fail_create_attr; > + > + ret = sysfs_create_group(&tlmi_priv.setting[i]->kobj, &tlmi_attr_group); > + if (ret) > + goto fail_create_attr; > + } ... > + int i = 0; Why assignment? > + for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) { > + char *item = NULL; > + int spleng = 0; > + int num = 0; > + char *p; > + struct tlmi_attr_setting *setting; > + > + tlmi_priv.setting[i] = NULL; > + status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); > + if (ACPI_FAILURE(status)) > + break; > + if (!item) > + break; > + if (!*item) > + continue; > + > + /* It is not allowed to have '/' for file name. Convert it into '\'. */ > + spleng = strlen(item); > + for (num = 0; num < spleng; num++) { > + if (item[num] == '/') > + item[num] = '\\'; > + } NIH of strreplace(), but please check its name. > + > + /* Remove the value part */ > + p = strchr(item, ','); > + if (p) > + *p = '\0'; > + > + /* Create a setting entry */ > + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); > + if (!setting) { > + ret = -ENOMEM; > + goto fail_clear_attr; > + } > + setting->index = i; > + strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN); > + /* If BIOS selections supported, load those */ > + if (tlmi_priv.can_get_bios_selections) { > + ret = tlmi_get_bios_selections(setting->display_name, > + &setting->possible_values); > + if (ret || !setting->possible_values) > + pr_info("Error retrieving possible values for %d : %s\n", > + i, setting->display_name); > + } > + tlmi_priv.setting[i] = setting; > + tlmi_priv.settings_count++; > + kfree(item); > + } I stopped here because it's full of such small issues. Basically the summary - try to find the existing APIs for routines that quite often in use (usually string operations) - try to make your code compact with the same level of readability
Hi, > -----Original Message----- > From: Hans de Goede <hdegoede@redhat.com> > Sent: Friday, May 21, 2021 10:25 PM > To: Mark Pearson > Cc: mgross@linux.intel.com; platform-driver-x86@vger.kernel.org; Bharathi, > Divya; Ksr, Prasanth; Dell Client Kernel > Subject: Re: [External] Re: [PATCH v2 3/3] platform/x86: think-lmi: Add WMI > interface support on Lenovo platforms > > > [EXTERNAL EMAIL] > > Hi, > > On 5/21/21 5:55 PM, Mark Pearson wrote: > > <snip> > > >>> I know it would make Dell and Lenovo operate differently (I can add > >>> that detail to the documentation) but it just feels like a nicer > >>> design. > >> > >> That works for me. Perhaps you can also do a (compile tested only) > >> RFC patch for the Dell code to do the same thing (replace the memset > >> 0 with the strscpy) to see if the Dell folks are ok with also doing > >> things this way ? > >> > > I'm not hugely comfortable with that. If for some reason it broke > > things for Dell customers I wouldn't want to be responsible :) > > Right, that is why I suggested making it a RFC patch and I would certainly not > apply that patch without it being tested by Dell first. > > The idea behind the patch is for it to be a way to get a discussion about this > started. In my experience patches tend to get more of a reaction then > hypothetical discussions about changes :) > > > I'd rather they > > made the changes and were able to test it - I know that's what I'd > > prefer if it was the other way around. Apologies if I'm being over cautious! > > If you don't feel comfortable doing this, that is fine, lets wait what the Dell > folks have to say; and if they don't respond I might do a RFC myself. > Ack. We will implement the same from Dell side as well to have uniformity and seems nicer from a user point of view rather than populating the current_password field again in case of password change scenario. > > I've added the new Dell kernel group email to the thread so they're > > aware :) > > Thanks. > > Regards, > > Hans
Hi, On 5/24/21 12:19 PM, Ksr, Prasanth wrote: > Hi, > >> -----Original Message----- >> From: Hans de Goede <hdegoede@redhat.com> >> Sent: Friday, May 21, 2021 10:25 PM >> To: Mark Pearson >> Cc: mgross@linux.intel.com; platform-driver-x86@vger.kernel.org; Bharathi, >> Divya; Ksr, Prasanth; Dell Client Kernel >> Subject: Re: [External] Re: [PATCH v2 3/3] platform/x86: think-lmi: Add WMI >> interface support on Lenovo platforms >> >> >> [EXTERNAL EMAIL] >> >> Hi, >> >> On 5/21/21 5:55 PM, Mark Pearson wrote: >> >> <snip> >> >>>>> I know it would make Dell and Lenovo operate differently (I can add >>>>> that detail to the documentation) but it just feels like a nicer >>>>> design. >>>> >>>> That works for me. Perhaps you can also do a (compile tested only) >>>> RFC patch for the Dell code to do the same thing (replace the memset >>>> 0 with the strscpy) to see if the Dell folks are ok with also doing >>>> things this way ? >>>> >>> I'm not hugely comfortable with that. If for some reason it broke >>> things for Dell customers I wouldn't want to be responsible :) >> >> Right, that is why I suggested making it a RFC patch and I would certainly not >> apply that patch without it being tested by Dell first. >> >> The idea behind the patch is for it to be a way to get a discussion about this >> started. In my experience patches tend to get more of a reaction then >> hypothetical discussions about changes :) >> >>> I'd rather they >>> made the changes and were able to test it - I know that's what I'd >>> prefer if it was the other way around. Apologies if I'm being over cautious! >> >> If you don't feel comfortable doing this, that is fine, lets wait what the Dell >> folks have to say; and if they don't respond I might do a RFC myself. >> > > Ack. We will implement the same from Dell side as well to have uniformity and > seems nicer from a user point of view rather than populating the > current_password field again in case of password change scenario. Great, thank you. Regards, Hans
Hi On 2021-05-24 11:27 a.m., Hans de Goede wrote: > Hi, > > On 5/24/21 12:19 PM, Ksr, Prasanth wrote: >> Hi, >> >>> -----Original Message----- From: Hans de Goede >>> <hdegoede@redhat.com> Sent: Friday, May 21, 2021 10:25 PM To: >>> Mark Pearson Cc: mgross@linux.intel.com; >>> platform-driver-x86@vger.kernel.org; Bharathi, Divya; Ksr, >>> Prasanth; Dell Client Kernel Subject: Re: [External] Re: [PATCH >>> v2 3/3] platform/x86: think-lmi: Add WMI interface support on >>> Lenovo platforms >>> >>> >>> [EXTERNAL EMAIL] >>> >>> Hi, >>> >>> On 5/21/21 5:55 PM, Mark Pearson wrote: >>> >>> <snip> >>> >>>>>> I know it would make Dell and Lenovo operate differently (I >>>>>> can add that detail to the documentation) but it just feels >>>>>> like a nicer design. >>>>> >>>>> That works for me. Perhaps you can also do a (compile tested >>>>> only) RFC patch for the Dell code to do the same thing >>>>> (replace the memset 0 with the strscpy) to see if the Dell >>>>> folks are ok with also doing things this way ? >>>>> >>>> I'm not hugely comfortable with that. If for some reason it >>>> broke things for Dell customers I wouldn't want to be >>>> responsible :) >>> >>> Right, that is why I suggested making it a RFC patch and I would >>> certainly not apply that patch without it being tested by Dell >>> first. >>> >>> The idea behind the patch is for it to be a way to get a >>> discussion about this started. In my experience patches tend to >>> get more of a reaction then hypothetical discussions about >>> changes :) >>> >>>> I'd rather they made the changes and were able to test it - I >>>> know that's what I'd prefer if it was the other way around. >>>> Apologies if I'm being over cautious! >>> >>> If you don't feel comfortable doing this, that is fine, lets wait >>> what the Dell folks have to say; and if they don't respond I >>> might do a RFC myself. >>> >> >> Ack. We will implement the same from Dell side as well to have >> uniformity and seems nicer from a user point of view rather than >> populating the current_password field again in case of password >> change scenario. > Just as a note, testing this over the weekend, I found that the password change doesn't become active until after a reboot on our systems. So, at least on our systems, copying over strings is pointless. I'll just document the behaviour in the document for our systems, and see if we can improve it in our firmware going forward. Thanks Mark
Hi Andy, Many thanks for the review On 2021-05-22 7:04 a.m., Andy Shevchenko wrote: > On Sun, May 9, 2021 at 5:02 AM Mark Pearson <markpearson@lenovo.com> wrote: >> >> For Lenovo platforms that support a WMI interface to the BIOS add >> support, using the firmware-attributes class, to allow users to access >> and modify various BIOS related settings. > > Few comments below. > > ... > >> +/* Convert BIOS WMI error string to suitable error code */ > > Can it be rather the mapping structure? In that case you move string > literals, error codes and comments to its entries. > Here will be something like for-loop. > Yes, I can do this > ... > >> +static int tlmi_extract_error(const struct acpi_buffer *output) >> +{ >> + const union acpi_object *obj; >> + int ret; >> + >> + obj = output->pointer; >> + if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { > > I would split !obj check, but I don't know if it is even a useful > check. Can it be the case? I wasn't certain on if obj could ever be null, I suspect not but as I wasn't 100% sure I put the check in just in case. > >> + kfree(obj); > > Basically you double check for NULL (see above) > > Same for other similar places in the code. > OK - understood. Thanks. >> + return -EIO; >> + } >> + >> + ret = tlmi_errstr_to_err(obj->string.pointer); >> + kfree(obj); >> + return ret; > > What I really do not like here is that you are freeing something out > of the scope. Please, free it where it was acquired. Ack > >> +} > > ... > >> + /* >> + * duplicated call required to match bios workaround for behavior >> + * seen when WMI accessed via scripting on other OS > > /* > * Multi-line comments > * should have this kind of > * style. Pay attention to the details. > */ Ack (and I'm assuming here the concern is the 'D' and missing '.', let me know if there's anything else that is important) > >> + */ > > ... > >> + *string = kstrdup(obj->string.pointer, GFP_KERNEL); >> + kfree(obj); >> + return *string ? 0 : -ENOMEM; > > This breaks the principle "don't touch the output in error case". But I'm not changing *string in an error case here - I'm not understanding the issue here. Happy to rewrite it to make it clearer though if that would help. > > ... > >> + if (setting->valid) >> + return sysfs_emit(buf, "1\n"); >> + else >> + return sysfs_emit(buf, "0\n"); > > sysfs_emit(buf, "%d\n", !!setting->valid); > ? Yeah - this was picked up on in an earlier review. Apologies. > > ... > >> +static ssize_t current_password_store(struct kobject *kobj, >> + struct kobj_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >> + int length; > >> + length = strlen(buf); >> + if (buf[length-1] == '\n') >> + length--; > > This will prevent you from using \n in the password. Why? The BIOS doesn't like it - so we strip it out :) > >> + if (length >= TLMI_PWD_MAXLEN) >> + return -EINVAL; > > I guess password *string* length is wrong per se. count seems the > proper one which one should use. Yes, this is being cleaned up after review from Hans. > >> + >> + memcpy(setting->password, buf, length); > >> + setting->password[length] = '\0'; > > Why is the password a *string*? From where that assumption comes from? Sorry, I'm not understanding the question here. It's what our BIOS is expecting. I'm missing something here > >> + return count; >> +} >> + >> +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password); > > ... > >> + p = strchr(new_pwd, '\n'); >> + if (p) >> + *p = '\0'; > > strtrim(). But see above. ack. > >> + if (strlen(new_pwd) > setting->maxlen) { >> + ret = -EINVAL; >> + goto out; >> + } > > ... > >> + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ >> + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) >> + + strlen(setting->kbdlang) + 3 /* type */ >> + + strlen(new_pwd) + 6 /* punctuation and terminator*/; >> + auth_str = kzalloc(len, GFP_KERNEL); >> + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", >> + setting->pwd_type, setting->password, new_pwd, >> + encoding_options[setting->encoding], setting->kbdlang); > > NIH of kasprintf() Not sure what NIH is - but I'm assuming I should be using kasprintf instead of snprinf :) I wasn't aware of it - thank you. > > ... > >> + return ret ? ret : count; > > return ret ?: count; > > is shorter. Ack > > ... > >> + if (strcmp(kobj->name, "Admin") == 0) >> + return sprintf(buf, "bios-admin\n"); >> + else if (strcmp(kobj->name, "System") == 0) > > Redundant 'else' Ack > >> + return sprintf(buf, "power-on\n"); >> + return -EIO; > > ... > >> +static struct attribute *auth_attrs[] = { >> + &auth_is_pass_set.attr, >> + &auth_min_pass_length.attr, >> + &auth_max_pass_length.attr, >> + &auth_current_password.attr, >> + &auth_new_password.attr, >> + &auth_role.attr, >> + &auth_mechanism.attr, >> + &auth_encoding.attr, >> + &auth_kbdlang.attr, >> + NULL, > > The terminator line doesn't need a comma. Argh. I always get this wrong as to when it is required and when it isn't. I'll fix > >> +}; > > ... > >> + new_setting = kstrdup(buf, GFP_KERNEL); > > strtrim(buf) ? ack > >> + if (!new_setting) >> + return -ENOMEM; > >> + p = strchr(new_setting, '\n'); >> + if (p) >> + *p = '\0'; > > See above. Thanks > >> + alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting); >> + >> + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) { >> + auth_len += strlen(tlmi_priv.pwd_admin->password) + 1 >> + + strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1 >> + + strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1; >> + auth_str = kmalloc(auth_len, GFP_KERNEL); >> + if (!auth_str) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + sprintf(auth_str, "%s,%s,%s;", >> + tlmi_priv.pwd_admin->password, >> + encoding_options[tlmi_priv.pwd_admin->encoding], >> + tlmi_priv.pwd_admin->kbdlang); >> + alloc_len += auth_len; > > HIN of kasprintf() ack > >> + } >> + >> + set_str = kmalloc(alloc_len, GFP_KERNEL); >> + if (!set_str) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting); > > Ditto, ack > >> + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) >> + sprintf(set_str + str_ix, ",%s", auth_str); >> + else >> + sprintf(set_str + str_ix, ";"); >> + >> + ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str); >> + if (ret) >> + goto out; >> + >> + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) >> + ret = tlmi_save_bios_settings(auth_str); >> + else >> + ret = tlmi_save_bios_settings(""); > >> + if (ret) >> + goto out; > > Useless. Ack (and oops) > >> +out: >> + kfree(auth_str); >> + kfree(set_str); >> + kfree(new_setting); >> + return ret ? ret : count; > > Can be shorter. Ack > >> +} > >> +static struct kobj_attribute attr_displ_name = >> + __ATTR_RO(display_name); > > One line (and so on for all of them). Ack > > ... > >> +static struct attribute *tlmi_attrs[] = { >> + &attr_displ_name.attr, >> + &attr_current_val.attr, >> + &attr_possible_values.attr, >> + NULL, > > No comma. Ack > >> +}; > > ... > >> + struct kobj_attribute *kattr; > >> + ssize_t ret = -EIO; > > Useless. Use direct return. Ack > >> + >> + kattr = container_of(attr, struct kobj_attribute, attr); >> + if (kattr->show) >> + ret = kattr->show(kobj, kattr, buf); >> + return ret; > > ... > >> + struct kobj_attribute *kattr; >> + ssize_t ret = -EIO; >> + >> + kattr = container_of(attr, struct kobj_attribute, attr); >> + if (kattr->store) >> + ret = kattr->store(kobj, kattr, buf, count); >> + return ret; > > As above. Ack > > ... > >> + for (i = 0; i < TLMI_SETTINGS_COUNT; i++) { >> + /*Check if index is a valid setting - skip if it isn't */ > > Missed space. Ack > >> + if (!tlmi_priv.setting[i]) >> + continue; >> + >> + /* build attribute */ >> + tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset; >> + ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &attr_name_ktype, >> + NULL, "%s", tlmi_priv.setting[i]->display_name); >> + if (ret) >> + goto fail_create_attr; >> + >> + ret = sysfs_create_group(&tlmi_priv.setting[i]->kobj, &tlmi_attr_group); >> + if (ret) >> + goto fail_create_attr; >> + } > > ... > >> + int i = 0; > > Why assignment? Left over from earlier iteration. Should be removed. > >> + for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) { >> + char *item = NULL; >> + int spleng = 0; >> + int num = 0; >> + char *p; >> + struct tlmi_attr_setting *setting; >> + >> + tlmi_priv.setting[i] = NULL; >> + status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); >> + if (ACPI_FAILURE(status)) >> + break; >> + if (!item) >> + break; >> + if (!*item) >> + continue; >> + >> + /* It is not allowed to have '/' for file name. Convert it into '\'. */ >> + spleng = strlen(item); >> + for (num = 0; num < spleng; num++) { >> + if (item[num] == '/') >> + item[num] = '\\'; >> + } > > NIH of strreplace(), but please check its name. Will do. Thanks! > >> + >> + /* Remove the value part */ >> + p = strchr(item, ','); >> + if (p) >> + *p = '\0'; >> + >> + /* Create a setting entry */ >> + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); >> + if (!setting) { >> + ret = -ENOMEM; >> + goto fail_clear_attr; >> + } >> + setting->index = i; >> + strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN); >> + /* If BIOS selections supported, load those */ >> + if (tlmi_priv.can_get_bios_selections) { >> + ret = tlmi_get_bios_selections(setting->display_name, >> + &setting->possible_values); >> + if (ret || !setting->possible_values) >> + pr_info("Error retrieving possible values for %d : %s\n", >> + i, setting->display_name); >> + } >> + tlmi_priv.setting[i] = setting; >> + tlmi_priv.settings_count++; >> + kfree(item); >> + } > > I stopped here because it's full of such small issues. Ouch. Many thanks for the review. I'll go through the rest and see if I can clean it up more to make it less painful. > > Basically the summary > - try to find the existing APIs for routines that quite often in use > (usually string operations) > - try to make your code compact with the same level of readability > > I really had tried to do that - many thanks for your patience and review details. Mark
On Tue, May 25, 2021 at 6:14 PM Mark Pearson <markpearson@lenovo.com> wrote: > On 2021-05-22 7:04 a.m., Andy Shevchenko wrote: > > On Sun, May 9, 2021 at 5:02 AM Mark Pearson <markpearson@lenovo.com> wrote: ... > >> + *string = kstrdup(obj->string.pointer, GFP_KERNEL); > >> + kfree(obj); > >> + return *string ? 0 : -ENOMEM; > > > > This breaks the principle "don't touch the output in error case". > > But I'm not changing *string in an error case here - I'm not > understanding the issue here. > Happy to rewrite it to make it clearer though if that would help. *string may be not NULL when you do assign it. You need to assign it iff you are about to return 0. ... > >> + length = strlen(buf); > >> + if (buf[length-1] == '\n') > >> + length--; > > > > This will prevent you from using \n in the password. Why? > The BIOS doesn't like it - so we strip it out :) I haven't checked, but if there is no description of this in the documentation/commit message, should be added. ... > >> + memcpy(setting->password, buf, length); > > > >> + setting->password[length] = '\0'; > > > > Why is the password a *string*? From where that assumption comes from? > Sorry, I'm not understanding the question here. It's what our BIOS is > expecting. I'm missing something here So, BIOS restrictions should be documented if not yet. ... > >> + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ > >> + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) > >> + + strlen(setting->kbdlang) + 3 /* type */ > >> + + strlen(new_pwd) + 6 /* punctuation and terminator*/; > >> + auth_str = kzalloc(len, GFP_KERNEL); > >> + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", > >> + setting->pwd_type, setting->password, new_pwd, > >> + encoding_options[setting->encoding], setting->kbdlang); > > > > NIH of kasprintf() > Not sure what NIH is - https://en.wikipedia.org/wiki/Not_invented_here > but I'm assuming I should be using kasprintf > instead of snprinf :) > I wasn't aware of it - thank you. strlen+kmalloc+sprintf == kasprintf ... > > The terminator line doesn't need a comma. > Argh. I always get this wrong as to when it is required and when it isn't. > I'll fix If it is supposed to be the last entry (i.o.w. terminator) --> no comma.
Hi, On 5/25/21 5:14 PM, Mark Pearson wrote: > Hi Andy, > >>> +static ssize_t current_password_store(struct kobject *kobj, >>> + struct kobj_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >>> + int length; >> >>> + length = strlen(buf); >>> + if (buf[length-1] == '\n') >>> + length--; >> >> This will prevent you from using \n in the password. Why? > The BIOS doesn't like it - so we strip it out :) Erm, I don't believe that that is the whole story, there are 2 separate things at play here: 1. When entering the BIOS password at system power-on pressing enter means you're done and the BIOS should check what you've just entered as password before pressing the enter key, so the password can never contain '\n' since the enter key is the terminator for entering the password at boot 2. People often use sysfs files by doing things like this: echo mysecretpassword > /sys/.../current_password And the "echo" shell command will then add an extra '\n' this is why you will see code like this to strip the '\n' in functions which use the input string as is (instead of doing strtol, sysfs_match_string or something else which does not care about a terminating '\n' already, note that functions like sysfs_str_equals and sysfs_match_string are special helpers for not caring about the '\n' without needing to strip it (because stripping it requires a strdup). So what is happening here is simply stripping the '\n' which may have been added by echo (if it was added). Regards, Hans
Thanks Andy On 2021-05-25 12:18 p.m., Andy Shevchenko wrote: > On Tue, May 25, 2021 at 6:14 PM Mark Pearson <markpearson@lenovo.com> wrote: >> On 2021-05-22 7:04 a.m., Andy Shevchenko wrote: >>> On Sun, May 9, 2021 at 5:02 AM Mark Pearson <markpearson@lenovo.com> wrote: > > ... > >>>> + *string = kstrdup(obj->string.pointer, GFP_KERNEL); >>>> + kfree(obj); >>>> + return *string ? 0 : -ENOMEM; >>> >>> This breaks the principle "don't touch the output in error case". >> >> But I'm not changing *string in an error case here - I'm not >> understanding the issue here. >> Happy to rewrite it to make it clearer though if that would help. > > *string may be not NULL when you do assign it. > You need to assign it iff you are about to return 0. Ah - got it. I'll fix. > > ... > >>>> + length = strlen(buf); >>>> + if (buf[length-1] == '\n') >>>> + length--; >>> >>> This will prevent you from using \n in the password. Why? >> The BIOS doesn't like it - so we strip it out :) > > I haven't checked, but if there is no description of this in the > documentation/commit message, should be added. Ack. > > ... > >>>> + memcpy(setting->password, buf, length); >>> >>>> + setting->password[length] = '\0'; >>> >>> Why is the password a *string*? From where that assumption comes from? >> Sorry, I'm not understanding the question here. It's what our BIOS is >> expecting. I'm missing something here > > So, BIOS restrictions should be documented if not yet. Ack > > ... > >>>> + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ >>>> + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) >>>> + + strlen(setting->kbdlang) + 3 /* type */ >>>> + + strlen(new_pwd) + 6 /* punctuation and terminator*/; >>>> + auth_str = kzalloc(len, GFP_KERNEL); >>>> + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", >>>> + setting->pwd_type, setting->password, new_pwd, >>>> + encoding_options[setting->encoding], setting->kbdlang); >>> >>> NIH of kasprintf() >> Not sure what NIH is - > > https://en.wikipedia.org/wiki/Not_invented_here Neat :) > >> but I'm assuming I should be using kasprintf >> instead of snprinf :) >> I wasn't aware of it - thank you. > > strlen+kmalloc+sprintf == kasprintf > > ... > >>> The terminator line doesn't need a comma. >> Argh. I always get this wrong as to when it is required and when it isn't. >> I'll fix > > If it is supposed to be the last entry (i.o.w. terminator) --> no comma. > Thanks
On 2021-05-25 12:29 p.m., Hans de Goede wrote: > Hi, > > On 5/25/21 5:14 PM, Mark Pearson wrote: >> Hi Andy, >> >>>> +static ssize_t current_password_store(struct kobject *kobj, >>>> + struct kobj_attribute *attr, >>>> + const char *buf, size_t count) >>>> +{ >>>> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); >>>> + int length; >>> >>>> + length = strlen(buf); >>>> + if (buf[length-1] == '\n') >>>> + length--; >>> >>> This will prevent you from using \n in the password. Why? >> The BIOS doesn't like it - so we strip it out :) > > Erm, I don't believe that that is the whole story, there are 2 > separate things at play here: > > 1. When entering the BIOS password at system power-on pressing > enter means you're done and the BIOS should check what you've > just entered as password before pressing the enter key, so the > password can never contain '\n' since the enter key is the > terminator for entering the password at boot > > 2. People often use sysfs files by doing things like this: > > echo mysecretpassword > /sys/.../current_password > > And the "echo" shell command will then add an extra '\n' this > is why you will see code like this to strip the '\n' in functions > which use the input string as is (instead of doing strtol, > sysfs_match_string or something else which does not care about a > terminating '\n' already, note that functions like sysfs_str_equals > and sysfs_match_string are special helpers for not caring about > the '\n' without needing to strip it (because stripping it > requires a strdup). > > So what is happening here is simply stripping the '\n' which may > have been added by echo (if it was added). > > Regards, > > Hans > Agreed on all, I guess I was giving the story summary :) I've been using this method in testing and if the \n gets passed on then you'll get an error returned - the BIOS doesn't like it. I've no idea if this is Lenovo specific or not. I'll document it as a Lenovo specific case unless there are objections. Mark
diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes index 8ea59fea4..31d1becfa 100644 --- a/Documentation/ABI/testing/sysfs-class-firmware-attributes +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes @@ -185,6 +185,17 @@ Description: A write only value that when used in tandem with current_password will reset a system or admin password. + encoding: + For platforms that require it (currently Lenovo) the + encoding method that is used. This can be either "ascii" + or "scancode". Default is set to "ascii" + + kbdlang: + For platforms that require it (currently Lenovo only) the + keyboard language method that is used. This is generally a + two char code (e.g. "us", "fr", "gr") and may vary per platform. + Default is set to "us" + Note, password management is session specific. If Admin password is set, same password must be written into current_password file (required for password-validation) and must be cleared once the session is over. @@ -197,7 +208,7 @@ Description: Drivers may emit a CHANGE uevent when a password is set or unset userspace may check it again. - On Dell systems, if Admin password is set, then all BIOS attributes + On Dell and Lenovo systems, if Admin password is set, then all BIOS attributes require password validation. What: /sys/class/firmware-attributes/*/attributes/pending_reboot diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index b0e1e5f65..0511c54fc 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -639,6 +639,18 @@ config THINKPAD_ACPI_HOTKEY_POLL If you are not sure, say Y here. The driver enables polling only if it is strictly necessary to do so. +config THINKPAD_LMI + tristate "Lenovo WMI-based systems management driver" + default m + depends on ACPI_WMI + select FW_ATTR_CLASS + help + This driver allows changing BIOS settings on Lenovo machines whose + BIOS support the WMI interface. + + To compile this driver as a module, choose M here: the module will + be called think-lmi. + config INTEL_ATOMISP2_LED tristate "Intel AtomISP2 camera LED driver" depends on GPIOLIB && LEDS_GPIO diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 147573f69..cab19672e 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -64,6 +64,7 @@ obj-$(CONFIG_IBM_RTL) += ibm_rtl.o obj-$(CONFIG_IDEAPAD_LAPTOP) += ideapad-laptop.o obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o obj-$(CONFIG_THINKPAD_ACPI) += thinkpad_acpi.o +obj-$(CONFIG_THINKPAD_LMI) += think-lmi.o # Intel obj-$(CONFIG_INTEL_ATOMISP2_LED) += intel_atomisp2_led.o diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c new file mode 100644 index 000000000..2fa989e98 --- /dev/null +++ b/drivers/platform/x86/think-lmi.c @@ -0,0 +1,890 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * think-lmi.c - Think LMI BIOS configuration driver + * + * Copyright(C) 2019-2021 Lenovo + * + * Original code from Thinkpad-wmi project https://github.com/iksaif/thinkpad-wmi + * Copyright(C) 2017 Corentin Chary <corentin.chary@gmail.com> + * Distributed under the GPL-2.0 license + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/acpi.h> +#include <linux/fs.h> +#include <linux/wmi.h> +#include "firmware_attributes_class.h" +#include "think-lmi.h" + +/* + * Name: + * Lenovo_BiosSetting + * Description: + * Get item name and settings for current LMI instance. + * Type: + * Query + * Returns: + * "Item,Value" + * Example: + * "WakeOnLAN,Enable" + */ +#define LENOVO_BIOS_SETTING_GUID "51F5230E-9677-46CD-A1CF-C0B23EE34DB7" + +/* + * Name: + * Lenovo_SetBiosSetting + * Description: + * Change the BIOS setting to the desired value using the Lenovo_SetBiosSetting + * class. To save the settings, use the Lenovo_SaveBiosSetting class. + * BIOS settings and values are case sensitive. + * After making changes to the BIOS settings, you must reboot the computer + * before the changes will take effect. + * Type: + * Method + * Arguments: + * "Item,Value,Password,Encoding,KbdLang;" + * Example: + * "WakeOnLAN,Disable,pswd,ascii,us;" + */ +#define LENOVO_SET_BIOS_SETTINGS_GUID "98479A64-33F5-4E33-A707-8E251EBBC3A1" + +/* + * Name: + * Lenovo_SaveBiosSettings + * Description: + * Save any pending changes in settings. + * Type: + * Method + * Arguments: + * "Password,Encoding,KbdLang;" + * Example: + * "pswd,ascii,us;" + */ +#define LENOVO_SAVE_BIOS_SETTINGS_GUID "6A4B54EF-A5ED-4D33-9455-B0D9B48DF4B3" + +/* + * Name: + * Lenovo_BiosPasswordSettings + * Description: + * Return BIOS Password settings + * Type: + * Query + * Returns: + * PasswordMode, PasswordState, MinLength, MaxLength, + * SupportedEncoding, SupportedKeyboard + */ +#define LENOVO_BIOS_PASSWORD_SETTINGS_GUID "8ADB159E-1E32-455C-BC93-308A7ED98246" + +/* + * Name: + * Lenovo_SetBiosPassword + * Description: + * Change a specific password. + * - BIOS settings cannot be changed at the same boot as power-on + * passwords (POP) and hard disk passwords (HDP). If you want to change + * BIOS settings and POP or HDP, you must reboot the system after changing + * one of them. + * - A password cannot be set using this method when one does not already + * exist. Passwords can only be updated or cleared. + * Type: + * Method + * Arguments: + * "PasswordType,CurrentPassword,NewPassword,Encoding,KbdLang;" + * Example: + * "pop,oldpop,newpop,ascii,us;” + */ +#define LENOVO_SET_BIOS_PASSWORD_GUID "2651D9FD-911C-4B69-B94E-D0DED5963BD7" + +/* + * Name: + * Lenovo_GetBiosSelections + * Description: + * Return a list of valid settings for a given item. + * Type: + * Method + * Arguments: + * "Item" + * Returns: + * "Value1,Value2,Value3,..." + * Example: + * -> "FlashOverLAN" + * <- "Enabled,Disabled" + */ +#define LENOVO_GET_BIOS_SELECTIONS_GUID "7364651A-132F-4FE7-ADAA-40C6C7EE2E3B" + +#define TLMI_POP_PWD (1 << 0) +#define TLMI_PAP_PWD (1 << 1) + +static const char * const encoding_options[] = { + [TLMI_ENCODING_ASCII] = "ascii", + [TLMI_ENCODING_SCANCODE] = "scancode", +}; +static struct think_lmi tlmi_priv; +struct class *fw_attr_class; + +/* ------ Utility functions ------------*/ +/* Convert BIOS WMI error string to suitable error code */ +static int tlmi_errstr_to_err(const char *errstr) +{ + if (!strcmp(errstr, "Success")) + /* Operation completed successfully */ + return 0; + if (!strcmp(errstr, "Not Supported")) + /* The feature is not supported on this system */ + return -EOPNOTSUPP; + if (!strcmp(errstr, "Invalid")) + /* The item or value provided is not valid parameter */ + return -EINVAL; + if (!strcmp(errstr, "Access Denied")) + /* + * The change could not be made due to an authentication problem. + * If a supervisor password exists, the correct supervisor password + * must be provided. + */ + return -EPERM; + if (!strcmp(errstr, "System Busy")) + /* + * BIOS changes have already been made that need to be committed. + 238G* Reboot the system and try again. + */ + return -EBUSY; + + pr_debug("Unknown error string: '%s'", errstr); + return -EINVAL; +} + +/* Extract error string from WMI return buffer */ +static int tlmi_extract_error(const struct acpi_buffer *output) +{ + const union acpi_object *obj; + int ret; + + obj = output->pointer; + if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { + kfree(obj); + return -EIO; + } + + ret = tlmi_errstr_to_err(obj->string.pointer); + kfree(obj); + return ret; +} + +/* Utility function to execute WMI call to BIOS */ +static int tlmi_simple_call(const char *guid, const char *arg) +{ + const struct acpi_buffer input = { strlen(arg), (char *)arg }; + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + acpi_status status; + int i, err; + + /* + * duplicated call required to match bios workaround for behavior + * seen when WMI accessed via scripting on other OS + */ + for (i = 0; i < 2; i++) { + /* (re)initialize output buffer to default state */ + output.length = ACPI_ALLOCATE_BUFFER; + output.pointer = NULL; + + status = wmi_evaluate_method(guid, 0, 0, &input, &output); + if (ACPI_FAILURE(status)) { + kfree(output.pointer); + return -EIO; + } + err = tlmi_extract_error(&output); + if (err) + return err; + } + return 0; +} + +/* Extract output string from WMI return buffer */ +static int tlmi_extract_output_string(const struct acpi_buffer *output, + char **string) +{ + const union acpi_object *obj; + + obj = output->pointer; + if (!obj || obj->type != ACPI_TYPE_STRING || !obj->string.pointer) { + kfree(obj); + return -EIO; + } + + *string = kstrdup(obj->string.pointer, GFP_KERNEL); + kfree(obj); + return *string ? 0 : -ENOMEM; +} + +/* ------ Core interface functions ------------*/ + +/* Get password settings from BIOS */ +static int tlmi_get_pwd_settings(struct tlmi_pwdcfg *pwdcfg) +{ + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + const union acpi_object *obj; + acpi_status status; + + if (!tlmi_priv.can_get_password_settings) + return -EOPNOTSUPP; + + status = wmi_query_block(LENOVO_BIOS_PASSWORD_SETTINGS_GUID, 0, + &output); + if (ACPI_FAILURE(status)) + return -EIO; + + obj = output.pointer; + if (!obj || obj->type != ACPI_TYPE_BUFFER || !obj->buffer.pointer) { + kfree(obj); + return -EIO; + } + /* + * The size of thinkpad_wmi_pcfg on ThinkStation is larger than ThinkPad. + * To make the driver compatible on different brands, we permit it to get + * the data in below case. + */ + if (obj->buffer.length < sizeof(struct tlmi_pwdcfg)) { + pr_warn("Unknown pwdcfg buffer length %d\n", obj->buffer.length); + kfree(obj); + return -EIO; + } + memcpy(pwdcfg, obj->buffer.pointer, sizeof(struct tlmi_pwdcfg)); + kfree(obj); + return 0; +} + +static int tlmi_save_bios_settings(const char *password) +{ + return tlmi_simple_call(LENOVO_SAVE_BIOS_SETTINGS_GUID, + password); +} + +static int tlmi_setting(int item, char **value, const char *guid_string) +{ + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + acpi_status status; + + status = wmi_query_block(guid_string, item, &output); + if (ACPI_FAILURE(status)) { + kfree(output.pointer); + return -EIO; + } + + return tlmi_extract_output_string(&output, value); +} + +static int tlmi_get_bios_selections(const char *item, char **value) +{ + const struct acpi_buffer input = { strlen(item), (char *)item }; + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; + acpi_status status; + + status = wmi_evaluate_method(LENOVO_GET_BIOS_SELECTIONS_GUID, + 0, 0, &input, &output); + + if (ACPI_FAILURE(status)) { + kfree(output.pointer); + return -EIO; + } + + return tlmi_extract_output_string(&output, value); +} + +/* ---- Authentication sysfs --------------------------------------------------------- */ +static ssize_t is_enabled_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + if (setting->valid) + return sysfs_emit(buf, "1\n"); + else + return sysfs_emit(buf, "0\n"); +} + +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); + +static ssize_t current_password_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + int length; + + length = strlen(buf); + if (buf[length-1] == '\n') + length--; + + if (length >= TLMI_PWD_MAXLEN) + return -EINVAL; + + memcpy(setting->password, buf, length); + setting->password[length] = '\0'; + return count; +} + +static struct kobj_attribute auth_current_password = __ATTR_WO(current_password); + +static ssize_t new_password_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + char *p, *new_pwd; + char *auth_str; + int ret = 0, len; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if (!tlmi_priv.can_set_bios_password) + return -EOPNOTSUPP; + + new_pwd = kstrdup(buf, GFP_KERNEL); + if (!new_pwd) + return -ENOMEM; + + p = strchr(new_pwd, '\n'); + if (p) + *p = '\0'; + + if (strlen(new_pwd) > setting->maxlen) { + ret = -EINVAL; + goto out; + } + + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */ + len = strlen(setting->password) + strlen(encoding_options[setting->encoding]) + + strlen(setting->kbdlang) + 3 /* type */ + + strlen(new_pwd) + 6 /* punctuation and terminator*/; + auth_str = kzalloc(len, GFP_KERNEL); + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", + setting->pwd_type, setting->password, new_pwd, + encoding_options[setting->encoding], setting->kbdlang); + ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str); + kfree(auth_str); +out: + kfree(new_pwd); + return ret ? ret : count; +} + +static struct kobj_attribute auth_new_password = __ATTR_WO(new_password); + +static ssize_t min_password_length_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + return sysfs_emit(buf, "%d\n", setting->minlen); +} + +static struct kobj_attribute auth_min_pass_length = __ATTR_RO(min_password_length); + +static ssize_t max_password_length_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + return sysfs_emit(buf, "%d\n", setting->maxlen); +} +static struct kobj_attribute auth_max_pass_length = __ATTR_RO(max_password_length); + +static ssize_t mechanism_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "password\n"); +} +static struct kobj_attribute auth_mechanism = __ATTR_RO(mechanism); + +static ssize_t encoding_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + return sysfs_emit(buf, "%s\n", encoding_options[setting->encoding]); +} + +static ssize_t encoding_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + int i; + + /* Scan for a matching profile */ + i = sysfs_match_string(encoding_options, buf); + if (i < 0) + return -EINVAL; + + setting->encoding = i; + return count; +} + +static struct kobj_attribute auth_encoding = __ATTR_RW(encoding); + +static ssize_t kbdlang_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + + return sysfs_emit(buf, "%s\n", setting->kbdlang); +} + +static ssize_t kbdlang_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); + int length; + + length = strlen(buf); + if (buf[length-1] == '\n') + length--; + if (!length || (length >= TLMI_LANG_MAXLEN)) + return -EINVAL; + + memcpy(setting->kbdlang, buf, length); + setting->kbdlang[length] = '\0'; + return count; +} + +static struct kobj_attribute auth_kbdlang = __ATTR_RW(kbdlang); + +static ssize_t role_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + if (strcmp(kobj->name, "Admin") == 0) + return sprintf(buf, "bios-admin\n"); + else if (strcmp(kobj->name, "System") == 0) + return sprintf(buf, "power-on\n"); + return -EIO; +} +static struct kobj_attribute auth_role = __ATTR_RO(role); + +static struct attribute *auth_attrs[] = { + &auth_is_pass_set.attr, + &auth_min_pass_length.attr, + &auth_max_pass_length.attr, + &auth_current_password.attr, + &auth_new_password.attr, + &auth_role.attr, + &auth_mechanism.attr, + &auth_encoding.attr, + &auth_kbdlang.attr, + NULL, +}; + +static const struct attribute_group auth_attr_group = { + .attrs = auth_attrs, +}; + +/* ---- Attributes sysfs --------------------------------------------------------- */ +static ssize_t display_name_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + + return sysfs_emit(buf, "%s\n", setting->display_name); +} + +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + char *item; + int ret; + + ret = tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID); + if (ret) + return ret; + + ret = sysfs_emit(buf, "%s\n", item); + kfree(item); + return ret; +} + +static ssize_t possible_values_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + + if (!tlmi_priv.can_get_bios_selections) + return -EOPNOTSUPP; + + return sysfs_emit(buf, "%s\n", setting->possible_values); +} + +static ssize_t current_value_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj); + int alloc_len, auth_len = 0; + int str_ix = 0; + char *auth_str = NULL; + char *set_str, *new_setting, *p; + int ret; + + if (!tlmi_priv.can_set_bios_settings) + return -EOPNOTSUPP; + + new_setting = kstrdup(buf, GFP_KERNEL); + if (!new_setting) + return -ENOMEM; + p = strchr(new_setting, '\n'); + if (p) + *p = '\0'; + + alloc_len = strlen(setting->display_name) + 3 + strlen(new_setting); + + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) { + auth_len += strlen(tlmi_priv.pwd_admin->password) + 1 + + strlen(encoding_options[tlmi_priv.pwd_admin->encoding]) + 1 + + strlen(tlmi_priv.pwd_admin->kbdlang) + 1 + 1; + auth_str = kmalloc(auth_len, GFP_KERNEL); + if (!auth_str) { + ret = -ENOMEM; + goto out; + } + + sprintf(auth_str, "%s,%s,%s;", + tlmi_priv.pwd_admin->password, + encoding_options[tlmi_priv.pwd_admin->encoding], + tlmi_priv.pwd_admin->kbdlang); + alloc_len += auth_len; + } + + set_str = kmalloc(alloc_len, GFP_KERNEL); + if (!set_str) { + ret = -ENOMEM; + goto out; + } + + str_ix += sprintf(set_str, "%s,%s", setting->display_name, new_setting); + + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) + sprintf(set_str + str_ix, ",%s", auth_str); + else + sprintf(set_str + str_ix, ";"); + + ret = tlmi_simple_call(LENOVO_SET_BIOS_SETTINGS_GUID, set_str); + if (ret) + goto out; + + if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) + ret = tlmi_save_bios_settings(auth_str); + else + ret = tlmi_save_bios_settings(""); + if (ret) + goto out; + +out: + kfree(auth_str); + kfree(set_str); + kfree(new_setting); + return ret ? ret : count; +} + +static struct kobj_attribute attr_displ_name = + __ATTR_RO(display_name); + +static struct kobj_attribute attr_possible_values = + __ATTR_RO(possible_values); + +static struct kobj_attribute attr_current_val = + __ATTR_RW_MODE(current_value, 0600); + +static struct attribute *tlmi_attrs[] = { + &attr_displ_name.attr, + &attr_current_val.attr, + &attr_possible_values.attr, + NULL, +}; + +static const struct attribute_group tlmi_attr_group = { + .attrs = tlmi_attrs, +}; + +static ssize_t tlmi_attr_show(struct kobject *kobj, struct attribute *attr, + char *buf) +{ + struct kobj_attribute *kattr; + ssize_t ret = -EIO; + + kattr = container_of(attr, struct kobj_attribute, attr); + if (kattr->show) + ret = kattr->show(kobj, kattr, buf); + return ret; +} + +static ssize_t tlmi_attr_store(struct kobject *kobj, struct attribute *attr, + const char *buf, size_t count) +{ + struct kobj_attribute *kattr; + ssize_t ret = -EIO; + + kattr = container_of(attr, struct kobj_attribute, attr); + if (kattr->store) + ret = kattr->store(kobj, kattr, buf, count); + return ret; +} + +static const struct sysfs_ops tlmi_kobj_sysfs_ops = { + .show = tlmi_attr_show, + .store = tlmi_attr_store, +}; + +static struct kobj_type attr_name_ktype = { + .sysfs_ops = &tlmi_kobj_sysfs_ops, +}; + +/* ---- Initialisation --------------------------------------------------------- */ +static void tlmi_release_attr(void) +{ + int i; + + /* Attribute structures */ + for (i = 0; i < TLMI_SETTINGS_COUNT; i++) { + + if (tlmi_priv.setting[i]) { + kfree(tlmi_priv.setting[i]->possible_values); + sysfs_remove_group(&tlmi_priv.setting[i]->kobj, &tlmi_attr_group); + kobject_put(&tlmi_priv.setting[i]->kobj); + } + } + kset_unregister(tlmi_priv.attribute_kset); + + /* Authentication structures */ + sysfs_remove_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group); + kobject_put(&tlmi_priv.pwd_admin->kobj); + sysfs_remove_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group); + kobject_put(&tlmi_priv.pwd_power->kobj); + kset_unregister(tlmi_priv.authentication_kset); +} + +static int tlmi_sysfs_init(void) +{ + int i, ret; + + ret = fw_attributes_class_register(&fw_attr_class); + if (ret) + return ret; + + tlmi_priv.class_dev = device_create(fw_attr_class, NULL, MKDEV(0, 0), + NULL, "%s", "thinklmi"); + if (IS_ERR(tlmi_priv.class_dev)) { + ret = PTR_ERR(tlmi_priv.class_dev); + goto fail_class_created; + } + + tlmi_priv.attribute_kset = kset_create_and_add("attributes", NULL, + &tlmi_priv.class_dev->kobj); + if (!tlmi_priv.attribute_kset) { + ret = -ENOMEM; + goto fail_device_created; + } + + for (i = 0; i < TLMI_SETTINGS_COUNT; i++) { + /*Check if index is a valid setting - skip if it isn't */ + if (!tlmi_priv.setting[i]) + continue; + + /* build attribute */ + tlmi_priv.setting[i]->kobj.kset = tlmi_priv.attribute_kset; + ret = kobject_init_and_add(&tlmi_priv.setting[i]->kobj, &attr_name_ktype, + NULL, "%s", tlmi_priv.setting[i]->display_name); + if (ret) + goto fail_create_attr; + + ret = sysfs_create_group(&tlmi_priv.setting[i]->kobj, &tlmi_attr_group); + if (ret) + goto fail_create_attr; + } + + /* Create authentication entries */ + tlmi_priv.authentication_kset = kset_create_and_add("authentication", NULL, + &tlmi_priv.class_dev->kobj); + if (!tlmi_priv.authentication_kset) { + ret = -ENOMEM; + goto fail_create_attr; + } + tlmi_priv.pwd_admin->kobj.kset = tlmi_priv.authentication_kset; + ret = kobject_init_and_add(&tlmi_priv.pwd_admin->kobj, &attr_name_ktype, + NULL, "%s", "Admin"); + if (ret) + goto fail_create_attr; + + ret = sysfs_create_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group); + if (ret) + goto fail_create_attr; + + tlmi_priv.pwd_power->kobj.kset = tlmi_priv.authentication_kset; + ret = kobject_init_and_add(&tlmi_priv.pwd_power->kobj, &attr_name_ktype, + NULL, "%s", "System"); + if (ret) + goto fail_create_attr; + + ret = sysfs_create_group(&tlmi_priv.pwd_power->kobj, &auth_attr_group); + if (ret) + goto fail_create_attr; + + return ret; + +fail_create_attr: + tlmi_release_attr(); +fail_device_created: + device_destroy(fw_attr_class, MKDEV(0, 0)); +fail_class_created: + fw_attributes_class_remove(); + return ret; +} + +/* ---- Base Driver -------------------------------------------------------- */ +static int tlmi_analyze(void) +{ + struct tlmi_pwdcfg pwdcfg; + acpi_status status; + int i = 0; + int ret; + + if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) && + wmi_has_guid(LENOVO_SAVE_BIOS_SETTINGS_GUID)) + tlmi_priv.can_set_bios_settings = true; + + if (wmi_has_guid(LENOVO_GET_BIOS_SELECTIONS_GUID)) + tlmi_priv.can_get_bios_selections = true; + + if (wmi_has_guid(LENOVO_SET_BIOS_PASSWORD_GUID)) + tlmi_priv.can_set_bios_password = true; + + if (wmi_has_guid(LENOVO_BIOS_PASSWORD_SETTINGS_GUID)) + tlmi_priv.can_get_password_settings = true; + + /* + * Try to find the number of valid settings of this machine + * and use it to create sysfs attributes + */ + for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) { + char *item = NULL; + int spleng = 0; + int num = 0; + char *p; + struct tlmi_attr_setting *setting; + + tlmi_priv.setting[i] = NULL; + status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); + if (ACPI_FAILURE(status)) + break; + if (!item) + break; + if (!*item) + continue; + + /* It is not allowed to have '/' for file name. Convert it into '\'. */ + spleng = strlen(item); + for (num = 0; num < spleng; num++) { + if (item[num] == '/') + item[num] = '\\'; + } + + /* Remove the value part */ + p = strchr(item, ','); + if (p) + *p = '\0'; + + /* Create a setting entry */ + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); + if (!setting) { + ret = -ENOMEM; + goto fail_clear_attr; + } + setting->index = i; + strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN); + /* If BIOS selections supported, load those */ + if (tlmi_priv.can_get_bios_selections) { + ret = tlmi_get_bios_selections(setting->display_name, + &setting->possible_values); + if (ret || !setting->possible_values) + pr_info("Error retrieving possible values for %d : %s\n", + i, setting->display_name); + } + tlmi_priv.setting[i] = setting; + tlmi_priv.settings_count++; + kfree(item); + } + + /* Create password setting structure */ + ret = tlmi_get_pwd_settings(&pwdcfg); + if (ret) + goto fail_clear_attr; + + tlmi_priv.pwd_admin = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL); + if (!tlmi_priv.pwd_admin) { + ret = -ENOMEM; + goto fail_clear_attr; + } + sprintf(tlmi_priv.pwd_admin->display_name, "admin"); + sprintf(tlmi_priv.pwd_admin->kbdlang, "us"); + tlmi_priv.pwd_admin->encoding = TLMI_ENCODING_ASCII; + sprintf(tlmi_priv.pwd_admin->pwd_type, "pap"); + tlmi_priv.pwd_admin->minlen = pwdcfg.min_length; + tlmi_priv.pwd_admin->maxlen = pwdcfg.max_length; + if (pwdcfg.password_state & TLMI_PAP_PWD) + tlmi_priv.pwd_admin->valid = true; + + tlmi_priv.pwd_power = kzalloc(sizeof(struct tlmi_pwd_setting), GFP_KERNEL); + if (!tlmi_priv.pwd_power) { + ret = -ENOMEM; + goto fail_clear_attr; + } + sprintf(tlmi_priv.pwd_power->display_name, "power-on"); + sprintf(tlmi_priv.pwd_power->kbdlang, "us"); + tlmi_priv.pwd_power->encoding = TLMI_ENCODING_ASCII; + sprintf(tlmi_priv.pwd_admin->pwd_type, "pop"); + tlmi_priv.pwd_power->minlen = pwdcfg.min_length; + tlmi_priv.pwd_power->maxlen = pwdcfg.max_length; + + if (pwdcfg.password_state & TLMI_POP_PWD) + tlmi_priv.pwd_power->valid = true; + + return 0; + +fail_clear_attr: + for (i = 0; i < TLMI_SETTINGS_COUNT; ++i) + kfree(tlmi_priv.setting[i]); + return ret; +} + +static void tlmi_remove(struct wmi_device *wdev) +{ + tlmi_release_attr(); + device_destroy(fw_attr_class, MKDEV(0, 0)); + fw_attributes_class_remove(); +} + +static int tlmi_probe(struct wmi_device *wdev, const void *context) +{ + tlmi_analyze(); + return tlmi_sysfs_init(); +} + +static const struct wmi_device_id tlmi_id_table[] = { + { .guid_string = LENOVO_BIOS_SETTING_GUID }, + { } +}; + +static struct wmi_driver tlmi_driver = { + .driver = { + .name = "think-lmi", + }, + .id_table = tlmi_id_table, + .probe = tlmi_probe, + .remove = tlmi_remove, +}; + +MODULE_AUTHOR("Sugumaran L <slacshiminar@lenovo.com>"); +MODULE_AUTHOR("Mark Pearson <markpearson@lenovo.com>"); +MODULE_AUTHOR("Corentin Chary <corentin.chary@gmail.com>"); +MODULE_DESCRIPTION("ThinkLMI Driver"); +MODULE_LICENSE("GPL"); + +module_wmi_driver(tlmi_driver); diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h new file mode 100644 index 000000000..2c1484304 --- /dev/null +++ b/drivers/platform/x86/think-lmi.h @@ -0,0 +1,73 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef _THINK_LMI_H_ +#define _THINK_LMI_H_ + +#define TLMI_SETTINGS_COUNT 256 +#define TLMI_SETTINGS_MAXLEN 512 +#define TLMI_PWD_MAXLEN 128 +#define TLMI_PWDTYPE_MAXLEN 64 +#define TLMI_ENC_MAXLEN 64 +#define TLMI_LANG_MAXLEN 4 +#define TLMI_PWDTYPE_LEN 4 +/* + * Longest string should be in the set command: allow size of BIOS + * option and choice + */ +#define TLMI_GETSET_MAXLEN (TLMI_SETTINGS_MAXLEN + TLMI_SETTINGS_MAXLEN) + +enum encoding_option { + TLMI_ENCODING_ASCII, + TLMI_ENCODING_SCANCODE, +}; + +/* password configuration details */ +struct tlmi_pwdcfg { + uint32_t password_mode; + uint32_t password_state; + uint32_t min_length; + uint32_t max_length; + uint32_t supported_encodings; + uint32_t supported_keyboard; +}; + +/* password setting details */ +struct tlmi_pwd_setting { + struct kobject kobj; + bool valid; + char display_name[TLMI_PWDTYPE_MAXLEN]; + char password[TLMI_PWD_MAXLEN]; + char pwd_type[TLMI_PWDTYPE_LEN]; + int minlen; + int maxlen; + enum encoding_option encoding; + char kbdlang[TLMI_LANG_MAXLEN]; +}; + +/* Attribute setting details */ +struct tlmi_attr_setting { + struct kobject kobj; + int index; + char display_name[TLMI_SETTINGS_MAXLEN]; + char *possible_values; +}; + +struct think_lmi { + struct wmi_device *wmi_device; + + int settings_count; + bool can_set_bios_settings; + bool can_get_bios_selections; + bool can_set_bios_password; + bool can_get_password_settings; + + struct tlmi_attr_setting *setting[TLMI_SETTINGS_COUNT]; + struct device *class_dev; + struct kset *attribute_kset; + struct kset *authentication_kset; + struct tlmi_pwd_setting *pwd_admin; + struct tlmi_pwd_setting *pwd_power; +}; + +#endif /* !_THINK_LMI_H_ */ +
For Lenovo platforms that support a WMI interface to the BIOS add support, using the firmware-attributes class, to allow users to access and modify various BIOS related settings. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- Changes in V2: - Improved error handling - updated to use kobj's better (thanks Hans for pointers). Makes code simpler and cleaner - Removed some dead code from previous implementation that I'd missed - Moved think-lmi.h into platform/x86 directory - Use firmware_attributes_class helper - Corrected some bad indexing when storing an attribute - improved encoding options to limit selection - Added details on encoding and kbdlang to sysfs-class-firmware-attributes documentation .../testing/sysfs-class-firmware-attributes | 13 +- drivers/platform/x86/Kconfig | 12 + drivers/platform/x86/Makefile | 1 + drivers/platform/x86/think-lmi.c | 890 ++++++++++++++++++ drivers/platform/x86/think-lmi.h | 73 ++ 5 files changed, 988 insertions(+), 1 deletion(-) create mode 100644 drivers/platform/x86/think-lmi.c create mode 100644 drivers/platform/x86/think-lmi.h