Hi, On 5/27/21 4:16 PM, 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. Since you have more remarks then I anticipated I guess it is best if I drop v4 from my review-hans branch and Mark does a v5. Mark, you can then also just squash the addition of the MAINTAINERS entry into v5. <snip> >> + 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. As I already explained in a previous reply the password cannot contain '\n' so this cannot happen. I did miss that this was still there, where as new_password_store() uses strreplace() which is not really consistent with each other... >> + 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. I asked for all this to be set to NULL in my review of v2, since there are various "goto out"s in the function and out: calls kfree() on all 3, in v2 there was a path which would end up calling kfree() on an uninitialized char *. IMHO just initializing all of them up front is best here because that guarantees that they are either still NULL, or point to memory returned by kmalloc. >> + /* 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. Erm, you actually suggested using strreplace() here in your previous review ... >> + 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. There are 2 max_lengths, one hardcoded in the driver so we don't need to dynamically manage memory for the password storage and one which is actually queried from the BIOS, the BIOS max-length should always be less then the hardcoded one in the driver (and currently this is true for all known models). I suggested adding the WARN_ON so that if future BIOS-es ever bump max_length we will get a bug-report about this and then we can bump the driver's TLMI_PWD_BUFSIZE value. Regards, Hans
