Hi,
On 3/14/22 16:46, Mark Pearson wrote:
> Thanks for the review Hans
>
> On 2022-03-14 11:15, Hans de Goede wrote:
>> Hi,
>>
>> On 3/12/22 01:04, Mark Pearson wrote:
>>> Implementation of certificate authentication feature for Lenovo
>>> platforms. This allows for signed updates of BIOS settings.
>>>
>>> Functionality supported:
>>> - Cert support available check. At initialisation check if BIOS
>>> supports certification authentication and if a certificate is
>>> installed. Enable the sysfs nodes appropriately
>>> - certificate and signature authentication attributes to enable
>>> a user to install, update and delete a certificate using signed
>>> signatures
>>> - certificate_thumbprint to confirm installed certificate details
>>> - support to go from certificate to password based authentication
>>> - set_signature and save_signature attributes needed for setting
>>> BIOS attributes using certificate authentication
>>>
>>> Tested on X1 Carbon 10 with special trial BIOS. This feature is not
>>> generally available yet but will be released later this year.
>>>
>>> Note, I also cleaned up the formating of the GUIDs when I was adding
>>> the new defines. Hope that's OK to combine in this commit.
>>>
>>> Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx>
>>> ---
>>> drivers/platform/x86/think-lmi.c | 560 ++++++++++++++++++++++++++-----
>>> drivers/platform/x86/think-lmi.h | 7 +
>>> 2 files changed, 475 insertions(+), 92 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index 0b73e16cccea..54ce71f97c37 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -16,6 +16,7 @@
>
> <snip>
>>>
>>> +static ssize_t cert_thumbprint(char *buf, const char *arg, int count)
>>> +{
>>> + const struct acpi_buffer input = { strlen(arg), (char *)arg };
>>> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>>> + const union acpi_object *obj;
>>> + acpi_status status;
>>> +
>>> + output.length = ACPI_ALLOCATE_BUFFER;
>>> + output.pointer = NULL;
>>
>> This initialization of output is redundant, it is already initialized
>> when it is declared.
> Ack
>
> <snip>
>>> +
>>> +static ssize_t certificate_store(struct kobject *kobj,
>>> + struct kobj_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> + char *auth_str, *new_cert, *p;
>>> + int ret;
>>> +
>>> + if (!capable(CAP_SYS_ADMIN))
>>> + return -EPERM;
>>> +
>>> + if (!tlmi_priv.certificate_support)
>>> + return -EOPNOTSUPP;
>>> +
>>> + new_cert = kstrdup(buf, GFP_KERNEL);
>>> + if (!new_cert)
>>> + return -ENOMEM;
>>> + /* Strip out CR if one is present */
>>> + p = strchrnul(new_cert, '\n');
>>> + *p = '\0';
>>> +
>>> + /* If empty then clear installed certificate */
>>> + if (new_cert[0] == '\0') { /* Clear installed certificate */
>>
>> You don't need new_cert anymore here, so do:
>>
>> kfree(new_cert);
>>
>> here.
>>
>>> + /* Check that signature is set */
>>> + if (!setting->signature || !setting->signature[0]) {
>>> + kfree(new_cert);
>>
>> and drop it here,
>>
>>> + return -EACCES;
>>> + }
>>> + /* Format: 'serial#, signature' */
>>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>>> + dmi_get_system_info(DMI_PRODUCT_SERIAL),
>>> + setting->signature);
>>> + if (!auth_str) {
>>> + kfree(new_cert);
>>
>> and here.
>>
>>> + return -ENOMEM;
>>> + }
>>> + ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
>>> + kfree(auth_str);
>>
>> Because you were missing a kfree(new_cert) here. Also you should free + clear
>> setting->certificate here. So this if block should end up like this:
>>
>> /* If empty then clear installed certificate */
>> if (new_cert[0] == '\0') { /* Clear installed certificate */
>> kfree(new_cert);
>>
>> /* Check that signature is set */
>> if (!setting->signature || !setting->signature[0])
>> return -EACCES;
>>
>> /* Format: 'serial#, signature' */
>> auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>> dmi_get_system_info(DMI_PRODUCT_SERIAL),
>> setting->signature);
>> if (!auth_str)
>> return -ENOMEM;
>>
>> ret = tlmi_simple_call(LENOVO_CLEAR_BIOS_CERT_GUID, auth_str);
>> kfree(auth_str);
>> if (ret)
>> return ret;
>>
>> kfree(setting->certificate);
>> setting->certificate = NULL;
>> return count;
>> }
>>
>>
> Agreed - that all makes sense. Will update.
>
>>> + }
>>> +
>>> + if (setting->cert_installed) {
>>> + /* Certificate is installed so this is an update */
>>> + if (!setting->signature || !setting->signature[0]) {
>>> + kfree(new_cert);
>>> + return -EACCES;
>>> + }
>>> + /* Format: 'Certificate,Signature' */
>>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>>> + new_cert, setting->signature);> + if (!auth_str) {
>>> + kfree(new_cert);
>>> + return -ENOMEM;
>>> + }
>>> + ret = tlmi_simple_call(LENOVO_UPDATE_BIOS_CERT_GUID, auth_str);
>>> + kfree(auth_str);
>>> + } else {
>>> + /* This is a fresh install */
>>> + if (!setting->valid || !setting->password[0]) {
>>> + kfree(new_cert);
>>> + return -EACCES;
>>> + }
>>> + /* Format: 'Certificate,Admin-password' */
>>> + auth_str = kasprintf(GFP_KERNEL, "%s,%s",
>>> + new_cert, setting->password);
>>> + if (!auth_str) {
>>> + kfree(new_cert);
>>> + return -ENOMEM;
>>> + }
>>> + ret = tlmi_simple_call(LENOVO_SET_BIOS_CERT_GUID, auth_str);
>>> + kfree(auth_str);
>>> + }
>>> +
>>> + /* If successful update stored certificate */
>>> + if (!ret) {
>>> + kfree(setting->certificate);
>>> + setting->certificate = new_cert;
>>> + } else
>>> + kfree(new_cert);
>>> +
>>> + return ret ?: count;
>>
>> You have 2 "if (ret)" statements here (1 hidden in the return), please change this to:
>>
>> if (ret) {
>> kfree(new_cert);
>> return ret;
>> }
>>
>> kfree(setting->certificate);
>> setting->certificate = new_cert;
>> return count;
>>
> Will do
>
>>
>>> +}
>>> +
>>> +static ssize_t certificate_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> +
>>
>> setting->certificate may be NULL here, you need to check for that and in
>> that case only emit a "\n" I guess.
> Ack.
>
>>
>>> + return sysfs_emit(buf, "%s\n", setting->certificate);
>>> +}
>>> +
>>> +static struct kobj_attribute auth_certificate = __ATTR_RW(certificate);
>>> +
>>> +static ssize_t signature_store(struct kobject *kobj,
>>> + struct kobj_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> + char *new_signature, *p;
>>> + int ret;
>>> +
>>> + if (!capable(CAP_SYS_ADMIN))
>>> + return -EPERM;
>>> +
>>> + if (!tlmi_priv.certificate_support)
>>> + return -EOPNOTSUPP;
>>> +
>>> + new_signature = kstrdup(buf, GFP_KERNEL);
>>> + if (!new_signature)
>>> + return -ENOMEM;
>>> +
>>> + /* Strip out CR if one is present */
>>> + p = strchrnul(new_signature, '\n');
>>> + *p = '\0';
>>
>> Idea for a follow-up clean-up patch: this pattern of kstrdup user-argument
>> (buf) + strip '\n' is repeated all over the driver, maybe add a little helper
>> for this?
>>
> Yes - that would make sense. Will do.
>
>>> +
>>> + /* Free any previous signature */
>>> + kfree(setting->signature);
>>> + setting->signature = new_signature;
>>> +
>>> + return ret ?: count;
>>> +}
>>> +
>>> +static ssize_t signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>>> +
>>
>> setting->signature can be NULL here.
>
> Ack
>
> <snip>
>
>>> +
>>> +static ssize_t set_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + return sysfs_emit(buf, "%s\n", tlmi_priv.set_signature);
>>> +}
>>> +
>>> +static ssize_t save_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>>> + char *buf)
>>> +{
>>> + return sysfs_emit(buf, "%s\n", tlmi_priv.save_signature);
>>> +}
>>> +
>>> +static struct kobj_attribute attr_set_signature = __ATTR_RW(set_signature);
>>> +static struct kobj_attribute attr_save_signature = __ATTR_RW(save_signature);
>>
>> <note I missed this while reviewing the documentation patch...>
>>
>> Why not just use the /sys/class/firmware-attributes/thinklmi/authentication/Admin/signature
>> value here ?
>>
>> /sys/class/firmware-attributes/thinklmi/authentication/Admin/current_password is
>> what is used for password based setting of fw-attributes as well as for changing
>> the password; and
>>
>> /sys/class/firmware-attributes/thinklmi/authentication/Admin/signature is set
>> for changing the certificate, so it would be much more consitent to also use
>> that for setting attributes when using certificate based auth?
>>
>> Can / will the set and save signature ever be different from one another ? If yes
>> then I guess we may need 2 attributes for this, I guess maybe just signature +
>> save_signature ? Either way IMHO these 2 attributes / the 1 extra attribute
>> for a separate save-signature really belongs under
>> /sys/class/firmware-attributes/thinklmi/authentication/Admin/ IMHO and
>> not under /sys/class/firmware-attributes/thinklmi/attributes/
>>
>> What do you think about moving these there ?
>>
> I have no issues with moving them. I had originally intended to have
> them in auth but as I needed two signatures (the set and save are
> different) I decided to make it explicit and to keep them in the same
> place as the attribute being modified. But I can see it making sense to
> just keep those under Authentication instead.
>
> I'll update and get rid of set_signature and move save_signature.
Sounds good, thank you.
> Many thanks for the review
You're welcome.
Regards,
Hans
