Hi, On 3/21/22 19:06, Mark Pearson wrote: > Complete some clean-ups as reqested from the last review as follow-ups > - Remove certificate from structure as no need to store it any more > - Clean up return code handling > - Moved freeing of signature to before admin object released (issue > seen in testing when unloading module) > - Minor code flow improvements > > Signed-off-by: Mark Pearson <markpearson@xxxxxxxxxx> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/think-lmi.c | 45 +++++++++++--------------------- > drivers/platform/x86/think-lmi.h | 1 - > 2 files changed, 15 insertions(+), 31 deletions(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index 1817dd8d5d95..a01a92769c1a 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -740,16 +740,8 @@ static ssize_t certificate_store(struct kobject *kobj, > 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 */ > - strip_cr(new_cert); > - > /* If empty then clear installed certificate */ > - if (new_cert[0] == '\0') { /* Clear installed certificate */ > - kfree(new_cert); > - > + if ((buf[0] == '\0') || (buf[0] == '\n')) { /* Clear installed certificate */ > /* Check that signature is set */ > if (!setting->signature || !setting->signature[0]) > return -EACCES; > @@ -763,14 +755,16 @@ static ssize_t certificate_store(struct kobject *kobj, > > 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; > + return ret ?: count; > } > > + new_cert = kstrdup(buf, GFP_KERNEL); > + if (!new_cert) > + return -ENOMEM; > + /* Strip out CR if one is present */ > + strip_cr(new_cert); > + > if (setting->cert_installed) { > /* Certificate is installed so this is an update */ > if (!setting->signature || !setting->signature[0]) { > @@ -792,21 +786,14 @@ static ssize_t certificate_store(struct kobject *kobj, > auth_str = kasprintf(GFP_KERNEL, "%s,%s", > new_cert, setting->password); > } > - if (!auth_str) { > - kfree(new_cert); > + kfree(new_cert); > + if (!auth_str) > return -ENOMEM; > - } > > ret = tlmi_simple_call(guid, auth_str); > kfree(auth_str); > - if (ret) { > - kfree(new_cert); > - return ret; > - } > > - kfree(setting->certificate); > - setting->certificate = new_cert; > - return count; > + return ret ?: count; > } > > static struct kobj_attribute auth_certificate = __ATTR_WO(certificate); > @@ -846,7 +833,6 @@ static ssize_t save_signature_store(struct kobject *kobj, > { > struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj); > char *new_signature; > - int ret; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > @@ -1195,6 +1181,10 @@ static void tlmi_release_attr(void) > > kset_unregister(tlmi_priv.attribute_kset); > > + /* Free up any saved signatures */ > + kfree(tlmi_priv.pwd_admin->signature); > + kfree(tlmi_priv.pwd_admin->save_signature); > + > /* Authentication structures */ > sysfs_remove_group(&tlmi_priv.pwd_admin->kobj, &auth_attr_group); > kobject_put(&tlmi_priv.pwd_admin->kobj); > @@ -1211,11 +1201,6 @@ static void tlmi_release_attr(void) > } > > kset_unregister(tlmi_priv.authentication_kset); > - > - /* Free up any saved certificates/signatures */ > - kfree(tlmi_priv.pwd_admin->certificate); > - kfree(tlmi_priv.pwd_admin->signature); > - kfree(tlmi_priv.pwd_admin->save_signature); > } > > static int tlmi_sysfs_init(void) > diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h > index 4f69df6eed07..4daba6151cd6 100644 > --- a/drivers/platform/x86/think-lmi.h > +++ b/drivers/platform/x86/think-lmi.h > @@ -63,7 +63,6 @@ struct tlmi_pwd_setting { > int index; /*Used for HDD and NVME auth */ > enum level_option level; > bool cert_installed; > - char *certificate; > char *signature; > char *save_signature; > };
