Re: [External] Re: [PATCH v2 3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms

[Hans de Goede <hdegoede@redhat.com>]

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