Message-ID: <df6e2dc8-0906-0e1f-4aef-47f929b98647@redhat.com>
Date:   Fri, 25 Sep 2020 21:40:29 +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>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        mark gross <mgross@...ux.intel.com>,
        "Ksr, Prasanth" <Prasanth.Ksr@...l.com>,
        Mark Pearson <mpearson@...ovo.com>
Subject: Re: [PATCH v4] Introduce support for Systems Management Driver over
 WMI for Dell Systems

Hi,

On 9/25/20 8:37 PM, Limonciello, Mario wrote:

<snip>

>>> So a user would see the different authentication mechanisms available
>>> by looking At the contents of /sys/class/firmware-
>> attributes/*/authentication
>>> and if they don't understand it's purpose they look at the type sysfs file.
>>
>> But one role can still have multiple mechanisms, so for Dell in the future
>> we could have say:
>>
>> /sys/class/firmware-attributes/dell/authentication/Admin-password
>> /sys/class/firmware-attributes/dell/authentication/Admin-hotp
>> /sys/class/firmware-attributes/dell/authentication/System-password
>>
>> So although I'm fine with taking the role_name directly from WMI
>> (combined with a roll attribute with standardized values) I think
>> we still need to postfix a -password to it now, to allow room
>> for adding say a -hotp mechanism for the same role_name in the
>> future ?
> 
> Could this be captured in the role attribute instead perhaps?  So the role
> attributes values could hypothetically be:
> bios-admin-password
> power-on-password
> 
> And if HOTP is added some day these could be added:
> bios-admin-hotp
> power-on-hotp

I would rather have the auth-mechanism-type in a separate mechanism sysfs-file
which could then contain e.g. "password" or "hotp".

But that does not solve the bit which I'm worried about,
what I'm worried about is a future scenario where there are multiple
auth mechanisms for the Admin role, and assuming we use one dir per
auth-mechanism then we would need 2 Admin sub-dirs which is not
allowed of course.

I assume that that is what the symlink suggestion you did:

 > I was going to suggest that if necessary a compatible way to add these would
 > be symlinks.  So if the directory right now was Admin and later was had
 > split to something like AdminHotp/AdminPassword
 > but wanted to discriminate between the old Admin it could be Admin->AdminPassword
 > or vice versa.

Is meant to address. So for now we go with just Admin and then if
the 2 auth-mechanisms for Admin at the same time scenario happens
add the "-<mechanism>" suffix to the directory name at that point.
Or I guess we could even add a ".%d" suffix for duplicates, since
the mechanism info would already be in the separate mechanism
sysfs file.  Then we can also avoid the symlink since we can just
leave out the .%d suffix when the integer for %d is 0, there is
precedence for that.

So TL;DR: yes we could put the mechanism in a sysfs file too,
in that case I would prefer to go with a separate file though,
rather then concatenating it to the role and storing it in the
role sysfs file.

If we have both the role and mechanism (hardcoded to "password"
for now) as sysfs-files in say a:

/sys/class/firmware-attributes/dell/authentication/Admin

Dir then indeed we will not need the "-password" suffix. And if
we get more then one auth-mechanism for say the "Admin" role then
we can just name the second subdir Admin.1, etc.

So compared to the current sysfs API from the v4 patch that would
mean adding the role ("admin", or "power-on") and mechanism
(always "password" for now) sysfs files and otherwise no changes.

Oh and can we rename the "is_authentication_set" sysfs file to "is_enabled"
please?  Being set is well defined for a password, but not so much for possible
other authentication-mechanisms.

<snip>

>>>> ###
>>>>
>>>> So as with the actual firmware-attributes I have also been comparing the
>>>> authentication
>>>> bits for the Dell case with the community thinkpad_wmi code. And again
>> things
>>>> are a pretty
>>>> good match already, including being able to query a minimum and maximin
>>>> password length.
>>>>
>>>> The only thing which is different, which I think would be good to add now,
>> is
>>>> a password_encoding sysfs-attribute. The Lenovo password interface supports
>>>> 2 encodings, "ascii" and "scancodes". Although I wonder if scancodes still
>>>> works on modern UEFI based platforms.
>>>>
>>>> Since the Dell password code uses UTF8 to UTF16 translation routines, I
>> guess
>>>> the encoding for the Dell password is UTF8 (at the sysfs level). So I would
>>>> like to propose
>>>> an extra password-authentication attribute like:
>>>>
>>>> 	password_encoding:  A file that can be read to obtain the encoding used
>>>> by
>>>> 			    the current_password and new_password attributes.
>>>> 			    The value returned should be one of: "utf8", "ascii".
>>>> 			    In some cases this may return a vendor-specific encoding
>>>> 			    like, e.g. "lenovo-scancodes".
>>>>
>>>> And for the Dell driver this would just be hardcoded to utf8.
>>>
>>> I don't really believe that another vendor's implementation would be likely
>> to
>>> use scan codes for the input into the WMI method.
>>
>> I did not make that example up, Lenovo really as a scan-codes encoding for
>> their password authentication mechanism, see:
>>
>> https://download.lenovo.com/pccbbs/mobiles_pdf/kbl-r_deploy_01.pdf
>>

<snip>

> The documentation you linked doesn't seem to indicate when to use scancodes or
> ASCII to me, so I can't draw any conclusions if certain models support one or
> the other.
> 
> I would suggest yes please don't support scancodes from the sysfs perspective for
> another vendor's implementation.  We should probably keep sysfs as utf8 and let
> any conversions be hidden in the kernel driver if necessary.

Doing the conversions in kernel is not really ideal, the kernels i18n support
is very limited. But yes for utf8->ascii and utf8->utf16 we can do the conversion
in kernel.

>> Even then there still is the unicode (utf8
>> in sysfs, utf16 at the WMI level for Dell) vs ascii issue and it would be nice
>> if a UI for this could give an error when the user tries to use non ascii
>> chars in a password for a vendor implementation...
> 
> I think that the kernel driver can certainly parse and provide -EINVAL in this
> context.

True, but the advantage of having userspace know it needs to be ascii is
that it can provide a much more sensible error message to the user when
the user tries to use non ascii in the password.

So I think that in the non utf8 case it would still be good to have
a password_encoding file. With that said, as discussed this sysfs-file
will be optional too, so for the "Systems Management Driver over WMI for Dell
Systems" we can just leave it out and then revisit when we merge a driver
which does not support utf8 for passwords.

>>> I would much prefer that this attribute only be added if it's actually
>> deemed
>>> necessary.  Or to my point that all attributes can be considered optional,
>> Dell's
>>> implementation would just not offer it.
>>
>> I guess that would work (Dell not offering it). Which makes me realize that
>> we should specify somewhere in the doc that all sysfs files which contain
>> a string value, the encoding is UTF8 unless explicitly specified otherwise.
>> (for that specific sysfs-attribute).
> 
> Yeah.  I guess the very top where we will modify to mention that all attributes
> are optional we can also mention that the encoding is UTF8 unless otherwise
> specified.

Ack.

Regards,

Hans

Powered by blists - more mailing lists