diff mbox series

[v4,3/3] platform/x86: think-lmi: Add WMI interface support on Lenovo platforms

Message ID 20210526201447.3686-3-markpearson@lenovo.com (mailing list archive)
State Accepted, archived
Headers show
Series [v4,1/3] platform/x86: firmware_attributes_class: Create helper file for handling firmware-attributes class registration events | expand

Commit Message

Mark Pearson May 26, 2021, 8:14 p.m. UTC
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

Changes in v3:
 - Clean up is_enabled display
 - Use new firmware-attributes class API names
 - Correct release of kobj objects
 - Change pwd_type to const char
 - Add role field to pwd_settings
 - Improved checking when setting new password

Changes in v4:
 - Updated documentation with password update specifics
 - Implement tlmi_errs to make error string scanning cleaner
 - Clean up pointer check handling
 - Clean up pointer free handling
 - Clean up pwd string length handling as recommended
 - Use kasprintf where appropriate
 - use strreplace where appropriate
 - Cosmetic cleanups as recommended

 .../testing/sysfs-class-firmware-attributes   |  18 +-
 drivers/platform/x86/Kconfig                  |  11 +
 drivers/platform/x86/Makefile                 |   1 +
 drivers/platform/x86/think-lmi.c              | 884 ++++++++++++++++++
 drivers/platform/x86/think-lmi.h              |  80 ++
 5 files changed, 993 insertions(+), 1 deletion(-)
 create mode 100644 drivers/platform/x86/think-lmi.c
 create mode 100644 drivers/platform/x86/think-lmi.h

Comments

Hans de Goede May 27, 2021, 9:17 a.m. UTC | #1
Hi Mark, Andy,

So as mentioned in my reply to patch 1/3, overall this looks pretty good.
There are a few very small issues remaining, but they are so small that
I've decided to fix them up and merge this into my review-hans branch
with the issues fixed up.

I plan to let this sit in review-hans a bit longer then usual to
give you (Mark) a chance to check out the changes and ack them
and to give Andy the time to check if his review remarks were
addressed to his liking.

I've put remarks inline / below about the 2 things which
I've fixed up in this patch.

Andy, thank you for your review of this. Your suggestions have
improved this driver, esp. the use of kasprintf has made some
of the functions a lot better.

On 5/26/21 10:14 PM, Mark Pearson wrote:

<snip>

> +/* Extract error string from WMI return buffer */
> +static int tlmi_extract_error(const struct acpi_buffer *output)
> +{
> +	const union acpi_object *obj;
> +
> +	obj = output->pointer;
> +	if (!obj)
> +		return -ENOMEM;
> +	if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> +		kfree(obj);

This kfree() should not be here, all the callers of
tlmi_extract_error() unconditionally call:

	kfree(output->pointer);

after calling tlmi_extract_error() so if hit this path with
the kfree(obj) in place we get a double free.

I've dropped the kfree() in my review-hans branch, as well as the
now no longer necessary {}.

> +		return -EIO;
> +	}
> +	return tlmi_errstr_to_err(obj->string.pointer);
> +}
> +
> +/* 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);
> +		kfree(output.pointer);
> +		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;
> +	char *s;
> +
> +	obj = output->pointer;
> +	if (!obj)
> +		return -ENOMEM;
> +	if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
> +		kfree(obj);

Same as above, also fixed in the same way.

> +		return -EIO;
> +	}
> +
> +	s = kstrdup(obj->string.pointer, GFP_KERNEL);
> +	if (!s)
> +		return -ENOMEM;
> +	*string = s;
> +	return 0;
> +}
> +
> +/* ------ Core interface functions ------------*/

Regards,

Hans
Hans de Goede May 27, 2021, 9:19 a.m. UTC | #2
Hi again,

On 5/26/21 10:14 PM, 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>

Mark, checkpatch.pl is complaining about this patch not adding a
MAINTAINERS entry for the think-lmi driver, can you please do a
follow-up patch adding a MAINTAINERS entry for this?

Regards,

Hans
Mark Pearson May 27, 2021, 12:20 p.m. UTC | #3
Thanks Hans

On 2021-05-27 5:17 a.m., Hans de Goede wrote:
> Hi Mark, Andy,
> 
> So as mentioned in my reply to patch 1/3, overall this looks pretty good.
> There are a few very small issues remaining, but they are so small that
> I've decided to fix them up and merge this into my review-hans branch
> with the issues fixed up.
> 
> I plan to let this sit in review-hans a bit longer then usual to
> give you (Mark) a chance to check out the changes and ack them
> and to give Andy the time to check if his review remarks were
> addressed to his liking.

