Message-ID: <DM6PR19MB26368BB2B8C4D7CE58DF7C31FA230@DM6PR19MB2636.namprd19.prod.outlook.com>
Date:   Mon, 14 Sep 2020 16:06:15 +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>,
        "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

> 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
> 
> > Would those possible options
> > be hardcoded in their kernel driver?
> 
> Maybe, so the firmware implementation of an enum type, can take 2 forms:
> a) An integer in the range of 0-# where 0-# is like the integer value backing
> an enum in C
> b) Reading the current value as a string and when writing only a value from a
> fixed list of strings is valid
> 
> Now in both cases, either not knowing what the numbers actually mean, or not
> knowing
> which values are valid for writing would make the interface pretty hard to
> use,
> close to useless. So yes in that case the driver may need to hardcode to
> values
> (assuming that scales for that vendor and they don't have a gazillion
> different
> enums).
> 
> Also note that sysfs attributes can be marked as optional, so we could mark
> things like possible enum-values, min/max/scalar_inc as optal right from the
> start.
> We could for now mark everything optional except for type, current_value and
> display-name. That should make it easy for other vendors implementations to
> adhere to / implement the API.
> 
> > 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.

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

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

Powered by blists - more mailing lists