Re: [PATCH v13 RESEND 1/5] platform/x86: Sort firmware attributes class includes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds array read in get_instance_id macro due to an off-by-one error.
- [High] Out-of-bounds heap read and write in attribute_property_store macro via memchr.
--

--- Patch [1]: [PATCH v13 RESEND 1/5] platform/x86: Sort firmware attributes class includes ---
commit dff06344a1a1c35eb28b9e59670eeac00408066c
Author: Sean Rhodes <sean@starlabs.systems>

platform/x86: Sort firmware attributes class includes

Move firmware_attributes_class.h into the main include block and sort the
affected includes. This lets the header move replace each include in place.

> diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> index ab46a023cc34d..4ff9003c6589b 100644
> --- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> +++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> @@ -8,18 +8,19 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/align.h>
> -#include <linux/fs.h>
>  #include <linux/dmi.h>
> -#include <linux/module.h>
> +#include "../../firmware_attributes_class.h"
> +#include <linux/fs.h>
>  #include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/nls.h>
>  #include <linux/string.h>
>  #include <linux/sysfs.h>
>  #include <linux/wmi.h>
> +
>  #include "dell-wmi-sysman.h"

[Severity: High]
This is a pre-existing issue, but does the get_instance_id() macro in
dell-wmi-sysman.h have an off-by-one error in its loop condition?

drivers/platform/x86/dell/dell-wmi-sysman/dell-wmi-sysman.h:get_instance_id() {
	...
	for (i = 0; i <= wmi_priv.type##_instances_count; i++) {
		if (!(strcmp(kobj->name, wmi_priv.type##_data[i].attribute_name)))
	...
}

If no match is found among the valid entries, the loop will evaluate 
i = instances_count and access wmi_priv.type##_data[i].attribute_name, which
is memory beyond the allocated array. The subsequent strcmp() function will
then dereference this out-of-bounds heap memory.

[Severity: High]
This is a pre-existing issue, but does the attribute_property_store() macro in 
dell-wmi-sysman.h have an out-of-bounds memory access?

drivers/platform/x86/dell/dell-wmi-sysman/dell-wmi-sysman.h:attribute_property_store() {
	...
	buf_cp = kstrdup(buf, GFP_KERNEL);
	if (!buf_cp)
		return -ENOMEM;
	p = memchr(buf_cp, '\n', count);
	
	if (p != NULL)
		*p = '\0';
	...
}

kstrdup() determines the allocation size using strlen(buf). If a user
provides an input buffer with an embedded null byte before the newline
character, kstrdup() will allocate a buffer smaller than the user-provided
count.

Subsequently, memchr() searches for a newline character up to count bytes, 
reading beyond the dynamically allocated memory. Furthermore, if a newline 
character happens to be found in the out-of-bounds memory block, the code 
executes *p = '\0', causing an out-of-bounds heap write.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787584107.git.sean@starlabs.systems?part=1