Re: [PATCH v2 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] Platform device ID collision causing APMT parsing abort and subsequent UAF.

Pre-existing issues:
- [High] Missing rollback on failure leaves previously registered platform devices holding dangling pointers to an unmapped ACPI table.
- [High] Missing bounds check on `apmt_node->length` allows out-of-bounds memory read.
- [Medium] GSI IRQ mapping is leaked on platform device registration error paths.
--

--- Patch [1]: [PATCH v2 1/4] ACPI/APMT: Use stable device ID ---
commit 895c0eb864a4218fbb99620f4fa9efb2dc4e091c
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.

> diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
> index bb010f6164e52..91fcdd289e637 100644
> --- a/drivers/acpi/arm64/apmt.c
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -76,10 +76,12 @@ static int __init apmt_add_platform_device(struct acpi_apmt_node *node,
>  					   struct fwnode_handle *fwnode)
>  {
>  	struct platform_device *pdev;
> -	int ret, count;
> +	int ret, count, uid = node->id & INT_MAX;
>  	struct resource res[DEV_MAX_RESOURCE_COUNT];
>  
> -	pdev = platform_device_alloc(DEV_NAME, PLATFORM_DEVID_AUTO);
> +	if (uid != node->id)
> +		pr_warn("Unexpectedly large UID 0x%x, truncated to 0x%x\n", node->id, uid);
> +	pdev = platform_device_alloc(DEV_NAME, uid);

[Severity: High]
Does this newly introduced truncation create a risk of ID collisions if two
unique 32-bit APMT node IDs differ only in the sign bit?

If a collision occurs, platform_device_add() will fail with -EEXIST. This
halts the initialization of all remaining PMU nodes and triggers another issue.

[Severity: High]
This is a pre-existing issue, but when apmt_init_platform_devices() fails
(which is now more likely due to potential ID collisions), the caller unmaps
the ACPI table without a cleanup loop:

drivers/acpi/arm64/apmt.c:acpi_apmt_init() {
    ...
	ret = apmt_init_platform_devices();
	if (ret) {
		pr_err("Failed to initialize APMT platform devices, ret: %d\n", ret);
		acpi_put_table(apmt_table);
	}
}

Will this leave any previously successfully registered platform devices holding
dangling pointers in their platform_data referencing the now unmapped table
memory, leading to a use-after-free when a driver later binds?

[Severity: Medium]
This is a pre-existing issue, but if a device addition fails, does the
cleanup path leak the GSI IRQ mapping?

drivers/acpi/arm64/apmt.c:apmt_init_resources() {
    ...
		irq = acpi_register_gsi(NULL, node->ovflw_irq, trigger,
						ACPI_ACTIVE_HIGH);
    ...
}

When apmt_add_platform_device() takes the error path:

drivers/acpi/arm64/apmt.c:apmt_add_platform_device() {
    ...
dev_put:
	platform_device_put(pdev);

	return ret;
}

Should there be an acpi_unregister_gsi() call to reverse the mapping created
during apmt_init_resources()?

[Severity: High]
This is a pre-existing issue, but does the table parsing loop in
apmt_init_platform_devices() need a bounds check on apmt_node->length?

drivers/acpi/arm64/apmt.c:apmt_init_platform_devices() {
    ...
	while (offset < end) {
		apmt_node = ACPI_ADD_PTR(struct acpi_apmt_node, apmt,
				 offset);
        ...
		offset += apmt_node->length;
	}
    ...
}

If a malformed ACPI table provides a maliciously small or truncated length,
could this lead to ACPI_ADD_PTR yielding a misaligned pointer and reading
memory outside the mapped ACPI bounds?

>  	if (!pdev)
>  		return -ENOMEM;
>

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