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
