Skip to contentSkip to conversation

iommu/dma: Catch scatterlist length overflows

PATCH Applied Krzysztof Karas 5 participants 6 messages Oct 2 – Oct 9

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 iommu/dma: Catch scatterlist length overflows

It is possible, when a very large mapping uses only one scatterlist, that padding overflows scatterlist's length field. This results in:
1) silently wrapping the value
2) smaller than desired mappings produced by iommu_map_sg
3) leaving mapped bytes in memory (no iommu_unmap)

Address this issue by adding overflow detection for scatterlist length field.

Fixes 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
Assisted-by LLM

---
This patch was previously part of "drivers: Improve memory management for large object allocations when i915/shmem is used with iommu" series, but was extracted and posted separately on request of one of the reviewers (Andi Shyti). There have been no functional changes since last version.

this version:
* Added separating blank lines (Andi);
* Added IOMMU maintainers suggested by get_maintainer.pl script to CC (Andi);

 drivers/iommu/dma-iommu.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/iommu/dma-iommu.c +19 -1
index 58c624513cd43..06edb4056dc1b 100644
@@ -1477,6 +1477,11 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 		sg_dma_len(s) = s_length;
 		s->offset -= s_iova_off;
 		s_length = iova_align(iovad, s_length + s_iova_off);
+		if (overflows_type(s_length, s->length)) {
+			ret = -EOVERFLOW;
+
+			goto out_restore_sg;
+		}
 		s->length = s_length;
 
 		/*
@@ -1493,7 +1498,20 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 		 *   time through here (i.e. before it has a meaningful value).
 		 */
 		if (pad_len && pad_len < s_length - 1) {
-			prev->length += pad_len;
+			unsigned int new_prev_len;
+
+			/*
+			 * For large mappings spanning multiple GBs we
+			 * may not be able to fit all needed padding into
+			 * sg->length.
+			 */
+			if (check_add_overflow(prev->length, pad_len, &new_prev_len)) {
+				ret = -EOVERFLOW;
+
+				goto out_restore_sg;
+			}
+
+			prev->length = new_prev_len;
 			iova_len += pad_len;
 		}
 
Signature · 1 line
2.34.1
#1 lore raw

✗ i915.CI.BAT: failure for iommu/dma: Catch scatterlist length overflows

== Series Details ==

== Series Details ==

Series: iommu/dma: Catch scatterlist length overflows
URL : https://patchwork.freedesktop.org/series/175398/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_19266 -> Patchwork_175398v1
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with Patchwork_175398v1 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_175398v1, please notify your bug team ([email protected]) to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/index.html

Participating hosts (39 -> 37)
------------------------------

Missing (2): bat-dg2-13 fi-glk-j4005

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in Patchwork_175398v1:

### IGT changes ###

#### Possible regressions ####

* igt@core_hotunplug@unbind-rebind:
- bat-arlh-2: [PASS][1] -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/bat-arlh-2/igt@[email protected]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/bat-arlh-2/igt@[email protected]

Known issues
------------

Here are the changes found in Patchwork_175398v1 that come from known issues:

### IGT changes ###

#### Possible fixes ####

