Hi Andy, On 2021-05-27 10:16 a.m., Andy Shevchenko wrote: > On Wed, May 26, 2021 at 11:15 PM Mark Pearson <markpearson@xxxxxxxxxx> wrote: >> >> For Lenovo platforms that support a WMI interface to the BIOS add >> support, using the firmware-attributes class, to allow users to access >> and modify various BIOS related settings. > > Thanks for an update! My comments below. > > ... > >> +/* >> + * think-lmi.c - Think LMI BIOS configuration driver > > It's not the best idea to include the file name into the file. If the > file gets renamed (by whatever reason) often this either brings an > additional burden or simply forgotten (as from my experience). I > recommend to drop the file names from the source code. Good advice - I'll update > >> + * Copyright(C) 2019-2021 Lenovo >> + * >> + * Original code from Thinkpad-wmi project https://github.com/iksaif/thinkpad-wmi >> + * Copyright(C) 2017 Corentin Chary <corentin.chary@xxxxxxxxx> >> + * Distributed under the GPL-2.0 license >> + */ > > ... > >> +#include <linux/acpi.h> > > linux/errno.h ? > >> +#include <linux/fs.h> > > linux/string.h ? > linux/types.h ? > >> +#include <linux/wmi.h> >> +#include "firmware_attributes_class.h" >> +#include "think-lmi.h" I hadn't included those as they call get pulled in by linux/acpi.h - so aren't needed here. I take it best practice is to add them. I'd deliberately stripped down the list to the bare minimu previously but happy to reverse that if it's preferred. > > ... > >> + * Type: >> + * Method >> + * Arguments: >> + * "Item,Value,Password,Encoding,KbdLang;" >> + * Example: >> + * "WakeOnLAN,Disable,pswd,ascii,us;" > > Is 'pswd' here an example of the password? Hacker's language can make > it more visible, e.g. 'pa55w0rd'. > Same for other examples. Yes it is. I can update that :) > >> + */ > > ... > >> + /* >> + * Duplicated call required to match bios workaround for behavior > > bios -> BIOS Ack > >> + * seen when WMI accessed via scripting on other OS. >> + */ > > ... > >> + struct tlmi_pwd_setting *setting = container_of(kobj, struct tlmi_pwd_setting, kobj); > > Candidate to have something like > > #define to_tlmi_pwd_setting(obj) container_of(...) > > since it has appeared a few times. Ack > >> + return sysfs_emit(buf, "%d\n", setting->valid); >> +} > >> + > > Unneeded blank line. Same for other similar places. Shoot - I thought I'd cleaned a bunch of these up. I'll do another pass. > >> +static struct kobj_attribute auth_is_pass_set = __ATTR_RO(is_enabled); > > Hmm... We have already define_one_global_ro(). The problems with that > are the name and location. But perhaps you can copy that macro here > with the same name and at least we can see the common grounds to clean > up in the future. Another possibility is to comment that it can be > replaced with the above mentioned macro. Ideally would be to refactor > right now, but it's not anyhow crucial or required for this series, so > may be postponed. OK - I'll have a look. I'm not 100% sure I understand the issues, but hopefully it will become clearer once I play with it a bit > > ... > >> + int pwdlen; > > Strictly speaking it should be size_t. > >> + pwdlen = strlen(buf); >> + if (buf[pwdlen-1] == '\n') >> + pwdlen--; > > But the question is what will happen with the string like > 'pa55\nw0rd\n' (note all \n:s)? > See also below. I know there is discussion on this in future mails, so I'll leave this one for those > >> + /* pwdlen == 0 is allowed to clear the password */ >> + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) > > The ' != 0' part is redundant. Ack > >> + return -EINVAL; > >> + memcpy(setting->password, buf, pwdlen); >> + setting->password[pwdlen] = '\0'; > > I'm not sure why we can't use strscpy() like you did in the other function. I'm not sure either :) I'll update > >> + return count; > > ... > >> + /* Strip out CR if one is present, setting password won't work if it is present */ >> + strreplace(new_pwd, '\n', '\0'); > > Basically it will stop on the first one. See the strchrnul() trick below. Thanks for the strchrnul trick - happy to use that > >> + pwdlen = strlen(new_pwd); >> + /* pwdlen == 0 is allowed to clear the password */ >> + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) { > > No need for ' != 0'. Ack > >> + ret = -EINVAL; >> + goto out; >> + } > >> +} > > > ... > >> + int length; >> + >> + length = strlen(buf); >> + if (buf[length-1] == '\n') >> + length--; >> + >> + if (!length || (length >= TLMI_LANG_MAXLEN)) >> + return -EINVAL; >> + >> + memcpy(setting->kbdlang, buf, length); >> + setting->kbdlang[length] = '\0'; >> + return count; > > Similar comments as per above. Ack > > ... > >> + char *set_str = NULL, *new_setting = NULL; >> + char *auth_str = NULL; > > The rule of thumb is to avoid forward assignments on stack allocated > variables. It may decrease readability, hide real issues, and simply > be unneeded churn, like here. Please revisit all of them in entire > series. Covered in Hans reply. Leaving as is > > ... > >> + /* Strip out CR if one is present */ >> + strreplace(new_setting, '\n', '\0'); > > As per above. > > ... > >> + /* Remove the value part */ >> + strreplace(item, ',', '\0'); > > This is kinda non-standard pattern. > > I would see rather something like > > char *p; > > p = strchrnul(item, ','); > *p = '\0'; > > Yes, it's longer, but better to understand what's going on here. Fair enough. I'll change. I quite liked the fact I had a one-liner do the same thing - oh well :) > >> + /* Create a setting entry */ >> + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); > > sizeof(*setting) ? OK. > >> + if (!setting) { >> + ret = -ENOMEM; >> + goto fail_clear_attr; >> + } >> + setting->index = i; >> + strscpy(setting->display_name, item, TLMI_SETTINGS_MAXLEN); > > ... > >> + sprintf(tlmi_priv.pwd_admin->display_name, "admin"); >> + sprintf(tlmi_priv.pwd_admin->kbdlang, "us"); > > Not sure why you need printf() type of function here. strcpy() or > strscpy() should be enough. OK - I can update. > > ... > >> + if (WARN_ON(pwdcfg.max_length >= TLMI_PWD_BUFSIZE)) >> + pwdcfg.max_length = TLMI_PWD_BUFSIZE - 1; > > Not sure if WARN_ON() is really what has to be called here. But I > haven't checked the context deeply. Covered in Hans' reply.. Leaving as is > > ... > >> + sprintf(tlmi_priv.pwd_power->display_name, "power-on"); >> + sprintf(tlmi_priv.pwd_power->kbdlang, "us"); > > As above. Ack > > ... > >> +#ifndef _THINK_LMI_H_ >> +#define _THINK_LMI_H_ > > + linux/types.h > > (At leas bool is from there) Ack > > >> +#endif /* !_THINK_LMI_H_ */ >
