Re: [PATCH 1/4] ACPI/APMT: Use stable device ID

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unvalidated u32 ACPI node ID is passed to a signed int parameter, causing unintended collisions with platform bus sentinel values and negative sysfs device names.
- [Medium] Lack of collision handling for firmware-supplied IDs causes complete APMT initialization failure on duplicate IDs.

Pre-existing issues:
- [High] Failure to cleanly unregister resources and platform devices on error paths leaks IRQ mappings and creates dangling pointers to unmapped ACPI tables (Use-After-Free).
- [High] Missing structural bounds checking on APMT node length allows infinite loops or out-of-bounds reads.
--

--- Patch [1]: [PATCH 1/4] ACPI/APMT: Use stable device ID ---
commit 5ad772664b7900cfeb996b91d7d0c6ff27d931d6
Author: Robin Murphy <robin.murphy@arm.com>

ACPI/APMT: Use stable device ID

The APMT node format includes a unique identifier, so we can use this as
the platform device ID to give userspace stable and identifiable device
names, rather than auto numbering dependent on how the table is parsed.

> --- a/drivers/acpi/arm64/apmt.c
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -79,7 +79,7 @@ static int __init apmt_add_platform_device(struct acpi_apmt_node *node,
>  	int ret, count;
>  	struct resource res[DEV_MAX_RESOURCE_COUNT];
>  
> -	pdev = platform_device_alloc(DEV_NAME, PLATFORM_DEVID_AUTO);
> +	pdev = platform_device_alloc(DEV_NAME, node->id);

[Severity: High]
Because node->id is a u32 and platform_device_alloc() takes a signed int
for the id parameter, could this cause unintended collisions?

If the firmware provides a large ID like 0xFFFFFFFE, this would be cast to
-2 (PLATFORM_DEVID_AUTO), which seems to defeat the goal of using stable
identifiers by falling back to auto-numbering.

Similarly, a value of 0xFFFFFFFF would become -1 (PLATFORM_DEVID_NONE),
causing the ID to be omitted from the device name entirely, and other large
values would result in negative sysfs device names.

[Severity: Medium]
By relying on the firmware for the ID rather than auto-numbering, is it
possible for duplicate IDs to cause initialization failures?

If firmware provides two APMT nodes with the same id, platform_device_add()
will return -EEXIST. When this propagates up to apmt_init_platform_devices():

apmt_init_platform_devices() {
    ...
    ret = apmt_add_platform_device(apmt_node, fwnode);
    if (ret) {
        acpi_free_fwnode_static(fwnode);
        return ret;
    }
    ...
}

Will this abort the entire APMT initialization and prevent all subsequent
PMUs from being registered?

[Severity: High]
This is a pre-existing issue, but this patch might make it more reachable
if duplicate firmware IDs trigger the error path.

If apmt_add_platform_device() fails, the loop in apmt_init_platform_devices()
returns an error without unregistering the platform devices that were already
successfully added during previous iterations.

When this error propagates to acpi_apmt_init(), it unmaps the table:

acpi_apmt_init() {
    ...
    ret = apmt_init_platform_devices();
    if (ret) {
        pr_err("Failed to initialize APMT platform devices...\n");
        acpi_put_table(apmt_table);
    }
}

Since the registered platform devices hold a pointer to the ACPI node in their
platform_data, will unmapping the table leave them with dangling pointers that
could cause a use-after-free when the arm_cspmu driver probes?

Additionally, the error path inside apmt_add_platform_device() does not appear
to call acpi_unregister_gsi() to clean up the IRQ mapped by
apmt_init_resources(), which might leak the GSI mapping.

[Severity: High]
This is another pre-existing issue, but looking at the table parsing loop in
apmt_init_platform_devices():

apmt_init_platform_devices() {
    ...
    offset += apmt_node->length;
    ...
}

Does the loop inherently trust apmt_node->length without validating it against
the structural bounds? If firmware provides a node with a length of 0, could
this result in an infinite loop during early boot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783605641.git.robin.murphy@arm.com?part=1