Date:   Fri, 25 Sep 2020 15:32:00 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@...l.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Divya Bharathi <divya27392@...il.com>,
        "dvhart@...radead.org" <dvhart@...radead.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "Bharathi, Divya" <Divya.Bharathi@...l.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        mark gross <mgross@...ux.intel.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>
Subject: RE: [PATCH v4] Introduce support for Systems Management Driver over
 WMI for Dell Systems

So I do want to preface this response by mentioning that Dell's implementation
is based off the PLDM specification from the DMTF.
https://www.dmtf.org/sites/default/files/standards/documents/DSP0247_1.0.0.pdf

A lot of the nomenclature that has been already proposed to change followed
nomenclature previously used in the PLDM specification.  In the spirit of matching
that specification in the Linux implementation we may consider to change things
like proposed to be renamed min_value back to lower_bound, but that's up to your
decision.

> 
> This list seems to miss a "type" sysfs attribute, which tells which type
> the firmware-attribute in question is. Sorry for not noticing that sooner.

Whoops :)

> 
> > +
> > +		current_value:	A file that can be read to obtain the current
> > +		value of the <attr>
> 
> Can you indent the continuation on the second line to align with the start
> of the description on the first line please?, e.g.:
> 
> 		current_value:	A file that can be read to obtain the current
> 				value of the <attr>
> 
> This goes for all the sysfs-attribute description texts.
> 
> > +
> > +		This file can also be written to in order to update
> > +		the value of a <attr>
> > +
> > +		default_value:	A file that can be read to obtain the default
> > +		value of the <attr>
> > +
> > +		display_name:	A file that can be read to obtain a user friendly
> > +		description of the at <attr>
> > +
> > +		display_name_language_code:	A file that can be read to obtain
> > +		the IETF language tag corresponding to the "display_name" of the
> <attr>
> > +
> > +		"enumeration"-type specific properties:
> > +
> > +		possible_values: A file that can be read to obtain the possible
> > +		values of the <attr>. Values are separated using semi-colon.
> 
> As non-native English speaker I had to lookup semi-colon to make sure that
> it indeed is ';' as I already sorta expected. So can we add "(';')" (without
> the "") behind the semi-colon to make this easier to parse for non-native
> English
> speakers?


Some parts of the kernel documentation seem to use semi-colon (``;``) - example of
admin-guide/bootconfig.rst

and others just set semi-colon - example of admin-guide/device-mapper/dm-init.rst.

But I don't see a problem with this change.

> 
> > +		"integer"-type specific properties:
> > +
> > +		min_value:	A file that can be read to obtain the lower
> > +		bound value of the <attr>
> > +
> > +		max_value:	A file that can be read to obtain the upper
> > +		bound value of the <attr>
> > +
> > +		scalar_increment:	A file that can be read to obtain the
> > +		resolution of the incremental value this attribute accepts.
> 
> Can we have an example here? I guess if for some reason only even/odd values
> are allowed then this would contain "2" ?

Maybe adopting the PLDM description is better to explain it.

"The scalar value that is used for the increments to this integer"
> 
> > +
> > +		"string"-type specific properties:
> > +
> > +		max_length:	A file that can be read to obtain the maximum
> > +		length value of the <attr>
> > +
> > +		min_length:	A file that can be read to obtain the minimum
> > +		length value of the <attr>
> 
> So I have been taking a look at a community written driven to allow changing
> BIOS-settings / firmware-attributes on some Lenovo laptops:
> 
> https://github.com/iksaif/thinkpad-wmi
> 
> My main reason for doing so is to check if other vendors also support things
> like "display_name", "default_value", etc.
> 
> So for the normal attributes, it seems that for the Thinkpad WMI interface
> they
> are all enums and the do contain possible_values. So that is fine.
> 
> But they do not have a separate display_name only a name-name, nor do they
> have a default_value.
> 
> So I think we should mark the display_name, display_name_language_code and
> default_value sysfs-attributes optional, e.g. make the description look like
> this:
> 
> 		default_value:	An optional file that can be read to obtain the
> 				default value of the <attr>
> 

At this point, why don't we just make a declaration at the top that all
attributes are optional?  If you want this to scale to non-BIOS implementations
of settings you couldn't know what they'll be able to offer and it sets
an expectation that userspace to need to look for every sysfs file exists rather
than just the vendor specific ones.

