Message-ID: <DM6PR19MB263641DF4B7442CEE31BEAB5FA360@DM6PR19MB2636.namprd19.prod.outlook.com>
Date:   Fri, 25 Sep 2020 18:37:47 +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>,
        Mark Pearson <mpearson@...ovo.com>
Subject: RE: [PATCH v4] Introduce support for Systems Management Driver over
 WMI for Dell Systems

> 
> I really prefer min_value and max_value here, those have 2 advantages:
> 
> 1. min/max is used almost everywhere in the kernel/sysfs for things like this,
>     upper-/lower-bound feels a bit like archaic Enlish to me.
> 
> 2. The _value postfix makes it immediately clear that they are the bounds for
>     the current_value attribute.
> 
> Also in case of the password sysfs inteface min/max_lenght is used, not
> e.g. password_length_lower_bound. So again using min/max seems more
> consistent.

OK.

> 
> Sure I would be happy to have it documented that all firmware-attributes
> sysfs-files
> are optional *except for type and current_value*. If you prefer that go for
> it.

OK.

> 
> >
> > +
> >>> +		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
> 
> It seems you never finished your sentence here?

Whoops - yeah I was jumping around in my response and forgot to finish my thought.

I was going to suggest that if necessary a compatible way to add these would
be symlinks.  So if the directory right now was Admin and later was had
split to something like AdminHotp/AdminPassword
but wanted to discriminate between the old Admin it could be Admin->AdminPassword
or vice versa.

> 
> >> 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.
> 
> If re-using the WMI names is important for you, then having a sysfs-attribute
> with some standardized value to say what is what inside the authentication-
> sub-dir
> is fine with me.
> 
> Except that I would not call it type, when thinking about authentication-types
> I think about things like password / totp / hotp. Can we call the sysfs-
> attribute
> for this "role" instead ?

That sounds good to me.

> 
> > 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.
> 
> But one role can still have multiple mechanisms, so for Dell in the future
> we could have say:
> 
> /sys/class/firmware-attributes/dell/authentication/Admin-password
> /sys/class/firmware-attributes/dell/authentication/Admin-hotp
> /sys/class/firmware-attributes/dell/authentication/System-password
> 
> So although I'm fine with taking the role_name directly from WMI
> (combined with a roll attribute with standardized values) I think
> we still need to postfix a -password to it now, to allow room
> for adding say a -hotp mechanism for the same role_name in the
> future ?

Could this be captured in the role attribute instead perhaps?  So the role
attributes values could hypothetically be:
bios-admin-password
power-on-password

And if HOTP is added some day these could be added:
bios-admin-hotp
power-on-hotp

> 
> (I guess this also ties into your unanswered question from above.
> 
> >> 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.
> 
> Ack, so as I asked above, what do you think of using "role" here instead?
> 

+1

> 
> >> 	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.
> 
> I did not make that example up, Lenovo really as a scan-codes encoding for
> their password authentication mechanism, see:
> 
> https://download.lenovo.com/pccbbs/mobiles_pdf/kbl-r_deploy_01.pdf
> 
> > Think back to how this all works on Windows...
> 
> <snip>
> 
> > So if you had to manually convert to scancodes, that would not at all user
> friendly.
> 
> It definitely is not user-friendly.
> 
> Note when calling the Lenovo WMI functions you can specify in which encoding
> (ascii or scancodes) you are providing the data and all Lenovo's examples
> use the ascii encoding.  But there also is a bitfield with supported encodings
> and I guess some older firmware may only support the scancodes variant?
> 
> It is all somewhat not nice, which is why I prefixed the scancodes encoding
> with lenovo- to make it clear that it is non-standard.
> 
> But lets say all Lenovo models support the ascii encoding and we never expose
> the scancodes bit to userspace. 

The documentation you linked doesn't seem to indicate when to use scancodes or
ASCII to me, so I can't draw any conclusions if certain models support one or
the other.

I would suggest yes please don't support scancodes from the sysfs perspective for
another vendor's implementation.  We should probably keep sysfs as utf8 and let
any conversions be hidden in the kernel driver if necessary. 

> Even then there still is the unicode (utf8
> in sysfs, utf16 at the WMI level for Dell) vs ascii issue and it would be nice
> if a UI for this could give an error when the user tries to use non ascii
> chars in a password for a vendor implementation...

I think that the kernel driver can certainly parse and provide -EINVAL in this
context. 

> 
> > 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.
> 
> I guess that would work (Dell not offering it). Which makes me realize that
> we should specify somewhere in the doc that all sysfs files which contain
> a string value, the encoding is UTF8 unless explicitly specified otherwise.
> (for that specific sysfs-attribute).

Yeah.  I guess the very top where we will modify to mention that all attributes
are optional we can also mention that the encoding is UTF8 unless otherwise
specified.

Andy - 

BTW - with the maintainer change in platform-x86 should Divya stop CC you and
Darren?

Powered by blists - more mailing lists