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
