Re: [External] Re: [PATCH v3 2/2] platform/x86: think-lmi: Certificate authentication support

From: Mark Pearson <markpearson@lenovo.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: <markgross@kernel.org>, <platform-driver-x86@vger.kernel.org>
Subject: Re: [External] Re: [PATCH v3 2/2] platform/x86: think-lmi: Certificate authentication support
Date: Sun, 20 Mar 2022 20:14:48 -0400	[thread overview]
Message-ID: <4a7138ee-0bca-2a9d-5589-6aa278cbf0f3@lenovo.com> (raw)
In-Reply-To: <cb22c2f8-e289-a7a2-f51d-f4d0056814e2@redhat.com>

Thanks Hans

On 2022-03-18 07:37, Hans de Goede wrote:
> Hi,
> 
> On 3/17/22 22:40, Mark Pearson wrote:
<anip>
>> +	pr_info("Simple_call %s : %s\n", guid, auth_str);
> 
> This appears to be a left-over debug pr_info, I've dropped
> this while merging this.
> 
Ouch - my bad. Thanks for catching that.

<snip>
>> +
>> +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;
>> +	int ret;
> 
> ret is never set in this function, but ...
> 
>> +
>> +	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->signature);
>> +	setting->signature = new_signature;
>> +
>> +	return ret ?: count;
> 
> It is checked here and for some reason the compiler does
> not warn about this.
> 
> I've changed the return to just:
> 
> 	return count;
> 
> And dropped the ret declaration while merging this.
> 
Ack and thanks.
> 
>> +}
>> +
>> +static struct kobj_attribute auth_signature = __ATTR_WO(signature);
>> +
>> +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;
> 
> Idem.
> 
>> +
>> +	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;
> 
> Idem.
> 
> 
> With these small changes I've added this to my review-hans
> branch now. I hope the kernel-build-robot will give it
> a pass there soon and then I'll push it to my for-next branch.
> 
> Note while merging this I did notice one thing which
> I would like you to address in a follow-up patch
> (based on the version currently in review-hans).
> 
> tlmi_priv.pwd_admin->certificate gets set but is never used,
> please drop the certificate member from struct tlmi_pwd_setting
> and also all the code setting it.
> 
> This will also allow you to move the kfree(new_cert)
> up a bit inside certificate_store() to directly
> after the if (setting->cert_installed) ... else ...
> like this:
> 
> 	if (setting->cert_installed) {
> 		...
> 		auth_str = kasprintf(...)
> 	} else {
> 		...
> 		auth_str = kasprintf(...)
> 	}
> 	kfree(new_cert);
> 
> Avoiding the need to have a kfree(new_cert) in
> all the error-returns below this block.
> 
> And the end of this function can then be
> simplified to just:
> 
> 	return ret ?: count;
> 
Agreed - I'll update this.

Mark