Thanks Hans
On 2022-03-17 07:21, Hans de Goede wrote:
> Hi Mark,
>
> On 3/15/22 20:56, 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
>> - 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>
>> ---
>> Changes in v2:
>> - Combined set_signature with signature and moved save_signature under
>> the authorisation folder
>> - utility function to strip CR from string
>> - Clean up code as recommended from review
>
> Thanks this is looking better now, I still have a few small
> remarks, see below.
>
>>
>> drivers/platform/x86/think-lmi.c | 560 +++++++++++++++++++++++++------
>> drivers/platform/x86/think-lmi.h | 5 +
>> 2 files changed, 461 insertions(+), 104 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index 0b73e16cccea..1db34a6c94c2 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -16,6 +16,7 @@
<snip>
>> +
>> + 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);
>
> The block starting here.
>
>> + if (!auth_str) {
>> + kfree(new_cert);
>> + return -ENOMEM;
>> + }
>> + ret = tlmi_simple_call(LENOVO_UPDATE_BIOS_CERT_GUID, auth_str);
>> + kfree(auth_str);
>
> And ending here, is identical in the if and else paths with the
> exception of the guid.
>
>> + } 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 */
>
> This comment looks weird with it being placed above the error check + return and
> the code really is self explanatory, please drop it.
>
>> + if (ret) {
>> + kfree(new_cert);
>> + return ret;
>> + }
>> +
>
> Together with the if/else refactoring we then get:
>
> if (setting->cert_installed) {
> /* Certificate is installed so this is an update */
> if (!setting->signature || !setting->signature[0]) {
> kfree(new_cert);
> return -EACCES;
> }
> guid = LENOVO_UPDATE_BIOS_CERT_GUID;
> /* Format: 'Certificate,Signature' */
> auth_str = kasprintf(GFP_KERNEL, "%s,%s",
> new_cert, setting->signature);
> } else {
> /* This is a fresh install */
> if (!setting->valid || !setting->password[0]) {
> kfree(new_cert);
> return -EACCES;
> }
> guid = LENOVO_SET_BIOS_CERT_GUID;
> /* 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_UPDATE_BIOS_CERT_GUID, auth_str);
> kfree(auth_str);
> if (ret) {
> kfree(new_cert);
> return ret;
> }
>
Agreed - much nicer. I'll get that implemented
>> + kfree(setting->certificate);
>> + setting->certificate = new_cert;
>> + return count;
>> +}
>> +
<snip>
>> +
>> +static ssize_t signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>> +
>> + if (!setting->signature)
>> + return sysfs_emit(buf, "Not set\n");
>> +
>> + return sysfs_emit(buf, "%s\n", setting->signature);
>> +}
>> +
>> +static struct kobj_attribute auth_signature = __ATTR_RW(signature);
>
> So thinking about this more, having a show function at all is a bad
> idea, that will allow an attacker to potentially steal the signature.
>
> I guess that the signature is specific to the setting being changed,
> but this will still allow a replay attack, to restore a setting which
> an attacker has seen being changed in the past. E.g. the admin
> enables USB ports for some debugging and then disables them again,
> now an attacker who was able to read the signature file while the
> admin was enableing the USB ports might re-enable them later.
>
> So it would be best to just make this __ATR_WO and not have
> a show function at all, like we do for the password.
>
Fair enough. I'm good to remove it.
>
>
>> +
>> +static ssize_t save_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;
>> + 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 */
>> + strip_cr(new_signature);
>> +
>> + /* Free any previous signature */
>> + kfree(setting->save_signature);
>> + setting->save_signature = new_signature;
>> +
>> + return ret ?: count;
>> +}
>> +
>> +static ssize_t save_signature_show(struct kobject *kobj, struct kobj_attribute *attr,
>> + char *buf)
>> +{
>> + struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
>> +
>> + if (!setting->save_signature)
>> + return sysfs_emit(buf, "Not set\n");
>> +
>> + return sysfs_emit(buf, "%s\n", setting->save_signature);
>> +}
>> +
>> +static struct kobj_attribute auth_save_signature = __ATTR_RW(save_signature);
>
> idem, make this __ATTR_WO too.
ack
>
<snip>
>> @@ -896,8 +1230,15 @@ static void tlmi_release_attr(void)
>> sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &pending_reboot.attr);
>> if (tlmi_priv.can_debug_cmd && debug_support)
>> sysfs_remove_file(&tlmi_priv.attribute_kset->kobj, &debug_cmd.attr);
>> +
>> kset_unregister(tlmi_priv.attribute_kset);
>>
>> + if (tlmi_priv.certificate_support) {
>> + kfree(tlmi_priv.pwd_admin->certificate);
>> + kfree(tlmi_priv.pwd_admin->signature);
>> + kfree(tlmi_priv.pwd_admin->save_signature);
>> + }
>> +
>
> 1. These kfree() calls should be done only after removing the
> sysfs group, otherwise here is a race where the could still
> be accessed after being free-ed
>
> 2. kfree(NULL) is a no-op so the "if (tlmi_priv.certificate_support)"
> check is not necessary.
Ack - I'll fix these.
Thanks for the review
Mark
