Skip to contentSkip to conversation

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

PATCH Applied Niklas Schnelle 7 participants 8 messages Aug 18 – Sep 7

Subsystems (3)
s390 IOMMU s390 IOMMU (PCI)
lore
Share & tools
lore

Niklas Schnelle

author maintainer
#0 lore raw

PATCH 1/1 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() returns NULL when the region-first entry is invalid. Yet in get_rto_from_iova() the region-second origin rso is not checked to be non-NULL before accessing rso[rsx] leading to a NULL pointer dereference instead of a NULL return when iova_to_phys() is called on a unmapped IOVA. Fix this by adding the missing NULL check.

Cc [email protected]
Fixes 81244074b518 ("iommu/s390: allow larger region tables")
Signed-off-by Niklas Schnelle <[email protected]>
---
 drivers/iommu/s390-iommu.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iommu/s390-iommu.c +2 -0
index f148f559ac56..58ca7727b7f2 100644
@@ -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))

---
base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260818-iommu_fix_iova_to_phys-5879275b95ec

Best regards,

Signature · 1 line
Niklas Schnelle
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

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.

drivers/iommu/s390-iommu.c static unsigned long *get_rto_from_iova(struct s390_domain *domain, · line 981

Quoted 13 lines
drivers/iommu/s390-iommu.c +2 -0
index f148f559ac564..58ca7727b7f24 100644
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v1 1/1
#2 lore raw
In reply to Niklas Schnelle
Quoted 7 lines · Niklas Schnelle

When using a 5-level translation table via ZPCI_TABLE_TYPE_RFX get_rso_from_iova() returns NULL when the region-first entry is invalid. Yet in get_rto_from_iova() the region-second origin rso is not checked to be non-NULL before accessing rso[rsx] leading to a NULL pointer dereference instead of a NULL return when iova_to_phys() is called on a unmapped IOVA. Fix this by adding the missing NULL check.

Quoted 6 lines
Cc [email protected]
Fixes 81244074b518 ("iommu/s390: allow larger region tables")
Signed-off-by Niklas Schnelle<[email protected]>
---
  drivers/iommu/s390-iommu.c | 2 ++
  1 file changed, 2 insertions(+)
Reviewed-by Farhan Ali<[email protected]>
maintainer Reviewed-by on v1 1/1
#3 lore raw
In reply to Niklas Schnelle
Quoted 10 lines · Niklas Schnelle

When using a 5-level translation table via ZPCI_TABLE_TYPE_RFX get_rso_from_iova() returns NULL when the region-first entry is invalid. Yet in get_rto_from_iova() the region-second origin rso is not checked to be non-NULL before accessing rso[rsx] leading to a NULL pointer dereference instead of a NULL return when iova_to_phys() is called on a unmapped IOVA. Fix this by adding the missing NULL check.

Cc [email protected]
Fixes 81244074b518 ("iommu/s390: allow larger region tables")
Signed-off-by Niklas Schnelle <[email protected]>
Reviewed-by Matthew Rosato <[email protected]>
Quoted 23 lines
---
 drivers/iommu/s390-iommu.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iommu/s390-iommu.c +2 -1
index f148f559ac56..58ca7727b7f2 100644
@@ -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))
 
---

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
change-id: 20260818-iommu_fix_iova_to_phys-5879275b95ec

Best regards,

Reviewed-by on v1 1/1
#4 lore raw
In reply to Niklas Schnelle
Quoted 19 lines · Niklas Schnelle

When using a 5-level translation table via ZPCI_TABLE_TYPE_RFX get_rso_from_iova() returns NULL when the region-first entry is invalid. Yet in get_rto_from_iova() the region-second origin rso is not checked to be non-NULL before accessing rso[rsx] leading to a NULL pointer dereference instead of a NULL return when iova_to_phys() is called on a unmapped IOVA. Fix this by adding the missing NULL check.

Cc [email protected]
Fixes 81244074b518 ("iommu/s390: allow larger region tables")
Signed-off-by Niklas Schnelle <[email protected]>
---
 drivers/iommu/s390-iommu.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iommu/s390-iommu.c +0 -0
index f148f559ac56..58ca7727b7f2 100644

drivers/iommu/s390-iommu.c static unsigned long *get_rto_from_iova(struct s390_domain *domain, · line 981

Quoted 8 lines
drivers/iommu/s390-iommu.c +2 -0
@@ -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))

Looks good to me!

Reviewed-by Benjamin Block <[email protected]>
Signature · 4 lines
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt         /        Geschäftsführung: David Faller
Sitz der Ges.: Ehningen     /     Registergericht: AmtsG Stuttgart, HRB 243294
maintainer Applied
#5 lore raw
In reply to Niklas Schnelle
Quoted 7 lines · Niklas Schnelle

When using a 5-level translation table via ZPCI_TABLE_TYPE_RFX get_rso_from_iova() returns NULL when the region-first entry is invalid. Yet in get_rto_from_iova() the region-second origin rso is not checked to be non-NULL before accessing rso[rsx] leading to a NULL pointer dereference instead of a NULL return when iova_to_phys() is called on a unmapped IOVA. Fix this by adding the missing NULL check.

Quoted 6 lines
Cc [email protected]
Fixes 81244074b518 ("iommu/s390: allow larger region tables")
Signed-off-by Niklas Schnelle <[email protected]>
---
 drivers/iommu/s390-iommu.c | 2 ++
 1 file changed, 2 insertions(+)

Applied, thank you!

maintainer
#6 lore raw
Quoted 8 lines · Vasily Gorbik
Quoted 7 lines · Niklas Schnelle

When using a 5-level translation table via ZPCI_TABLE_TYPE_RFX get_rso_from_iova() returns NULL when the region-first entry is invalid. Yet in get_rto_from_iova() the region-second origin rso is not checked to be non-NULL before accessing rso[rsx] leading to a NULL pointer dereference instead of a NULL return when iova_to_phys() is called on a unmapped IOVA. Fix this by adding the missing NULL check.

Quoted 8 lines
Quoted 6 lines
Cc [email protected]
Fixes 81244074b518 ("iommu/s390: allow larger region tables")
Signed-off-by Niklas Schnelle <[email protected]>
---
 drivers/iommu/s390-iommu.c | 2 ++
 1 file changed, 2 insertions(+)

Applied, thank you!

Oh, I was too eager here. Please disregard my previous "Applied" message. This usually goes via IOMMU subsystem tree and not s390 tree.

Joerg, Will, I'll leave this one to you. Sorry for the confusion.