Sounds good to me.
> 
> I've put remarks inline / below about the 2 things which
> I've fixed up in this patch.
> 
> Andy, thank you for your review of this. Your suggestions have
> improved this driver, esp. the use of kasprintf has made some
> of the functions a lot better.
Seconded :)
Thank you to both of you for the reviews - I learnt a lot with this
particular patch set.
> 
> On 5/26/21 10:14 PM, Mark Pearson wrote:
> 
> <snip>
> 
>> +/* Extract error string from WMI return buffer */
>> +static int tlmi_extract_error(const struct acpi_buffer *output)
>> +{
>> +	const union acpi_object *obj;
>> +
>> +	obj = output->pointer;
>> +	if (!obj)
>> +		return -ENOMEM;
>> +	if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
>> +		kfree(obj);
> 
> This kfree() should not be here, all the callers of
> tlmi_extract_error() unconditionally call:
> 
> 	kfree(output->pointer);
> 
> after calling tlmi_extract_error() so if hit this path with
> the kfree(obj) in place we get a double free.
> 
> I've dropped the kfree() in my review-hans branch, as well as the
> now no longer necessary {}.
Agreed - I should have noticed that one. Thanks
> 
>> +		return -EIO;
>> +	}
>> +	return tlmi_errstr_to_err(obj->string.pointer);
>> +}
>> +
>> +/* 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);
>> +		kfree(output.pointer);
>> +		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;
>> +	char *s;
>> +
>> +	obj = output->pointer;
>> +	if (!obj)
>> +		return -ENOMEM;
>> +	if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
>> +		kfree(obj);
> 
> Same as above, also fixed in the same way.
Thanks
> 
>> +		return -EIO;
>> +	}
>> +
>> +	s = kstrdup(obj->string.pointer, GFP_KERNEL);
>> +	if (!s)
>> +		return -ENOMEM;
>> +	*string = s;
>> +	return 0;
>> +}
>> +
>> +/* ------ Core interface functions ------------*/
> 


Mark
Mark Pearson May 27, 2021, 12:21 p.m. UTC | #4
On 2021-05-27 5:19 a.m., Hans de Goede wrote:
> Hi again,
> 
> On 5/26/21 10:14 PM, 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>
> 
> Mark, checkpatch.pl is complaining about this patch not adding a
> MAINTAINERS entry for the think-lmi driver, can you please do a
> follow-up patch adding a MAINTAINERS entry for this?
> 

Will do.

Mark
Andy Shevchenko May 27, 2021, 2:16 p.m. UTC | #5
On Wed, May 26, 2021 at 11:15 PM 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.

Thanks for an update! My comments below.

...

> +/*
> + * think-lmi.c - Think LMI BIOS configuration driver

It's not the best idea to include the file name into the file. If the
file gets renamed (by whatever reason) often this either brings an
additional burden or simply forgotten (as from my experience). I
recommend to drop the file names from the source code.

> + * 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
> + */

...

> +#include <linux/acpi.h>

linux/errno.h ?

> +#include <linux/fs.h>

linux/string.h ?
linux/types.h ?

> +#include <linux/wmi.h>
> +#include "firmware_attributes_class.h"
> +#include "think-lmi.h"

...

> + * Type:
> + *  Method
> + * Arguments:
> + *  "Item,Value,Password,Encoding,KbdLang;"
> + * Example:
> + *  "WakeOnLAN,Disable,pswd,ascii,us;"

Is 'pswd' here an example of the password? Hacker's language can make
it more visible, e.g. 'pa55w0rd'.
Same for other examples.

> + */

...

> +       /*
> +        * Duplicated call required to match bios workaround for behavior

bios -> BIOS

> +        * seen when WMI accessed via scripting on other OS.
> +        */

...

> +       struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj);

Candidate to have something like

#define to_tlmi_pwd_setting(obj)  container_of(...)

since it has appeared a few times.

> +       return sysfs_emit(buf, "%d\n", setting->valid);
> +}

> +

Unneeded blank line. Same for other similar places.

> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled);

Hmm... We have already define_one_global_ro(). The problems with that
are the name and location. But perhaps you can copy that macro here
with the same name and at least we can see the common grounds to clean
up in the future. Another possibility is to comment that it can be
replaced with the above mentioned macro. Ideally would be to refactor
right now, but it's not anyhow crucial or required for this series, so
may be postponed.

