Thanks Hans On 2021-05-21 4:10 a.m., Hans de Goede wrote: > Hi Mark, > > On 5/20/21 7:18 PM, Mark Pearson wrote: <snip> >>>> + + /* Format: 'PasswordType,CurrentPw,NewPw,Encoding,KbdLang;' >>>> */ + len = strlen(setting->password) + >>>> strlen(encoding_options[setting->encoding]) + + >>>> strlen(setting->kbdlang) + 3 /* type */ + + strlen(new_pwd) + >>>> 6 /* punctuation and terminator*/; + auth_str = kzalloc(len, >>>> GFP_KERNEL); + snprintf(auth_str, len, "%s,%s,%s,%s,%s;", + >>>> setting->pwd_type, setting->password, new_pwd, + >>>> encoding_options[setting->encoding], setting->kbdlang); + ret = >>>> tlmi_simple_call(LENOVO_SET_BIOS_PASSWORD_GUID, auth_str); >>> >>> So I guess on success any subsequent calls need to use the new >>> password, so the user is expected to write the new password to >>> the current_password file after changing the password this way? >>> >>> I just checked the dell-wmi-sysman code and that does this: >>> >>> /* clear current_password here and use user input from >>> wmi_priv.current_password */ if (!ret) memset(current_password, >>> 0, MAX_BUFF); >>> >>> Where current_password points to either the user or admin cached >>> password, depending on which one is being changed. >>> >>> So that seems to work the same way as what you are doing here >>> (the user needs to write the new password to current_password >>> after changing it through the new_password sysfs attribute). Can >>> you add a patch to the patch-set documenting this in >>> Documentation/ABI/testing/sysfs-class-firmware-attributes ? >>> >>> Also you may want to consider clearing out the old cached >>> password on success like the Dell code is doing. >>> >> Would you have any objections/concerns if, on a successful >> password update, this function just updates the stored password >> setting to the new password. Seems nicer from a user point of view >> then having to go and feed the current_password field again. e.g: >> strncpy(new_pwd, setting->password, TLMI_PWD_MAXLEN) > > Please use strscpy, strncpy has horrible syntax and using it is easy > to get wrong. > > e.g. the above example is wrong if strlen(new_pwd) >= > TLMI_PWD_MAXLEN the resulting setting->password will not be 0 > terminated (and you seem to have swapped src and dst, but that is > unrelated to strncpy being a sucky function). Yeah, sorry, typed as an example only, and will use strscpy > > This also make me realize that the code has 2 max-pwd-lengths: > > setting->maxlen TLMI_PWD_MAXLEN > > I think you should put a: > > if (WARN_ON(pwdcfg.max_length > TLMI_PWD_MAXLEN)) pwdcfg.max_length = > TLMI_PWD_MAXLEN; > > in tlmi_analyze() so that we get a kernel-backtrace (and thus > bugreports if this condition ever becomes true. Sounds good > > And then use pwdcfg.max_length everywhere (e.g. also for the check > in current_password_store()). > > Also the length checks in current_password_store() and > new_password_store() should probably also check against > settings->minlen ? Agreed - will add > > >> I know it would make Dell and Lenovo operate differently (I can add >> that detail to the documentation) but it just feels like a nicer >> design. > > That works for me. Perhaps you can also do a (compile tested only) > RFC patch for the Dell code to do the same thing (replace the memset > 0 with the strscpy) to see if the Dell folks are ok with also doing > things this way ? > I'm not hugely comfortable with that. If for some reason it broke things for Dell customers I wouldn't want to be responsible :) I'd rather they made the changes and were able to test it - I know that's what I'd prefer if it was the other way around. Apologies if I'm being over cautious! I've added the new Dell kernel group email to the thread so they're aware :) Mark
