| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMEXYWf9TF=O_vZTSsc4T7Go4dVw=xw5zZfGWKhGWu8d4eHwrg@mail.gmail.com>
Date: Wed, 24 Dec 2025 11:33:19 +0000
From: Benjamin Philip <benjamin.philip495@...il.com>
To: Mark Pearson <mpearson-lenovo@...ebb.ca>,
"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>, linux-kernel@...r.kernel.org
Cc: "Derek J . Clark" <derekjohn.clark@...il.com>, Hans de Goede <hansg@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow
"Mark Pearson" <mpearson-lenovo@...ebb.ca> writes:
> On Tue, Dec 23, 2025, at 2:24 PM, Benjamin Philip wrote:
>> This commit handles some column limit overflows (that occur after fixing
>> their alignment), i.e. the following check:
>>
>> CHECK: line length of ... exceeds 100 columns
>>
>> by defining a constant opt, and replacing the offending
>> expression with opt.
>>
>> Signed-off-by: Benjamin Philip <benjamin.philip495@...il.com>
>> ---
>> drivers/platform/x86/lenovo/think-lmi.c | 31 +++++++++++++++----------
>> 1 file changed, 19 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/platform/x86/lenovo/think-lmi.c
>> b/drivers/platform/x86/lenovo/think-lmi.c
>> index 1ada4d800383..07ba0d84720a 100644
>> --- a/drivers/platform/x86/lenovo/think-lmi.c
>> +++ b/drivers/platform/x86/lenovo/think-lmi.c
>> @@ -1083,12 +1083,13 @@ static ssize_t type_show(struct kobject *kobj,
>> struct kobj_attribute *attr,
>> }
>>
>> static ssize_t current_value_store(struct kobject *kobj,
>> - struct kobj_attribute *attr,
>> - const char *buf, size_t count)
>> + struct kobj_attribute *attr, const char *buf,
>> + size_t count)
>> {
>> struct tlmi_attr_setting *setting = to_tlmi_attr_setting(kobj);
>> char *set_str = NULL, *new_setting = NULL;
>> char *auth_str = NULL;
>> + const char *opt;
>> int ret;
>>
>> if (!tlmi_priv.can_set_bios_settings)
>> @@ -1163,10 +1164,11 @@ static ssize_t current_value_store(struct kobject *kobj,
>> ret = tlmi_save_bios_settings("");
>> } else { /* old non-opcode based authentication method (deprecated) */
>> if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> + opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> - tlmi_priv.pwd_admin->password,
>> - encoding_options[tlmi_priv.pwd_admin->encoding],
>> - tlmi_priv.pwd_admin->kbdlang);
>> + tlmi_priv.pwd_admin->password,
>> + opt,
>> + tlmi_priv.pwd_admin->kbdlang);
>> if (!auth_str) {
>> ret = -ENOMEM;
>> goto out;
>> @@ -1299,6 +1301,7 @@ static ssize_t save_settings_store(struct
>> kobject *kobj, struct kobj_attribute *
>> const char *buf, size_t count)
>> {
>> char *auth_str = NULL;
>> + const char *opt;
>> int ret = 0;
>> int cmd;
>>
>> @@ -1347,9 +1350,10 @@ static ssize_t save_settings_store(struct
>> kobject *kobj, struct kobj_attribute *
>> ret = tlmi_save_bios_settings("");
>> } else { /* old non-opcode based authentication method (deprecated) */
>> if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> + opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> tlmi_priv.pwd_admin->password,
>> - encoding_options[tlmi_priv.pwd_admin->encoding],
>> + opt,
>> tlmi_priv.pwd_admin->kbdlang);
>> if (!auth_str) {
>> ret = -ENOMEM;
>> @@ -1381,11 +1385,13 @@ static ssize_t save_settings_store(struct
>> kobject *kobj, struct kobj_attribute *
>> static struct kobj_attribute save_settings = __ATTR_RW(save_settings);
>>
>> /* ---- Debug
>> interface---------------------------------------------------------
>> */
>> -static ssize_t debug_cmd_store(struct kobject *kobj, struct
>> kobj_attribute *attr,
>> - const char *buf, size_t count)
>> +static ssize_t debug_cmd_store(struct kobject *kobj,
>> + struct kobj_attribute *attr, const char *buf,
>> + size_t count)
>> {
>> char *set_str = NULL, *new_setting = NULL;
>> char *auth_str = NULL;
>> + const char *opt;
>> int ret;
>>
>> if (!tlmi_priv.can_debug_cmd)
>> @@ -1397,10 +1403,11 @@ static ssize_t debug_cmd_store(struct kobject
>> *kobj, struct kobj_attribute *attr
>> return -ENOMEM;
>>
>> if (tlmi_priv.pwd_admin->pwd_enabled && tlmi_priv.pwd_admin->password[0]) {
>> + opt = encoding_options[tlmi_priv.pwd_admin->encoding];
>> auth_str = kasprintf(GFP_KERNEL, "%s,%s,%s;",
>> - tlmi_priv.pwd_admin->password,
>> - encoding_options[tlmi_priv.pwd_admin->encoding],
>> - tlmi_priv.pwd_admin->kbdlang);
>> + tlmi_priv.pwd_admin->password,
>> + opt,
>> + tlmi_priv.pwd_admin->kbdlang);
>> if (!auth_str) {
>> ret = -ENOMEM;
>> goto out;
>> @@ -1775,7 +1782,7 @@ static int tlmi_analyze(struct wmi_device *wdev)
>> ffs(tlmi_priv.pwdcfg.ext.hdd_user_password) - 1;
>> }
>> if (tlmi_priv.pwdcfg.ext.nvme_user_password ||
>> - tlmi_priv.pwdcfg.ext.nvme_master_password) {
>> + tlmi_priv.pwdcfg.ext.nvme_master_password) {
>> tlmi_priv.pwd_nvme->pwd_enabled = true;
>> if (tlmi_priv.pwdcfg.ext.nvme_master_password)
>> tlmi_priv.pwd_nvme->index =
>> --
>> 2.52.0
>
> I'll defer to the pdx86 reviewers for this set of changes.
>
> This seems to me to make things more complicated than needed, purely to address a 100 column limit. I personally don't like the change.
>
> Nothing wrong with the code, and if more experienced maintainers prefer it, I'm happy to defer to them. Otherwise it seems to me noise for the sake of noise I'm afraid
>
> Mark
An alternative could be to set this a constant pwd_admin to
tlmi_priv.pwd_admin. There are 13 other references to
tlmi_priv.pwd_admin in one function alone, so maybe it might be a more
meaningful improvement?
Then again, the question arises why we aren't following the same pattern
for all the other heavily used fields under tlmi_priv. Adding more
constants seems to be wrong way forward.
Another option would be to move repeated functionality into other
functions (all the column limit violations seem to be identical and
involve encoding_options and kasprintf in the same way), but a refactor
of this nature seemed *way* beyond the scope of a simple code syle clean
up.
Thus, assigning the required value to a small constant seemed to be the
best immediate solution. I can see why you feel it adds complexity.
Typically in a dynamic language (or even in a declare-as-you-need code
style in C) this is a trivial change, whereas in a top-of-block style
this does seem to add some cruft.
Perhaps we should drop this patch for now? Nevertheless, I think the
column limit violations, long functions, and repeated lines are a sign
that some refactoring is in order.
Thoughts?
-- bp
Powered by blists - more mailing lists