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