...

> +       int pwdlen;

Strictly speaking it should be size_t.

> +       pwdlen = strlen(buf);
> +       if (buf[pwdlen-1] == '\n')
> +               pwdlen--;

But the question is what will happen with the string like
'pa55\nw0rd\n' (note all \n:s)?
See also below.

> +       /* pwdlen == 0 is allowed to clear the password */
> +       if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))

The ' != 0' part is redundant.

> +               return -EINVAL;

> +       memcpy(setting->password, buf, pwdlen);
> +       setting->password[pwdlen] = '\0';

I'm not sure why we can't use strscpy() like you did in the other function.

> +       return count;

...

> +       /* Strip out CR if one is present, setting password won't work if it is present */
> +       strreplace(new_pwd, '\n', '\0');

Basically it will stop on the first one. See the strchrnul() trick below.

> +       pwdlen = strlen(new_pwd);
> +       /* pwdlen == 0 is allowed to clear the password */
> +       if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {

No need for ' != 0'.

> +               ret = -EINVAL;
> +               goto out;
> +       }

> +}


...

> +       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;

Similar comments as per above.

...

> +       char *set_str = NULL, *new_setting = NULL;
> +       char *auth_str = NULL;

The rule of thumb is to avoid forward assignments on stack allocated
variables. It may decrease readability, hide real issues, and simply
be unneeded churn, like here. Please revisit all of them in entire
series.

...

> +       /* Strip out CR if one is present */
> +       strreplace(new_setting, '\n', '\0');

As per above.

...

> +               /* Remove the value part */
> +               strreplace(item, ',', '\0');

This is kinda non-standard pattern.

I would see rather something like

char *p;

p = strchrnul(item, ',');
*p = '\0';

Yes, it's longer, but better to understand what's going on here.

> +               /* Create a setting entry */
> +               setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL);

sizeof(*setting) ?

> +               if (!setting) {
> +                       ret = -ENOMEM;
> +                       goto fail_clear_attr;
> +               }
> +               setting->index = i;
> +               strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN);

...

> +       sprintf(tlmi_priv.pwd_admin->display_name, "admin");
> +       sprintf(tlmi_priv.pwd_admin->kbdlang, "us");

Not sure why you need printf() type of function here. strcpy() or
strscpy() should be enough.

...

> +       if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE))
> +               pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1;

Not sure if WARN_ON() is really what has to be called here. But I
haven't checked the context deeply.

...

> +       sprintf(tlmi_priv.pwd_power->display_name, "power-on");
> +       sprintf(tlmi_priv.pwd_power->kbdlang, "us");

As above.

...

> +#ifndef _THINK_LMI_H_
> +#define _THINK_LMI_H_

+ linux/types.h

(At leas bool is from there)


> +#endif /* !_THINK_LMI_H_ */
Andy Shevchenko May 27, 2021, 2:18 p.m. UTC | #6
On Thu, May 27, 2021 at 12:17 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Mark, Andy,
>
> So as mentioned in my reply to patch 1/3, overall this looks pretty good.
> There are a few very small issues remaining, but they are so small that
> I've decided to fix them up and merge this into my review-hans branch
> with the issues fixed up.
>
> I plan to let this sit in review-hans a bit longer then usual to
> give you (Mark) a chance to check out the changes and ack them
> and to give Andy the time to check if his review remarks were
> addressed to his liking.

I looked into it again and commented on some stuff, but nothing
serious noted, so I think the next version would be ready to go.

> I've put remarks inline / below about the 2 things which
> I've fixed up in this patch.
>
> Andy, thank you for your review of this. Your suggestions have
> improved this driver, esp. the use of kasprintf has made some
> of the functions a lot better.

You are welcome!
Hans de Goede May 27, 2021, 2:36 p.m. UTC | #7
Hi,

On 5/27/21 4:16 PM, Andy Shevchenko wrote:
> On Wed, May 26, 2021 at 11:15 PM 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.
> 
> Thanks for an update! My comments below.

Since you have more remarks then I anticipated I guess it is best if I drop v4
from my review-hans branch and Mark does a v5.

Mark, you can then also just squash the addition of the MAINTAINERS
entry into v5.

<snip>

>> +       int pwdlen;
> 
> Strictly speaking it should be size_t.
> 
>> +       pwdlen = strlen(buf);
>> +       if (buf[pwdlen-1] == '\n')
>> +               pwdlen--;
> 
> But the question is what will happen with the string like
> 'pa55\nw0rd\n' (note all \n:s)?
> See also below.

