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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4935 bytes --]

On Wed, 19 Aug 2026, David Laight wrote:

> On Tue, 18 Aug 2026 15:39:35 +0200
> Thorsten Blum <thorsten.blum@linux.dev> wrote:
> 
> > On Tue, Aug 18, 2026 at 04:20:10PM +0300, Ilpo Järvinen wrote:
> > > On Tue, 18 Aug 2026, Thorsten Blum wrote:  
> > > > 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: stable@vger.kernel.org
> > > > > > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Keep and reword the newline comment
> > > > > > - v1: https://lore.kernel.org/r/20260810132018.156868-3-thorsten.blum@linux.dev/
> > > > > > ---
> > > > > >  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'.  
> > > 
> > > Okay, I was thinking in my mind if I've a off-by-one error in my 
> > > suggestion but didn't want to spend too much time on figuring it out.
> > >   
> > > > strscpy(setting->password, buf, pwdlen + 1) would work, but since we
> > > > already know that exactly pwdlen bytes need to be copied, memcpy() is
> > > > sufficient.  
> 
> No that overwrites past the end out the output buffer.

Hi David,

So are you suggesting ->password cannot hold a maxlen long password or 
what?

> > > It may work, but since you then go to nul terminate it yourself, I think 
> > > using the existing function is way better than memcpy() + custom nul 
> > > termination code.  
> > 
> > I used memcpy() because we already determined the string length pwdlen,
> > which strscpy() would have to determine again internally.
> > 
> > The performance difference shouldn't matter here, so it's mostly a
> > matter of style or personal preference. I'm fine either way.
> 
> Personally I'd always use memcpy() if the size is known.
> 
> Whoever added the strscpy() should have used the size of the array not
> the soft bound for the password length (which is limited by the array size).
> 
> In any case the code could be:
> 	pwdlen = count;
> 	if (pwdlen && buf[pwdlen - 1] == '\n')
> 		pwdlen--;
> 	...
> 	memcpy(setting->password, buf, pwdlen);
> 	setting->password[pwdlen] = 0;
> 	return count;
> }
> 
> I'm sure there could be (might even be) a helper to copy some
> characters and append a '\0'.

Are you perhaps looking for strscpy() ? :-)

--
 i.