Message-ID: <a8bf92ca-641a-3006-9876-d57ffa47ecb5@redhat.com>
Date:   Tue, 22 Sep 2020 11:02:27 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     "Limonciello, Mario" <Mario.Limonciello@...l.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>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>,
        Richard Hughes <rhughes@...hat.com>,
        Jared Dominguez <jaredz@...hat.com>
Subject: Re: [PATCH] Introduce support for Systems Management Driver over WMI
 for Dell Systems

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

Powered by blists - more mailing lists