* igt@i915_selftest@live@sanitycheck:
- fi-kbl-7567u: [DMESG-WARN][3] ([i915#13735]) -> [PASS][4] +79 other tests pass
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/fi-kbl-7567u/igt@i915_selftest@[email protected]
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/fi-kbl-7567u/igt@i915_selftest@[email protected]

* igt@kms_busy@basic@flip:
- fi-kbl-7567u: [DMESG-WARN][5] ([i915#13735] / [i915#180]) -> [PASS][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/fi-kbl-7567u/igt@kms_busy@[email protected]
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/fi-kbl-7567u/igt@kms_busy@[email protected]

* igt@kms_pm_rpm@basic-pci-d3-state:
- fi-kbl-7567u: [DMESG-WARN][7] ([i915#13735] / [i915#15673] / [i915#180]) -> [PASS][8] +52 other tests pass
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/fi-kbl-7567u/igt@[email protected]
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/fi-kbl-7567u/igt@[email protected]

[i915#13735]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13735
[i915#15673]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15673
[i915#180]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/180

Build changes
-------------

* Linux: CI_DRM_19266 -> Patchwork_175398v1

CI-20190529: 20190529
CI_DRM_19266: fc5f5579dbfab13003fd4d6c1c9214a558f58c9d @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_9123: afd8cfb7aa15906974dd979b6dc739ed17ab9e33 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_175398v1: fc5f5579dbfab13003fd4d6c1c9214a558f58c9d @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/index.html

#2 lore raw

Re: ✗ i915.CI.BAT: failure for iommu/dma: Catch scatterlist length overflows

Hi Ci Infra Team,

The below failure is unrelated to this patch.

Quoted 88 lines · Patchwork

== Series Details ==

Series: iommu/dma: Catch scatterlist length overflows
URL : https://patchwork.freedesktop.org/series/175398/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_19266 -> Patchwork_175398v1
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with Patchwork_175398v1 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_175398v1, please notify your bug team ([email protected]) to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/index.html

Participating hosts (39 -> 37)
------------------------------

Missing (2): bat-dg2-13 fi-glk-j4005

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in Patchwork_175398v1:

### IGT changes ###

#### Possible regressions ####

* igt@core_hotunplug@unbind-rebind:
- bat-arlh-2: [PASS][1] -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/bat-arlh-2/igt@[email protected]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/bat-arlh-2/igt@[email protected]

Known issues
------------

Here are the changes found in Patchwork_175398v1 that come from known issues:

### IGT changes ###

#### Possible fixes ####

* igt@i915_selftest@live@sanitycheck:
- fi-kbl-7567u: [DMESG-WARN][3] ([i915#13735]) -> [PASS][4] +79 other tests pass
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/fi-kbl-7567u/igt@i915_selftest@[email protected]
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/fi-kbl-7567u/igt@i915_selftest@[email protected]

* igt@kms_busy@basic@flip:
- fi-kbl-7567u: [DMESG-WARN][5] ([i915#13735] / [i915#180]) -> [PASS][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/fi-kbl-7567u/igt@kms_busy@[email protected]
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/fi-kbl-7567u/igt@kms_busy@[email protected]

* igt@kms_pm_rpm@basic-pci-d3-state:
- fi-kbl-7567u: [DMESG-WARN][7] ([i915#13735] / [i915#15673] / [i915#180]) -> [PASS][8] +52 other tests pass
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19266/fi-kbl-7567u/igt@[email protected]
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/fi-kbl-7567u/igt@[email protected]

[i915#13735]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13735
[i915#15673]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15673
[i915#180]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/180

Build changes
-------------

* Linux: CI_DRM_19266 -> Patchwork_175398v1

CI-20190529: 20190529
CI_DRM_19266: fc5f5579dbfab13003fd4d6c1c9214a558f58c9d @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_9123: afd8cfb7aa15906974dd979b6dc739ed17ab9e33 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_175398v1: fc5f5579dbfab13003fd4d6c1c9214a558f58c9d @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_175398v1/index.html

Signature · 2 lines
Best Regards,
Krzysztof
#3 lore raw
In reply to Krzysztof Karas
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] iommu/dma: missed scatterlist length overflow on 32-bit architectures --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iommu/dma: missed scatterlist length overflow on 32-bit architectures --

commit c8808dcafa857db06b75cf791820b2414095b3cc
Author: Krzysztof Karas <[email protected]>

iommu/dma: Catch scatterlist length overflows

It is possible, when a very large mapping uses only one scatterlist, that padding overflows scatterlist's length field. Address this issue by adding overflow detection for scatterlist length field.

drivers/iommu/dma-iommu.c int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents, · line 1484

Quoted 13 lines
drivers/iommu/dma-iommu.c +5 -0
index 58c624513cd43..06edb4056dc1b 100644
@@ -1477,6 +1477,11 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 		sg_dma_len(s) = s_length;
 		s->offset -= s_iova_off;
 		s_length = iova_align(iovad, s_length + s_iova_off);
+		if (overflows_type(s_length, s->length)) {
+			ret = -EOVERFLOW;
+
+			goto out_restore_sg;
+		}

[Severity: Medium]
Does this catch scatterlist length wrapping on 32-bit architectures?

If a caller provides a scatterlist segment with a length close to 4GB on a 32-bit system, s_length and s_iova_off are both size_t. Their addition and the subsequent iova_align() will silently wrap around in 32-bit arithmetic before the overflows_type() check is reached.

Because s_length has already wrapped, overflows_type() will evaluate if the wrapped 32-bit value fits inside a 32-bit unsigned int (s->length), which always returns false.

In contrast, the subsequent pad_len check correctly uses check_add_overflow(), avoiding this pitfall. Could a similar approach be used here?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v1 1/1
#4 lore raw
In reply to Krzysztof Karas

Hi Krzysztof,

Quoted 10 lines · Krzysztof Karas

It is possible, when a very large mapping uses only one scatterlist, that padding overflows scatterlist's length field. This results in:
1) silently wrapping the value
2) smaller than desired mappings produced by iommu_map_sg
3) leaving mapped bytes in memory (no iommu_unmap)

Address this issue by adding overflow detection for scatterlist length field.

Quoted 6 lines
Fixes 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
Assisted-by LLM

---

Signature · 2 lines
Best regards,
Sebastian
maintainer Applied
#5 lore raw
In reply to Krzysztof Karas
Quoted 2 lines · Krzysztof Karas
 drivers/iommu/dma-iommu.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Applied, thanks.