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. > + * 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" ... > + * 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. > + */ ... > + /* > + * Duplicated call required to match bios workaround for behavior bios -> BIOS > + * 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. > + return sysfs_emit(buf, "%d\n", setting->valid); > +} > + Unneeded blank line. Same for other similar places. > +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. ... > + 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. > + /* pwdlen == 0 is allowed to clear the password */ > + if (pwdlen != 0 && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen))) The ' != 0' part is redundant. > + 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. > + 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. > + 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'. > + 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. ... > + 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. ... > + /* 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. > + /* Create a setting entry */ > + setting = kzalloc(sizeof(struct tlmi_attr_setting), GFP_KERNEL); sizeof(*setting) ? > + 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. ... > + 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. ... > + sprintf(tlmi_priv.pwd_power->display_name, "power-on"); > + sprintf(tlmi_priv.pwd_power->kbdlang, "us"); As above. ... > +#ifndef _THINK_LMI_H_ > +#define _THINK_LMI_H_ + linux/types.h (At leas bool is from there) > +#endif /* !_THINK_LMI_H_ */ -- With Best Regards, Andy Shevchenko