As I already explained in a previous reply the password cannot
contain '\n' so this cannot happen. I did miss that this was
still there, where as new_password_store() uses strreplace()
which is not really consistent with each other...

>> +       char *set_str = NULL, *new_setting = NULL;
>> +       char *auth_str = NULL;
> 
> The rule of thumb is to avoid forward assignments on stack allocated
> variables. It may decrease readability, hide real issues, and simply
> be unneeded churn, like here. Please revisit all of them in entire
> series.

I asked for all this to be set to NULL in my review of v2,
since there are various "goto out"s in the function and out:
calls kfree() on all 3, in v2 there was a path which would
end up calling kfree() on an uninitialized char *. IMHO just
initializing all of them up front is best here because that
guarantees that they are either still NULL, or point to
memory returned by kmalloc.

>> +               /* Remove the value part */
>> +               strreplace(item, ',', '\0');
> 
> This is kinda non-standard pattern.
> 
> I would see rather something like
> 
> char *p;
> 
> p = strchrnul(item, ',');
> *p = '\0';
> 
> Yes, it's longer, but better to understand what's going on here.

Erm, you actually suggested using strreplace() here in your previous review ...


>> +       if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE))
>> +               pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1;
> 
> Not sure if WARN_ON() is really what has to be called here. But I
> haven't checked the context deeply.

There are 2 max_lengths, one hardcoded in the driver so we don't
need to dynamically manage memory for the password storage and one
which is actually queried from the BIOS, the BIOS max-length should
always be less then the hardcoded one in the driver (and currently
this is true for all known models).

I suggested adding the WARN_ON so that if future BIOS-es ever bump
max_length we will get a bug-report about this and then we can
bump the driver's TLMI_PWD_BUFSIZE value.

Regards,

Hans
Andy Shevchenko May 27, 2021, 2:54 p.m. UTC | #8
On Thu, May 27, 2021 at 5:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/27/21 4:16 PM, Andy Shevchenko wrote:
> > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@lenovo.com> wrote:

...

> >> +       char *set_str = NULL, *new_setting = NULL;
> >> +       char *auth_str = NULL;
> >
> > The rule of thumb is to avoid forward assignments on stack allocated
> > variables. It may decrease readability, hide real issues, and simply
> > be unneeded churn, like here. Please revisit all of them in entire
> > series.
>
> I asked for all this to be set to NULL in my review of v2,
> since there are various "goto out"s in the function and out:
> calls kfree() on all 3, in v2 there was a path which would
> end up calling kfree() on an uninitialized char *. IMHO just
> initializing all of them up front is best here because that
> guarantees that they are either still NULL, or point to
> memory returned by kmalloc.

I see your point, fine with me!

...

> >> +               /* Remove the value part */
> >> +               strreplace(item, ',', '\0');
> >
> > This is kinda non-standard pattern.
> >
> > I would see rather something like
> >
> > char *p;
> >
> > p = strchrnul(item, ',');
> > *p = '\0';
> >
> > Yes, it's longer, but better to understand what's going on here.
>
> Erm, you actually suggested using strreplace() here in your previous review ...

There is strreplace() in use somewhere still which is what I suggested.
It might be that I missed the fact that in some places the change
happens to the ''\0' rather than another non-NUL character.

So, strreplace() is basically for changing '$a' to '$b' where neither
of them is NUL.

Otherwise we have
 - strsep()
 - strchrnul()
 - strtrim()

depending on the case.

Sorry if I was not completely clear about something.

...

> >> +       if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE))
> >> +               pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1;
> >
> > Not sure if WARN_ON() is really what has to be called here. But I
> > haven't checked the context deeply.
>
> There are 2 max_lengths, one hardcoded in the driver so we don't
> need to dynamically manage memory for the password storage and one
> which is actually queried from the BIOS, the BIOS max-length should
> always be less then the hardcoded one in the driver (and currently
> this is true for all known models).
>
> I suggested adding the WARN_ON so that if future BIOS-es ever bump
> max_length we will get a bug-report about this and then we can
> bump the driver's TLMI_PWD_BUFSIZE value.

I see your point, thanks for elaboration.
Mark Pearson May 28, 2021, 5:36 p.m. UTC | #9
Hi Andy,

