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

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

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.