Date:   Mon, 21 Sep 2020 12:02:54 +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/17/20 6:18 PM, Limonciello, Mario wrote:
>>> Those are very different flows to get to and change the same "types" of
>> data.  By Dell's interface
>>> being Dell specific we can guarantee that the documented flow works how it
>> should.
>>
>> Documenting the flow could be part of the documentation for the
>> different passwd types.
> 
> In the concept of a "generic" API I don't think the word "password" is futureproof
> and it would need to be avoided.  I think a better term would be "authentication".

Ack.

>> For how things currently work the User and
>> Admin password attributed would have a type of "password", the hash
>> example you gave will have a different type for its password attribute,
>> e.g. "hotp" and not only the type could be different but also
>> the sysfs-attributes, e.g. the "Admin" password-dir which has a "type"
>> sysfs-atrribute which returns "htop" may not have a current_password
>> attribute at all, instead it may would have a hash attribute, making
>> it (more) clear that this one works differently.
> 
> In our definition `current_password` is intentionally not readable by userspace.
> One process could be writing it (think obtaining it from an escrow service) and
> another interacting with attributes, and their threat models might not match.

I completely agree that current_password should be write-only, I don't
see that s a problem.

> Furthermore - what's to say multiple authentication schemes might not be
> simultaneously supported and this needs to be expressed?  This can be a difference
> between OEM implementations.

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.

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.

>> This would mean that existing userspace software can not work with
>> systems using the new "hotp" password atrributes, but that is
>> unavoidable.
> 
>>
>> I think that the current generic password flow will work well
>> for other vendors too, they may need to not cache it in the
>> kernel (instead sending it directly to the firmware once), but the basic
>> concept of having to write the plain-text bios Admin password before
>> being able to change protected settings seems like it is something which
>> matches how most current BIOS-es work.  And needing a way to re-lock the
>> settings also sounds like something which will be pretty common for most
>> implementations.
>>
> 
> OK so here is another place that I think vendors might have a different
> implementation.  When you have a BIOS admin password set, Dell requires that
> password to interact with any of these attributes.  Another vendor might
> only require it only for certain attributes they deemed protected.
> 
> So again, Dell's flow might not scale to everyone else.
> 
> I do acknowledge this might be mitigatable by adding a sysfs file to every
> attribute for Dell's implementation "is_authentication_required" that is always
> 1 when admin password required and in another implementation an OEM might choose
> to set that on a case by case basis.

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.


>>>> We could even do something like we have for .desktop files
>>>> fields, where we add something to the sysfs ABI documentation
>>>> that vendors may add vendor specific types prefixed with X-<vendorname>.
>>>>
>>>> So all in all I believe that we can make using the proposed sysfs ABI
>>>> a generic one work, and that this will be worth it to avoid having the
>>>> issue of eventually having both a couple of vendor specific APIs +
>>>> one grant unified generic API replacing those vendor-APIs
>>>> (where we can never drop the vendor specific APIs because of
>>>> backward compat. guarantees).
>>>
>>> I'm personally leaning on the right place to have a vendor agnostic view is
>> "outside"
>>> of the kernel in an userland library.  All the vendor drivers that change
>> settings can
>>> behave very similarly for the most part, but differences between vendor
>> implementations
>>> can be better expressed there.
>>
>> We have tried that before in several cases (that I'm aware of) and this never
>> works out. Esp. not when the basic kernel interface is reasonable sane,
>> then a lot of people / projects avoid the lib and just poke the kernel API
>> directly. We have seen this e.g. with hwmon-class devices and with v4l devices
>> and with backlight-class devices. Since I've seen this happen 3 times already
>> I'm not a big believer in adding a userspace library to hide the vendor
>> differences.
>>
>> Regards,
>>
>> Hans
> 
> Another area that comes to mind is Dell's value_modifier and modifier rules.  This
> dependency logic is handled and expressed by the firmware.  You'll notice the Dell
> driver only displays the information that came out of the firmware in sysfs and doesn't
> do any processing in driver.
> 
> So by using Dell's format, another vendor's driver will need to follow Dell's
> formatting and rule validation/generation which their firmware might not support and
> they will be forced to implement Dell's schema in their kernel driver.

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.

> Lastly I want to caution that individual firmware items with the same name might have
> a different meaning across vendors.  Here is my hypothetical example:
> 
> Dell has an attribute called "Camera"  With V3 it populates under:
> /sys/devices/platform/dell-wmi-sysman/attributes/Camera
> 
> The description sysfs for it reads as "Enable Camera" and it's possible values are
> "Disabled;Enabled;".  For Dell this is pretty obviously it turns on and off the camera
> functionality.
> 
> For another vendor they might actually not offer to enable/disable the camera but instead
> To enable the control of an electromagnetic camera shutter from such an attribute.
> Their attribute could still be called "Camera" but the description might read as
> "Enable camera shutter control".  For them it would still read as "Disabled;Enabled;"
> for possible values but have a completely different meaning!
> 
> There is no standard for this, and again userspace will need to basically look at
> the directory and structure to figure out what the meaning actually is.

I can envision similar issues popping up between different generations / models
of Dell hardware even. 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 purpose behind having a unified userspace ABI is to e.g. allow configuring
firmware settings for a fleet of machines from:

https://wiki.gnome.org/Projects/FleetCommander

Using a generic plugin which works across different vendors.

And maybe have a simple vendor-agnostic pygtk3 UI which allows users to
poke at things, even if they have to figure out in which order they need
to change things in some cases (which again is actually not that
different from the current builtin firmware setup utility experience
for a lot of vendors).

I guess a could way to look at the generic sysfs firmware attributes
class I'm proposing is looking at it as a lowest common denominator
solution. With the addition of vendor specific extensions so that
vendors (e.g. Dell) are not limited to only offering functionality
offered by the generic, shared ABI. Does that make sense ?

Regards,

Hans

Powered by blists - more mailing lists