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

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

[Thorsten Blum]

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.

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);
 }
 
 /**

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