Re: [PATCH v2 06/11] iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbounded memory allocation in report_partial_fault() allows a malicious PCIe endpoint to exhaust kernel memory (OOM).
- [High] Unratelimited logging of unexpected PRI requests allows a malicious endpoint to flood the kernel log.
--

commit 9904fc0d39f6cc48ddad4141a58759c0d2ae2811
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Support PRI Page Request in arm_smmu_handle_ppr()
    
Now, arm_smmu_page_response() can issue CMDQ_OP_PRI_RESP for page requests
from the IOPF infrastructure. Forward PRI requests from the priq into the
IOPF infrastructure for PRI-enabled masters by building an iopf_fault from
the priq entry and calling iommu_report_device_fault().

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 061f1d46fda0d..371a8bbdf6756 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2252,6 +2253,37 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device *smmu, u64 *evt)
>  	last = FIELD_GET(PRIQ_0_PRG_LAST, evt[0]);
>  	grpid = FIELD_GET(PRIQ_1_PRG_IDX, evt[1]);
>  
> +	mutex_lock(&smmu->streams_mutex);
> +	master = arm_smmu_find_master(smmu, sid);
> +	if (master && master->pri_enabled) {
> +		struct iopf_fault iopf_fault = {0};
> +		struct iommu_fault *fault = &iopf_fault.fault;
> +
> +		fault->type = IOMMU_FAULT_PAGE_REQ;
> +		if (last)
> +			fault->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;

[ ... ]

> +		fault->prm.addr = FIELD_GET(PRIQ_1_ADDR_MASK, evt[1]) << 12;
> +
> +		iommu_report_device_fault(master->dev, &iopf_fault);

[Severity: High]
This isn't a bug introduced by this patch, but does the path handling partial
faults lack bounds checking?

When a PRI request lacks the LAST_PAGE flag, the fault is processed via
report_partial_fault() in drivers/iommu/io-pgfault.c, which dynamically
allocates memory for it:

drivers/iommu/io-pgfault.c:report_partial_fault() {
...
        iopf = kzalloc_obj(*iopf);
        if (!iopf)
                return -ENOMEM;

        iopf->fault = *fault;

        mutex_lock(&fault_param->lock);
        list_add(&iopf->list, &fault_param->partial);
        mutex_unlock(&fault_param->lock);
...
}

Is it possible for a compromised PCIe endpoint to exhaust kernel memory by
spamming partial page requests with the PRG_LAST bit cleared, completely
bypassing IOMMU queue limits and triggering an OOM condition?

> +		mutex_unlock(&smmu->streams_mutex);
> +		return;
> +	}
> +	mutex_unlock(&smmu->streams_mutex);
> +
>  	dev_info(smmu->dev, "unexpected PRI request received:\n");
>  	dev_info(smmu->dev,
>  		 "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova 0x%016llx\n",

[Severity: High]
This is a pre-existing issue, but could this logging lead to log flooding or a
soft lockup?

Since this message is unratelimited and can be triggered by external device
events in arm_smmu_handle_ppr(), an endpoint sending millions of invalid PRI
requests for disabled or unrecognized Stream IDs could force the kernel to
synchronously print to the log buffer.

Would it make sense to use dev_info_ratelimited() here to prevent CPU
starvation and disk space exhaustion?

>  		 sid, ssid, grpid, last ? "L" : "",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779944354.git.nicolinc@nvidia.com?part=6