Message-ID: <5847917c-2c34-5d74-b5db-f33bb8fc9e13@redhat.com>
Date:   Thu, 17 Sep 2020 12:11:57 +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/14/20 6:06 PM, Limonciello, Mario wrote:
>> So my thinking here is as follows:
>>
>> 1. AFAIK other vendors may want to do something similar in the near future
>> 2. The interface you (Dell) have come up with looks pretty generic / complete
>> to me

<snip>

>>> Dell sets precedent here by having the first driver.
>>
>> Right and normally I may have wanted to wait until a second vendor implements
>> a similar mechanism under Linux so that we can find common ground between the
>> 2 implementations for the generic userspace API for this.
>>
> 
> I think in terms of the basic sysfs files and their contents, a generic API makes
> fine sense, but I'm hung specifically up on the flow when the firmware interface is
> locked down.

Ok.

>> The problem with that approach is that because we do not break userspace,
>> we then get to carry the "temporary" vendor-specific implementation of the
>> userspace API around for ever, since it may very well have existing users
>> before we replace it with the generic API.
>>
>> This scenario would mean that after some point in time (when the generic API
>> gets
>> added to the kernel) Dell needs to support 2 userspace APIs the one which is
>> being introduced by this patch + the generic one going forward.
>>
>> Since to me the current API/ABI Dell is proposing is pretty generic I'm
>> trying to avoid this having 2 maintain 2 different userspace APIs problem
>> by making this the generic API from the get go.
> 
> I'm worried that we actually end up in a situation that the "generic" one supports
> these basic features.  This is very similar to the Dell one, but misses certain
> enhancements that are not in the generic one so you have to use the Dell one to get
> those features.  And then how do you know which one to select from the kernel config?
> 
> It gets messy quickly.

If there are 2 interfaces then yes it can get messy, but having 2 interfaces is
exactly what I'm trying to avoid here.

>>> 2) Dell has some extensions planned for other authentication mechanisms than
>> password.
>>> That is *definitely* going to be Dell specific, so should it be done in this
>> vendor
>>> agnostic directory?
>>
>> Well each property:
>>
>> /sys/class/firmware-properties/dell-bios/<property-name>
>>
>> Will have a type attribute:
>>
>> /sys/class/firmware-properties/dell-bios/<property-name>/type
>>
>> You can use dell-special-auth-mechanism as type for this and
>> then it is clear it is dell specific. As mentioned above I
>> fully expect new types to get added over this and userspace tools
>> will be expected to just skip properties with unknown types
>> (possibly with a warning).
> 
> So I think the nuance that is missed here is the actual flow for interacting with
> an attribute when password security is enabled in today's patch set (both v1 and v2).
> 
> Userspace would perform like this:
> 1) Check "is_password_set" attribute to determine if admin password required
> 2) If yes write password into the current_password attribute (location changed in 2 patches)
> 3) write new attribute value(s)
> 4) If necessary clear current_password attribute
> 
> This works like a "session" today with admin password.  So if you have a generic interface
> representing things as attributes you need to also have a generic attribute indicating
> authentication required.  That would mean ALL attributes need to have a "authentication_required"
> type of attribute.
> 
> And then that comes back to the point that authentication flow is definitely not generic.
> Dell requires you to write the password in every WMI call today, but the sysfs interface actually
> behaves like a session and caches the password in memory for the next call.
> 
> As a completely hypothetical idea what if another vendor also supports an admin password but decides for
> their threat model it's actually a password hashed appended with a nonce and hashed and hence
> needs to be set every time from sysfs?
> 
> Their flow might look something like this:
> 1) Check auth_required attribute
> 2) Write hash(password|nonce) to current_password
> 3) Write attribute
> 4) Write hash(password|nonce) to current_password
> 5) Not necessary to clear current_password
> 
> 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. 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.

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.

>> 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

Powered by blists - more mailing lists