Re: [PATCH v2] platform/x86: think-lmi: Fix current password length check

On Tue, Aug 18, 2026 at 02:28:50PM +0300, Ilpo Järvinen wrote:
> On Thu, 13 Aug 2026, Thorsten Blum wrote:
> > current_password_store() checks the password length before removing the
> > trailing newline, which can reject valid passwords that are exactly
> > ->maxlen bytes long.
> > 
> > It also passes ->maxlen to strscpy(), which truncates passwords without
> > a newline.
> > 
> > Use strchrnul() to measure the password length up to the newline, then
> > copy that many bytes and add a trailing NUL terminator.
> > 
> > Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> > Cc: [email protected]
> > Signed-off-by: Thorsten Blum <[email protected]>
> > ---
> > Changes in v2:
> > - Keep and reword the newline comment
> > - v1: https://lore.kernel.org/r/[email protected]/
> > ---
> >  drivers/platform/x86/lenovo/think-lmi.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c
> > index e215e86e3db7..d0ceb6aaa69e 100644
> > --- a/drivers/platform/x86/lenovo/think-lmi.c
> > +++ b/drivers/platform/x86/lenovo/think-lmi.c
> > @@ -438,14 +438,14 @@ static ssize_t current_password_store(struct kobject *kobj,
> >  	struct tlmi_pwd_setting *setting = to_tlmi_pwd_setting(kobj);
> >  	size_t pwdlen;
> >  
> > -	pwdlen = strlen(buf);
> > +	/* Strip newline; setting password won't work if one is present. */
> > +	pwdlen = strchrnul(buf, '\n') - buf;
> >  	/* pwdlen == 0 is allowed to clear the password */
> >  	if (pwdlen && ((pwdlen < setting->minlen) || (pwdlen > setting->maxlen)))
> >  		return -EINVAL;
> >  
> > -	strscpy(setting->password, buf, setting->maxlen);
> > -	/* Strip out CR if one is present, setting password won't work if it is present */
> > -	strreplace(setting->password, '\n', '\0');
> > +	memcpy(setting->password, buf, pwdlen);
> > +	setting->password[pwdlen] = '\0';
> 
> Hi,
> 
> I don't understand why is this strscpy() -> memcpy() conversion required? 

It's not required, strscpy() would also work.

> Wouldn't it work with:
> 
> 	strscpy(..., pwdlen);
> 
> ?

However, strscpy(setting->password, buf, pwdlen) wouldn't work because
pwdlen is the number of characters to copy, but the destination buffer
size also needs to include room for the NUL terminator. Since there is
no NUL before pwdlen, it would copy only pwdlen - 1 characters and set
setting->password[pwdlen - 1] = '\0'.

strscpy(setting->password, buf, pwdlen + 1) would work, but since we
already know that exactly pwdlen bytes need to be copied, memcpy() is
sufficient.