On 2021-05-27 10:16 a.m., Andy Shevchenko wrote:
> On Wed, May 26, 2021 at 11:15 PM 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.
> 
> Thanks for an update! My comments below.
> 
> ...
> 
>> +/*
>> + * think-lmi.c - Think LMI BIOS configuration driver
> 
> It's not the best idea to include the file name into the file. If the
> file gets renamed (by whatever reason) often this either brings an
> additional burden or simply forgotten (as from my experience). I
> recommend to drop the file names from the source code.
Good advice - I'll update

> 
>> + * 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
>> + */
> 
> ...
> 
>> +#include <linux/acpi.h>
> 
> linux/errno.h ?
> 
>> +#include <linux/fs.h>
> 
> linux/string.h ?
> linux/types.h ?
> 
>> +#include <linux/wmi.h>
>> +#include "firmware_attributes_class.h"
>> +#include "think-lmi.h"

I hadn't included those as they call get pulled in by linux/acpi.h - so
aren't needed here. I take it best practice is to add them. I'd
deliberately stripped down the list to the bare minimu previously but
happy to reverse that if it's preferred.

> 
> ...
> 
>> + * Type:
>> + *  Method
>> + * Arguments:
>> + *  "Item,Value,Password,Encoding,KbdLang;"
>> + * Example:
>> + *  "WakeOnLAN,Disable,pswd,ascii,us;"
> 
> Is 'pswd' here an example of the password? Hacker's language can make
> it more visible, e.g. 'pa55w0rd'.
> Same for other examples.
Yes it is. I can update that :)
> 
>> + */
> 
> ...
> 
>> +       /*
>> +        * Duplicated call required to match bios workaround for behavior
> 
> bios -> BIOS
Ack
> 
>> +        * seen when WMI accessed via scripting on other OS.
>> +        */
> 
> ...
> 
>> +       struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj);
> 
> Candidate to have something like
> 
> #define to_tlmi_pwd_setting(obj)  container_of(...)
> 
> since it has appeared a few times.
Ack

> 
>> +       return sysfs_emit(buf, "%d\n", setting->valid);
>> +}
> 
>> +
> 
> Unneeded blank line. Same for other similar places.
Shoot - I thought I'd cleaned a bunch of these up. I'll do another pass.
> 
>> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled);
> 
> Hmm... We have already define_one_global_ro(). The problems with that
> are the name and location. But perhaps you can copy that macro here
> with the same name and at least we can see the common grounds to clean
> up in the future. Another possibility is to comment that it can be
> replaced with the above mentioned macro. Ideally would be to refactor
> right now, but it's not anyhow crucial or required for this series, so
> may be postponed.
OK - I'll have a look. I'm not 100% sure I understand the issues, but
hopefully it will become clearer once I play with it a bit
> 
> ...
> 
>> +       int pwdlen;
> 
> Strictly speaking it should be size_t.
> 
>> +       pwdlen = strlen(buf);
>> +       if (buf[pwdlen-1] == '\n')
>> +               pwdlen--;
> 
> But the question is what will happen with the string like
> 'pa55\nw0rd\n' (note all \n:s)?
> See also below.
I know there is discussion on this in future mails, so I'll leave this
one for those
> 
>> +       /* pwdlen == 0 is allowed to clear the password */
>> +       if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))
> 
> The ' != 0' part is redundant.
Ack
> 
>> +               return -EINVAL;
> 
>> +       memcpy(setting->password, buf, pwdlen);
>> +       setting->password[pwdlen] = '\0';
> 
> I'm not sure why we can't use strscpy() like you did in the other function.
I'm not sure either :) I'll update
> 
>> +       return count;
> 
> ...
> 
>> +       /* Strip out CR if one is present, setting password won't work if it is present */
>> +       strreplace(new_pwd, '\n', '\0');
> 
> Basically it will stop on the first one. See the strchrnul() trick below.
Thanks for the strchrnul trick - happy to use that

