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.
Many thanks for the review
Mark
