| 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: <05eb6019-5ed5-463a-b556-27792ea43568@app.fastmail.com>
Date: Mon, 29 Dec 2025 22:07:26 -0500
From: "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
"Benjamin Philip" <benjamin.philip495@...il.com>
Cc:
"platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
"Derek J . Clark" <derekjohn.clark@...il.com>,
"Hans de Goede" <hansg@...nel.org>
Subject: Re: [PATCH 4/5] platform/x86: think-lmi: fix column limit overflow
On Mon, Dec 29, 2025, at 12:32 PM, Ilpo Järvinen wrote:
> On Mon, 29 Dec 2025, Benjamin Philip wrote:
>> Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com> writes:
>> > On Wed, 24 Dec 2025, Benjamin Philip wrote:
>> >> "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?
>> >
>> > Hi,
>> >
>> > The general theme in this driver seems to be that tlmi_priv.pwd_admin
>> > usually causes long lines so introducing a local variable for it in such
>> > functions would certainly help.
>> >
>> > This probably came to be when pwd_admin was migrated over into tlmi_priv
>> > which certainly was correct place for it, but case with the cost of adding
>> > into the line lengths.
>> >
>>
>> In this case, would you prefer that a local variable for pwd_admin be
>> introduced instead of variables for values in the long lines?
>
> Mark who's the maintainer seems to be against it, so I don't want to step
> too strongly on his lot in this. It's merely my opinion how we normally
> handle cases like this (I probably didn't express that clearly enough).
>
I don't have a strong opinion :) If it's business as usual, then it's fine and we can include it.
>From my perspective, it would be useful to know if it's been tested on real HW, and (if so) which platforms. Unfortunately Thinkpad, ThinkCenter and ThinkStation all use this module and all have their own little quirks.
I'll do some testing here (soonish...) to confirm they're all still sane after (they should be) and throw in a tested-by tag.
Mark
Powered by blists - more mailing lists