+
> > +		Drivers may emit a CHANGE uevent when a password is set or unset
> > +		userspace may check it again.
> 
> First of all some generic remarks:
> 
> Currently the "Admin" and "System" names come directly from the Dell WMI
> interface. I have 2 concerns with this:
> 
> 1) What if we do get multiple authentication mechanisms for a single user,
> e.g. both a type == "pasword" and type == "hotp" authentication. The way I
> have
> been thinking about that sofar, is that we then get 2 admin dirs under the
> /sys/class/firmware-attributes/*/authentication dir, with a type attribute
> per dir, following how we do the attributes. So we would get e.g. these 2
> dirs:
> 
> /sys/class/firmware-attributes/dell/authentication/admin-password
> /sys/class/firmware-attributes/dell/authentication/admin-hotp
> 
> For the admin user. If want to do it like this in the future we should
> add some indirection between the WMI username and the dir which is created
> now and create the Admin dir as admin-password starting now.

Yeah I think if HOTP is added to some vendor some day that's how it would work.
The indirection can be added at that time.  One way to do this is to add

> 
> 2) The "Admin" name is clear enough, but the "System" name is somewhat
> ambiguous and other vendors may call this differently, I think I have
> at least seen it called the "User" password in some cases and Lenovo
> seems to call it a power-on-password. I think that just calling it the
> "boot" password makes sense. My main concern is that "System" is a bit
> too vague. So then for now we would get:
> 
> /sys/class/firmware-attributes/dell/authentication/admin-password
> /sys/class/firmware-attributes/dell/authentication/boot-password

I want to be cognizant that vendors are going to call things differently
in their attributes and we want the specifications and/or whitepapers that
vendors use to refer to this to make sense to the system's administrator.

Dell uses the nomenclature "System" in all of it's documentation. If the Linux
implementation calls it "boot password" or "power on password" it will be
confusing to decode when someone see it. 

Dell also has other terms used such as Master password and HDD password.
They're not exposed in this interface but these all might have a different
connotation across vendors.

So instead I would propose that within the folder the "type" attribute
correspond to something decodable.  So the name the vendor uses is the
folder and the type of password is within a sysfs file "type".

Proposed types:
* "bios-admin"
* "power-on"

Those two types can then be hardcoded by the implementation.

So a user would see the different authentication mechanisms available
by looking At the contents of /sys/class/firmware-attributes/*/authentication
and if they don't understand it's purpose they look at the type sysfs file.

> 
> The spec. should also specify that the part before the first '-' is the
> username, and the part after it is the authentication type. E.g. the
> docs for this could look something like this:
> 
> 	Directories under /sys/class/firmware-attributes/*/authentication/
> 	use the following directory-name pattern:
> 	<username>-<authentication_method>
> 
> 	Where username must be one of: "admin" or "boot":

Username is inappropriate in this context, especially since firmware doesn't
have a concept of multi-user.  It's a configurable permissions scheme to what
you are allowed to do in firmware.

> 
> 	admin	If any authentication_method is enabled for the admin user, then
> 		authentication as the admin user is required to modify BIOS
> settings.
> 	boot	If any authentication_method is enabled for the admin user, then
> 		authentication as the boot user is required to boot the machine.
> 
> 	And where authentication_method must be "password". Note in the future
> 	both more usernames and more authentication_method-s may be added.
> 
> 	All authentication_methods must have the following sysfs-attributes:
> 
> 	is_enabled:  This reads "1" if the authentication_method is enabled,
> 		     and "0" if its disabled
> 
> 	Any changes to authentication_methods will generate a change uevent,
> 	upon receiving this event applications should recheck the authentication
> 	settings such as the is_enabled flag.
> 
> 	Password authentication_method specific sysfs-attributes:
> 
> 	max_password_length: ... (continue with the old text)
> 
> Note:
> 
> 1) This is a proposal to make the authentication bits a bit more generic /
>     future proof. This is very much open for discussion.
> 
> 2) The new generic is_enabled sysfs-attribute replaces the
> is_authentication_set flag
> 
> ###
> 
> So as with the actual firmware-attributes I have also been comparing the
> authentication
> bits for the Dell case with the community thinkpad_wmi code. And again things
> are a pretty
> good match already, including being able to query a minimum and maximin
> password length.
> 
> The only thing which is different, which I think would be good to add now, is
> a password_encoding sysfs-attribute. The Lenovo password interface supports
> 2 encodings, "ascii" and "scancodes". Although I wonder if scancodes still
> works on modern UEFI based platforms.
> 
> Since the Dell password code uses UTF8 to UTF16 translation routines, I guess
> the encoding for the Dell password is UTF8 (at the sysfs level). So I would
> like to propose
> an extra password-authentication attribute like:
> 
> 	password_encoding:  A file that can be read to obtain the encoding used
> by
> 			    the current_password and new_password attributes.
> 			    The value returned should be one of: "utf8", "ascii".
> 			    In some cases this may return a vendor-specific encoding
> 			    like, e.g. "lenovo-scancodes".
> 
> And for the Dell driver this would just be hardcoded to utf8.

I don't really believe that another vendor's implementation would be likely to
use scan codes for the input into the WMI method.

Think back to how this all works on Windows...

Here is the Dell whitepaper that supports this interface for Windows:
https://downloads.dell.com/manuals/common/dell-agentless-client-manageability.pdf

(And yes I acknowledge we should probably reference this and the PLDM spec it's based
From in the commit message)

You're supposed to get a WMI method which you can interact with from Powershell.
You get a reference to the namespace, and then you make a function call from something
in the binary MOF (which maps to an ACPI WM** method).

This is how a command using a password argument works on Windows (from that whitepaper)

$pwd = ”PASSWORD”
$encoder = New-Object System.Text.UTF8Encoding
$bytes = $encoder.GetBytes($pwd)
$BAI.SetAttribute(1,$bytes.Length,$bytes,"UefiNwStack","Disabled")

I don't see any other encoders other than Unicode in the System.Text namespace
https://docs.microsoft.com/en-us/dotnet/api/system.text?view=netcore-3.1

So if you had to manually convert to scancodes, that would not at all user friendly.

I would much prefer that this attribute only be added if it's actually deemed
necessary.  Or to my point that all attributes can be considered optional, Dell's
implementation would just not offer it.

Powered by blists - more mailing lists