> 
>> +       pwdlen = strlen(new_pwd);
>> +       /* pwdlen == 0 is allowed to clear the password */
>> +       if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
> 
> No need for ' != 0'.
Ack
> 
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
> 
>> +}
> 
> 
> ...
> 
>> +       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;
> 
> Similar comments as per above.
Ack
> 
> ...
> 
>> +       char *set_str = NULL, *new_setting = NULL;
>> +       char *auth_str = NULL;
> 
> The rule of thumb is to avoid forward assignments on stack allocated
> variables. It may decrease readability, hide real issues, and simply
> be unneeded churn, like here. Please revisit all of them in entire
> series.
Covered in Hans reply. Leaving as is
> 
> ...
> 
>> +       /* Strip out CR if one is present */
>> +       strreplace(new_setting, '\n', '\0');
> 
> As per above.
> 
> ...
> 
>> +               /* Remove the value part */
>> +               strreplace(item, ',', '\0');
> 
> This is kinda non-standard pattern.
> 
> I would see rather something like
> 
> char *p;
> 
> p = strchrnul(item, ',');
> *p = '\0';
> 
> Yes, it's longer, but better to understand what's going on here.
Fair enough. I'll change. I quite liked the fact I had a one-liner do
the same thing - oh well :)
> 
>> +               /* Create a setting entry */
>> +               setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL);
> 
> sizeof(*setting) ?
OK.

> 
>> +               if (!setting) {
>> +                       ret = -ENOMEM;
>> +                       goto fail_clear_attr;
>> +               }
>> +               setting->index = i;
>> +               strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN);
> 
> ...
> 
>> +       sprintf(tlmi_priv.pwd_admin->display_name, "admin");
>> +       sprintf(tlmi_priv.pwd_admin->kbdlang, "us");
> 
> Not sure why you need printf() type of function here. strcpy() or
> strscpy() should be enough.
OK - I can update.
> 
> ...
> 
>> +       if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE))
>> +               pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1;
> 
> Not sure if WARN_ON() is really what has to be called here. But I
> haven't checked the context deeply.
Covered in Hans' reply.. Leaving as is
> 
> ...
> 
>> +       sprintf(tlmi_priv.pwd_power->display_name, "power-on");
>> +       sprintf(tlmi_priv.pwd_power->kbdlang, "us");
> 
> As above.
Ack
> 
> ...
> 
>> +#ifndef _THINK_LMI_H_
>> +#define _THINK_LMI_H_
> 
> + linux/types.h
> 
> (At leas bool is from there)
Ack
> 
> 
>> +#endif /* !_THINK_LMI_H_ */
>
Andy Shevchenko May 28, 2021, 6:19 p.m. UTC | #10
On Fri, May 28, 2021 at 8:36 PM Mark Pearson <markpearson@lenovo.com> wrote:
> On 2021-05-27 10:16 a.m., Andy Shevchenko wrote:
> > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@lenovo.com> wrote:

...

> >> +#include <linux/acpi.h>
> >
> > linux/errno.h ?
> >
> >> +#include <linux/fs.h>
> >
> > linux/string.h ?
> > linux/types.h ?
> >
> >> +#include <linux/wmi.h>
> >> +#include "firmware_attributes_class.h"
> >> +#include "think-lmi.h"
>
> I hadn't included those as they call get pulled in by linux/acpi.h - so
> aren't needed here. I take it best practice is to add them. I'd
> deliberately stripped down the list to the bare minimu previously but
> happy to reverse that if it's preferred.

Basic headers like that are kinda what almost any file uses, but
acpi.h is not the one which guarantees their inclusion (you see, it's
a layering violation: acpi is not low level generic library).

...

> >> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled);
> >
> > Hmm... We have already define_one_global_ro(). The problems with that
> > are the name and location. But perhaps you can copy that macro here
> > with the same name and at least we can see the common grounds to clean
> > up in the future. Another possibility is to comment that it can be
> > replaced with the above mentioned macro. Ideally would be to refactor
> > right now, but it's not anyhow crucial or required for this series, so
> > may be postponed.
> OK - I'll have a look. I'm not 100% sure I understand the issues, but
> hopefully it will become clearer once I play with it a bit

Including cpufreq.h in your code will seem weird. That's why I marked
it as an issue.

...

> >> +       pwdlen = strlen(buf);
> >> +       if (buf[pwdlen-1] == '\n')
> >> +               pwdlen--;
> >
> > But the question is what will happen with the string like
> > 'pa55\nw0rd\n' (note all \n:s)?
> > See also below.
> I know there is discussion on this in future mails, so I'll leave this
> one for those

OK.

...

> >> +               /* Remove the value part */
> >> +               strreplace(item, ',', '\0');
> >
> > This is kinda non-standard pattern.
> >
> > I would see rather something like
> >
> > char *p;
> >
> > p = strchrnul(item, ',');
> > *p = '\0';
> >
> > Yes, it's longer, but better to understand what's going on here.
> Fair enough. I'll change. I quite liked the fact I had a one-liner do
> the same thing - oh well :)

