Re: [PATCH] platform/x86: dell-wmi-sysman: use strnlen in strlcpy_attr

[Thorsten Blum <thorsten.blum@linux.dev>]

On Wed, May 06, 2026 at 02:53:46PM +0300, Ilpo Järvinen wrote:
> On Sat, 2 May 2026, Thorsten Blum wrote:
> 
> > Use strnlen() to limit source string scanning to MAX_BUFF bytes. Return
> > early on error and make the "empty string means not applicable" case
> > explicit.
> > 
> > Use 'const char *' for the read-only source string while at it.
> 
> Hi Thorsten,
> 
> First of all, thanks for looking into these.
> 
> > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> > ---
> >  .../dell/dell-wmi-sysman/dell-wmi-sysman.h    |  2 +-
> >  .../x86/dell/dell-wmi-sysman/sysman.c         | 20 ++++++++++---------
> >  2 files changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/dell-wmi-sysman.h b/drivers/platform/x86/dell/dell-wmi-sysman/dell-wmi-sysman.h
> > index 5278a93fdaf7..f6943301b857 100644
> > --- a/drivers/platform/x86/dell/dell-wmi-sysman/dell-wmi-sysman.h
> > +++ b/drivers/platform/x86/dell/dell-wmi-sysman/dell-wmi-sysman.h
> > @@ -162,7 +162,7 @@ static ssize_t curr_val##_store(struct kobject *kobj,				\
> >  
> >  union acpi_object *get_wmiobj_pointer(int instance_id, const char *guid_string);
> >  int get_instance_count(const char *guid_string);
> > -void strlcpy_attr(char *dest, char *src);
> > +void strlcpy_attr(char *dest, const char *src);
> >  
> >  int populate_enum_data(union acpi_object *enumeration_obj, int instance_id,
> >  			struct kobject *attr_name_kobj, u32 enum_property_count);
> > diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> > index 51d25fdc1389..6c9911accefc 100644
> > --- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> > +++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> > @@ -234,18 +234,20 @@ static const struct kobj_type attr_name_ktype = {
> >   * @dest: Where to copy the string to
> >   * @src: Where to copy the string from
> >   */
> > -void strlcpy_attr(char *dest, char *src)
> > +void strlcpy_attr(char *dest, const char *src)
> >  {
> > -	size_t len = strlen(src) + 1;
> > +	size_t len = strnlen(src, MAX_BUFF);
> >  
> > -	if (len > 1 && len <= MAX_BUFF)
> > -		strscpy(dest, src, len);
> > -
> > -	/*len can be zero because any property not-applicable to attribute can
> > -	 * be empty so check only for too long buffers and log error
> > -	 */
> > -	if (len > MAX_BUFF)
> > +	if (len == MAX_BUFF) {
> >  		pr_err("Source string returned from BIOS is out of bound!\n");
> > +		return;
> > +	}
> > +
> > +	/* Empty string means "not applicable" and is skipped intentionally. */
> > +	if (len == 0)
> > +		return;
> > +
> > +	strscpy(dest, src, len + 1);
> 
> And how exactly is this last line different from strscpy(dest, serc, 
> MAX_BUFF);
> 
> ?

The result is the same, but happy to use MAX_BUFF instead. However,
since we already know the length of the source string and that it is
NUL-terminated within the first MAX_BUFF bytes, we could just use
memcpy(dest, src, len + 1) directly.

> I agree something should be done here but I don't like this approach. The 
> length passed to strscpy() should be "Size of the destination buffer" but 
> your approach calculated the length of the source string (?!):
> 
> /**
>  * strscpy - Copy a C-string into a sized buffer
>  * @dst: Where to copy the string to
>  * @src: Where to copy the string from
>  * @...: Size of destination buffer (optional)
> 
> So, to make it safe and sound logically, to me it looks more like the 
> _caller_ should pass the output buffer's size to this function. Or 
> alternatively, this function could be wrapped with a macro such that the 
> sizeof(*dest) can still be checked to be of correct length.

I have some other changes queued up and would prefer to change this in a
follow-up patch since there are quite a few call sites that need to be
updated.

> Also, this function presents itself with str*() name like a generic string 
> copy function but what it really is more attr_check_and_copy(), it might 
> not copy anything if the checks fail.

OK, I'll take this into account when changing the function signature.

Thanks,
Thorsten