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

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

[Ilpo Järvinen]

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);

?

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.

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.


-- 
 i.

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

[Thorsten Blum]

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

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

[Ilpo Järvinen]

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

On Wed, 6 May 2026, Thorsten Blum wrote:

> 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.

The size should be related to the output buffer(!) and not come from 
length of the input (using input len is unsound way to engineer this). In 
that sense, MAX_BUFF is better than using len, but see my question about 
patch order below.

> 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.

Using memcpy() would be wrong direction to take. It doesn't validate the 
output length any better against the output buffer size than the current 
approach does.

> > 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.

Why exactly you have to make this change first at all? Can't this just be 
placed after the other changes and done properly right away. Do those 
other changes touch this same function so changing order is not easy?

> > 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.


-- 
 i.