Me too, but the problem with strreplace(..., '\0') is that it sounds
like a half-baked strsep() solution. First of all, it will go and
replace all \n to \0 while you stop anyway on the first one in the
following code.
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
index 8ea59fea4..3348bf80a 100644
--- a/Documentation/ABI/testing/sysfs-class-firmware-attributes
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -197,8 +197,24 @@  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.
+		On Lenovo systems if you change the Admin password the new password is not active until
+		the next boot.
+
+		Lenovo specific class extensions
+		------------------------------
+
+		On Lenovo systems the following additional settings are available:
+
+		lenovo_encoding:
+					The encoding method that is used. This can be either "ascii"
+					or "scancode". Default is set to "ascii"
+
+		lenovo_kbdlang:
+					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"
 
 What:		/sys/class/firmware-attributes/*/attributes/pending_reboot
 Date:		February 2021
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 57da8352d..c61aab19e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -639,6 +639,17 @@  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"
+	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..3ccae8224
--- /dev/null
+++ b/drivers/platform/x86/think-lmi.c
@@ -0,0 +1,884 @@ 
+// 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 struct tlmi_err_codes tlmi_errs[] = {
+	{"Success", 0},
+	{"Not Supported", -EOPNOTSUPP},
+	{"Invalid Parameter", -EINVAL},
+	{"Access Denied", -EACCES},
+	{"System Busy", -EBUSY},
+};
+
+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)
+{
+	int i;
+
+	for (i = 0; i < sizeof(tlmi_errs)/sizeof(struct tlmi_err_codes); i++) {
+		if (!strcmp(tlmi_errs[i].err_str, errstr))
+			return tlmi_errs[i].err_code;
+	}
+	return -EPERM;
+}
+
+/* Extract error string from WMI return buffer */
+static int tlmi_extract_error(const struct acpi_buffer *output)
+{
+	const union acpi_object *obj;
+
+	obj = output->pointer;
+	if (!obj)
+		return -ENOMEM;
+	if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
+		kfree(obj);
+		return -EIO;
+	}
+	return tlmi_errstr_to_err(obj->string.pointer);
+}
+
+/* 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);
+		kfree(output.pointer);
+		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;
+	char *s;
+
+	obj = output->pointer;
+	if (!obj)
+		return -ENOMEM;
+	if (obj->type != ACPI_TYPE_STRING || !obj->string.pointer) {
+		kfree(obj);
+		return -EIO;
+	}
+
+	s = kstrdup(obj->string.pointer, GFP_KERNEL);
+	if (!s)
+		return -ENOMEM;
+	*string = s;
+	return 0;
+}
+
+/* ------ 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)
+		return -ENOMEM;
+	if (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;
+	int ret;
+
+	status = wmi_query_block(guid_string, item, &output);
+	if (ACPI_FAILURE(status)) {
+		kfree(output.pointer);
+		return -EIO;
+	}
+
+	ret = tlmi_extract_output_string(&output, value);
+	kfree(output.pointer);
+	return ret;
+}
+
+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;
+	int ret;
+
+	status = wmi_evaluate_method(LENOVO_GET_BIOS_SELECTIONS_GUID,
+				     0, 0, &input, &output);
+
+	if (ACPI_FAILURE(status)) {
+		kfree(output.pointer);
+		return -EIO;
+	}
+
+	ret = tlmi_extract_output_string(&output, value);
+	kfree(output.pointer);
+	return ret;
+}
+
+/* ---- 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);
+
+	return sysfs_emit(buf, "%d\n", setting->valid);
+}
+
+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 pwdlen;
+
+	pwdlen = strlen(buf);
+	if (buf[pwdlen-1] == '\n')
+		pwdlen--;
+
+	/* pwdlen == 0 is allowed to clear the password */
+	if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))
+		return -EINVAL;
+
+	memcpy(setting->password, buf, pwdlen);
+	setting->password[pwdlen] = '\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);
+	int ret, pwdlen;
+	char *new_pwd;
+	char *auth_str;
+
+	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;
+
+	/* Strip out CR if one is present, setting password won't work if it is present */
+	strreplace(new_pwd, '\n', '\0');
+
+	pwdlen = strlen(new_pwd);
+	/* pwdlen == 0 is allowed to clear the password */
+	if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' */
+	auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s,%s,%s;",
+		 setting->pwd_type, setting->password, new_pwd,
+		 encoding_options[setting->encoding], setting->kbdlang);
+	if (!auth_str) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	ret = tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str);
+	kfree(auth_str);
+out:
+	kfree(new_pwd);
+	return 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)
+{
+	struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj);
+
+	return sysfs_emit(buf, "%s\n", setting->role);
+}
+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);
+	char *set_str = NULL, *new_setting = NULL;
+	char *auth_str = NULL;
+	int ret;
+
+	if (!tlmi_priv.can_set_bios_settings)
+		return -EOPNOTSUPP;
+
+	new_setting = kstrdup(buf, GFP_KERNEL);
+	if (!new_setting)
+		return -ENOMEM;
+
+	/* Strip out CR if one is present */
+	strreplace(new_setting, '\n', '\0');
+
+	if (tlmi_priv.pwd_admin->valid && tlmi_priv.pwd_admin->password) {
+		auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
+				tlmi_priv.pwd_admin->password,
+				encoding_options[tlmi_priv.pwd_admin->encoding],
+				tlmi_priv.pwd_admin->kbdlang);
+		if (!auth_str) {
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
+
+	if (auth_str)
+		set_str = kasprintf(GFP_KERNEL, "%s,%s,%s", setting->display_name,
+				new_setting, auth_str);
+	else
+		set_str = kasprintf(GFP_KERNEL, "%s,%s;", setting->display_name,
+				new_setting);
+	if (!set_str) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	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("");
+
+out:
+	kfree(auth_str);
+	kfree(set_str);
+	kfree(new_setting);
+	return 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;
+
+	kattr = container_of(attr, struct kobj_attribute, attr);
+	if (kattr->show)
+		return kattr->show(kobj, kattr, buf);
+	return -EIO;
+}
+
+static ssize_t tlmi_attr_store(struct kobject *kobj, struct attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct kobj_attribute *kattr;
+
+	kattr = container_of(attr, struct kobj_attribute, attr);
+	if (kattr->store)
+		return kattr->store(kobj, kattr, buf, count);
+	return -EIO;
+}
+
+static const struct sysfs_ops tlmi_kobj_sysfs_ops = {
+	.show	= tlmi_attr_show,
+	.store	= tlmi_attr_store,
+};
+
+static void tlmi_attr_setting_release(struct kobject *kobj)
+{
+	struct tlmi_attr_setting *setting = container_of(kobj, struct tlmi_attr_setting, kobj);
+
+	kfree(setting);
+}
+
+static void tlmi_pwd_setting_release(struct kobject *kobj)
+{
+	struct tlmi_pwd_setting *pwd = container_of(kobj, struct tlmi_pwd_setting, kobj);
+
+	kfree(pwd);
+}
+
+static struct kobj_type tlmi_attr_setting_ktype = {
+	.release        = &tlmi_attr_setting_release,
+	.sysfs_ops	= &tlmi_kobj_sysfs_ops,
+};
+
+static struct kobj_type tlmi_pwd_setting_ktype = {
+	.release        = &tlmi_pwd_setting_release,
+	.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_get(&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, &tlmi_attr_setting_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, &tlmi_pwd_setting_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, &tlmi_pwd_setting_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_put();
+	return ret;
+}
+
+/* ---- Base Driver -------------------------------------------------------- */
+static int tlmi_analyze(void)
+{
+	struct tlmi_pwdcfg pwdcfg;
+	acpi_status status;
+	int i, 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) {
+		struct tlmi_attr_setting *setting;
+		char *item = NULL;
+
+		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 '\'. */
+		strreplace(item, '/', '\\');
+
+		/* Remove the value part */
+		strreplace(item, ',', '\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;
+	tlmi_priv.pwd_admin->pwd_type = "pap";
+	tlmi_priv.pwd_admin->role = "bios-admin";
+	tlmi_priv.pwd_admin->minlen = pwdcfg.min_length;
+	if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE))
+		pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1;
+	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;
+	tlmi_priv.pwd_power->pwd_type = "pop";
+	tlmi_priv.pwd_power->role = "power-on";
+	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_put();
+}
+
+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..05005576b
--- /dev/null
+++ b/drivers/platform/x86/think-lmi.h
@@ -0,0 +1,80 @@ 
+/* 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_BUFSIZE     129
+#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)
+
+/* Possible error values */
+struct tlmi_err_codes {
+	const char *err_str;
+	int err_code;
+};
+
+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_BUFSIZE];
+	const char *pwd_type;
+	const char *role;
+	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_ */
+