Re: [PATCH] iommu/s390: Fix NULL dereference in iova_to_phys() with ZPCI_TABLE_TYPE_RFX

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

Pre-existing issues:
- [High] Intermediate IOMMU translation tables are dynamically allocated during map/unmap but never freed when empty, allowing an attacker to exhaust kernel memory.
- [High] IOMMU unmap operations needlessly allocate page tables with GFP_ATOMIC when unmapping unmapped regions, which wastes memory and causes unmap failures if allocation fails.
--

--- Patch [1]: [PATCH] iommu/s390: Fix NULL dereference in iova_to_phys() with ZPCI_TABLE_TYPE_RFX ---
commit 87f13b9b4d2f1a6021028f52263833bccd842526
Author: Niklas Schnelle <[email protected]>

iommu/s390: Fix NULL dereference in iova_to_phys() with ZPCI_TABLE_TYPE_RFX

When using a 5-level translation table via ZPCI_TABLE_TYPE_RFX,
get_rso_from_iova() can return NULL when the region-first entry is invalid.
This patch prevents a NULL pointer dereference by adding a missing NULL
check for the region-second origin before accessing it.

> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index f148f559ac564..58ca7727b7f24 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -974,6 +974,8 @@ static unsigned long *get_rto_from_iova(struct s390_domain *domain,
>  	case ZPCI_TABLE_TYPE_RFX:
>  	case ZPCI_TABLE_TYPE_RSX:
>  		rso = get_rso_from_iova(domain, iova);
> +		if (!rso)
> +			return NULL;
>  		rsx = calc_rsx(iova);
>  		rse = READ_ONCE(rso[rsx]);
>  		if (!reg_entry_isvalid(rse))

[Severity: High]
This is a pre-existing issue, but is it possible to exhaust kernel memory
with intermediate IOMMU translation tables?

Looking at dma_walk_cpu_trans() in drivers/iommu/s390-iommu.c:

drivers/iommu/s390-iommu.c:dma_walk_cpu_trans() {
    ...
    sto = dma_get_seg_table_origin(&rto[rtx], gfp);
    if (!sto)
        return NULL;

    sx = calc_sx(dma_addr);
    pto = dma_get_page_table_origin(&sto[sx], gfp);
    ...
}

These intermediate tables are dynamically allocated unconditionally, but
they appear to only be freed during domain destruction:

drivers/iommu/s390-iommu.c:dma_cleanup_tables() {
    ...
        for (rtx = 0; rtx < ZPCI_TABLE_ENTRIES; rtx++)
            if (reg_entry_isvalid(domain->dma_table[rtx]))
                dma_free_seg_table(domain->dma_table[rtx]);
    ...
}

Can an unprivileged user with VFIO access repeatedly map and unmap pages
at large strides across a huge IOVA aperture to leak these dynamically
allocated intermediate tables and cause an out of memory condition?


[Severity: High]
This is a pre-existing issue, but does unmapping an unmapped region
needlessly allocate page tables?

Looking at s390_iommu_invalidate_trans() in drivers/iommu/s390-iommu.c:

drivers/iommu/s390-iommu.c:s390_iommu_invalidate_trans() {
    ...
    for (i = 0; i < nr_pages; i++) {
        entry = dma_walk_cpu_trans(s390_domain, dma_addr, GFP_ATOMIC);
        if (unlikely(!entry)) {
            rc = -EINVAL;
            break;
        }
    ...
}

When encountering an unmapped hole, dma_get_seg_table_origin()
unconditionally allocates missing page table directories:

drivers/iommu/s390-iommu.c:dma_get_seg_table_origin() {
    ...
    rte = READ_ONCE(*rtep);
    if (reg_entry_isvalid(rte)) {
        sto = get_rt_sto(rte);
    } else {
        sto = dma_alloc_cpu_table(gfp);
    ...
}

If this atomic allocation fails under memory pressure during a bulk unmap,
would the loop abort and return an error? Could this leave any remaining
pages in the requested range fully mapped in the IOMMU while the kernel
potentially frees the underlying physical memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1