Re: [PATCH] Introduce support for Systems Management Driver over WMI for Dell Systems

From: Hans de Goede
Date: Tue Sep 22 2020 - 05:02:38 EST


Hi,

On 9/21/20 5:26 PM, Limonciello, Mario wrote:

Well if different schemes are supported and each scheme has its own type,
then I would expect there to be say / e.g.:

/sys/class/firmware-attributes/dell/authentication/admin-password
(with a type of "password") and:
/sys/class/firmware-attributes/dell/authentication/admin-hotp
(with a type of "hotp")

And then the user / userspace can choose which one to use,
I guess if the kernel knows that only hotp has been setup and
there is no standard password set, then it could hide the
/sys/class/firmware-attributes/dell/authentication/admin-password
password.

So you're proposing the flow to userspace that would look like this:

Authentication is off
----
# cat /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_needed
0
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value



Turning on and things that happen using authentication (error examples too):
----
# cat /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_needed
0
# echo "foobar123" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/new_password
# cat /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_needed
1
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
-EOPNOTSUPP

Why would this be -EOPNOTSUPP and not just -EACCESS too ? To in the end both are access denials,
no password being set is really just a variant of the wrong password being set.

# echo "foobar456" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/current_password
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
-EACCES
# echo "foobar123" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/current_password
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
# echo "" | sudo tee /sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/current_password
# echo "enabled" | sud tee /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/current_value
-EOPNOTSUPP



TBH I think all these things are (mostly) easily solvable if/when we
encounter them. I mean it is definitely good to keep these kind of things
in mind. But at some point we might get lost in all the what-ifs we
can come up with.

In trying to come up with a generic interface that scales to everyone's needs
the what-ifs are critical. Making assumptions on how authentication works means
future authentication mechanisms will be painful.

The way I see it the authentication mechanism and the ABI for actually
changing the settings are pretty much orthogonal. So we can add new
authentication mechanisms without that impacting the
ABI for actually changing the settings.

If a vendor comes along where authentication is not necessary
for *all* attributes, then we could add the "is_authentication_required"
as an optional sysfs-attribute for the firmware-attributes and state
in the documentation that if that file is lacking that means that
authentication is always required. That way the Dell code would not
even have to have the "is_authentication_required" sysfs-attribute.

But it's not true on Dell's systems even right now. If you don't have
an Admin password configured then you don't need it set for any attribute.
If you do have one set you need them for all. And if you need to know to look for
/sys/class/firmware-attributes/dell-wmi-sysman/authentication/Admin/is_password_set
then userspace needs to know to do this differently for Dell and someone else.

You are mixing up authentication and authorization here.

I guess that you are arguing for is an is_authorization_required
attribute, which can return "none" and "admin" (for now).

When no Admin password is set, everyone automatically get admin
level authority and in the Dell case (for now) all firmware-attributes
require admin authorization.

Looking at it this way has the advantage that for the current Dell
scenario the is_authorization_required sysfs-attribute would simply
always return admin.

Which is why I argued that we could omit it for now and add it as
an optional attribute, defaulting to admin when not present, later.

But if you want to we can certainly add it now and have it present
from the get go.

So you either need to have a top level is_authentication_required
IE /sys/class/firmware-attributes/dell-wmi-sysman/is_authentication_required

Or a per attribute one
IE /sys/class/firmware-attributes/dell-wmi-sysman/attributes/Touchscreen/is_authentication_required

This obviously needs to be a per firmware-attribute setting, because in
the future and/or with other vendors different attributes may require
different levels of authorization.

And this decision can't be put off because it has an implication that another
vendor may choose to do their authentication differently than Dell.


Since we also seem to have some trouble to get these 2 properly documented
(I have not looked at v3 yet), I'm fine with making them dell specific by
prefixing them
with dell-. I guess that that probably even makes sense.

They're documented in v3. The moment that you have a "Dell specific" attribute
what's the point of a common class? You're going to end up with Dell expresses
dependencies this way, Lenovo expresses them that way, and HP expresses them some
other way and userspace is going to have to sort out the differences.

So in userspace you end up with logic that is something like this:
1) (Generic) Check if authentication is set
2) (Dell) Check if you're running on Dell's driver, interpret this dependency or show a message
3) (Lenovo) Check if you're running on Lenovo's driver, interpret this dependency or show a message
4) (HP) Check if you're running on HP's driver, interpret this dependency or show a message
5) (Generic) Check what authentication schemes are supported
6) (Dell) Apply Dell's admin password authentication scheme
7) (Lenovo Example) Apply Lenovo's admin password authentication scheme or their TOTP authentication scheme
8) (Generic) write value into current_value
9) (Generic) Disable authentication

So if userspace is going to have to be different anyway for evaluating dependencies and authentication, why
go through the trouble to fit everyone into the same class?

Parsing the dependencies is not required for a functional userspace
application. As I explicitly stated in my previous email in many
existing builtin firmware setup UIs the dependencies are not even
shown. As for the authentication for now having a straight forward
admin + system/boot level passwords covers like 99% of the
existing use-cases. There is no need for separate Dell / Lenovo
admin password schemes as you list above those will work identically
from the sysfs interface pov.

So having a common class interface will allow userspace apps which
work with 99% of the systems currently out there (assuming they ever get
a driver implementing the class).

<snip>

Specifying what changing the attributes actually does
falls
(way) outside of the scope of the sysfs ABI IMHO. That will always be the case
of please consult your Laptop's / Workstation's / Server's manual.
That is actually not much different from the current builtin
firmware setup utility experience where the help text is often,
well, not helpful.

For all I care there is an enum called "HWvirt" with a description of
"Hardware virtualization support" and values of "Enabled" and "Disabled"
which controls something somewhat or even totally different from what the
name and description suggest. That would be less then ideal, but not a problem
from the pov of the sysfs ABI for firmware-attributes. It would be a simple
case of the garbage in garbage out principle.

So this is one problem which I'm happy to punt to userspace and I guess Dell
might do a Dell specific utility, which only works one certain model Dell's,
which is a lot fancier then the basic sysfs functionality and e.g. consumes
the dell-value_modifier and dell-modifier sysfs-attribures.

The goal here is that all of the functionality that would otherwise be expressed
in a proprietary utility could also be expressed in sysfs. Having to de-feature
the sysfs interface for the purpose of fitting into what's generic across vendors
defeats that goal and is why I think it should be a Dell interface in the first
place.

I have not asked you to de-feature the sysfs interface anywhere in this thread!

Regards,

Hans