Re: [PATCH v4 2/2] iommu/riscv: create a auxiliary device for HPM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data truncation, negative sysfs ID, and information leak when passing physical address to auxiliary device ID.
- [High] Teardown asymmetry causing Use-After-Disable and guaranteed Use-After-Free of PMU structure.
- [Medium] Overly generic auxiliary device namespace risking collisions.
--

--- Patch [2]: [PATCH v4 2/2] iommu/riscv: create a auxiliary device for HPM ---
commit b51f5eeba53f86d299b4eda15a076df1b60764b8
Author: Zong Li <zong.li@sifive.com>

iommu/riscv: create a auxiliary device for HPM

This commit creates an auxiliary device for the hardware performance monitor
when supported by the IOMMU. It uses the physical address of the device
directory table as the unique ID for the auxiliary device.

> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> --- a/drivers/iommu/riscv/iommu.c
> +++ b/drivers/iommu/riscv/iommu.c

[ ... ]

> @@ -565,6 +566,21 @@ static irqreturn_t riscv_iommu_fltq_process(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * IOMMU Hardware performance monitor
> + */
> +static int riscv_iommu_hpm_enable(struct riscv_iommu_device *iommu)
> +{
> +	struct auxiliary_device *auxdev;
> +
> +	auxdev = __devm_auxiliary_device_create(iommu->dev, KBUILD_MODNAME,
> +						"pmu", iommu, iommu->ddt_phys);

[Severity: High]
Will this cause truncation and potentially negative sysfs IDs?

The fifth argument to __devm_auxiliary_device_create() is an int id, but we
are passing iommu->ddt_phys which is a 64-bit dma_addr_t.

If bit 31 of the physical address is set (which is common for RAM on RISC-V),
the truncated integer becomes negative, resulting in malformed sysfs
directories.

Could this also cause sysfs naming collisions if two IOMMUs share the same
lower 32-bits of their physical address?

[Severity: Medium]
Could KBUILD_MODNAME result in an overly generic namespace for the auxiliary
bus?

Since this source file is compiled as iommu.o, KBUILD_MODNAME evaluates to
"iommu", creating an auxiliary device named "iommu.pmu.<id>".

If another IOMMU driver also registers a "pmu" auxiliary device using
KBUILD_MODNAME, would they collide on the bus, or could the riscv_iommu_pmu
driver incorrectly bind to the wrong device in a multi-arch build?

[Severity: High]
Does using a devm-managed allocation here create a teardown asymmetry with
the manual hardware shutdown in the remove path?

Because devm cleanup executes after the driver's remove callback returns,
the auxiliary device will remain active while the IOMMU hardware is disabled:

riscv_iommu_remove()
    ...
    riscv_iommu_iodir_set_mode(iommu, RISCV_IOMMU_DDTP_IOMMU_MODE_OFF);
    ...
<driver remove returns>
<devm cleanup destroys the auxiliary device>

Does this leave a window where an active devm-managed PMU interrupt could fire
and attempt to access disabled hardware?

> +	if (!auxdev)
> +		return -ENODEV;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714083625.1083606-1-zong.li@sifive.com?part=2