commit: 395cbb149a7874a22ba7417cab583e4d61d7098f Author: Mike Pagano <mpagano <AT> gentoo <DOT> org> AuthorDate: Mon Mar 28 22:32:07 2022 +0000 Commit: Mike Pagano <mpagano <AT> gentoo <DOT> org> CommitDate: Mon Mar 28 22:32:07 2022 +0000 URL: https://gitweb.gentoo.org/proj/linux-patches.git/commit/?id=395cbb14
Revert swiotlb: rework fix info leak with DMA_FROM_DEVICE Signed-off-by: Mike Pagano <mpagano <AT> gentoo.org> 0000_README | 4 + ...rework-fix-info-leak-with-dma_from_device.patch | 187 +++++++++++++++++++++ 2 files changed, 191 insertions(+) diff --git a/0000_README b/0000_README index ccfb3dab..a4e3e3b3 100644 --- a/0000_README +++ b/0000_README @@ -131,6 +131,10 @@ Patch: 2400_mt76-mt7921e-fix-possible-probe-failure-after-reboot.patch From: https://patchwork.kernel.org/project/linux-wireless/patch/70e27cbc652cbdb78277b9c691a3a5ba02653afb.1641540175.git.obj...@gmail.com/ Desc: mt76: mt7921e: fix possible probe failure after reboot +Patch: 2410_revert-swiotlb-rework-fix-info-leak-with-dma_from_device.patch +From: https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git +Desc: Revert swiotlb: rework fix info leak with DMA_FROM_DEVICE + Patch: 2900_tmp513-Fix-build-issue-by-selecting-CONFIG_REG.patch From: https://bugs.gentoo.org/710790 Desc: tmp513 requies REGMAP_I2C to build. Select it by default in Kconfig. See bug #710790. Thanks to Phil Stracchino diff --git a/2410_revert-swiotlb-rework-fix-info-leak-with-dma_from_device.patch b/2410_revert-swiotlb-rework-fix-info-leak-with-dma_from_device.patch new file mode 100644 index 00000000..69476ab1 --- /dev/null +++ b/2410_revert-swiotlb-rework-fix-info-leak-with-dma_from_device.patch @@ -0,0 +1,187 @@ +From bddac7c1e02ba47f0570e494c9289acea3062cc1 Mon Sep 17 00:00:00 2001 +From: Linus Torvalds <[email protected]> +Date: Sat, 26 Mar 2022 10:42:04 -0700 +Subject: Revert "swiotlb: rework "fix info leak with DMA_FROM_DEVICE"" +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +From: Linus Torvalds <[email protected]> + +commit bddac7c1e02ba47f0570e494c9289acea3062cc1 upstream. + +This reverts commit aa6f8dcbab473f3a3c7454b74caa46d36cdc5d13. + +It turns out this breaks at least the ath9k wireless driver, and +possibly others. + +What the ath9k driver does on packet receive is to set up the DMA +transfer with: + + int ath_rx_init(..) + .. + bf->bf_buf_addr = dma_map_single(sc->dev, skb->data, + common->rx_bufsize, + DMA_FROM_DEVICE); + +and then the receive logic (through ath_rx_tasklet()) will fetch +incoming packets + + static bool ath_edma_get_buffers(..) + .. + dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr, + common->rx_bufsize, DMA_FROM_DEVICE); + + ret = ath9k_hw_process_rxdesc_edma(ah, rs, skb->data); + if (ret == -EINPROGRESS) { + /*let device gain the buffer again*/ + dma_sync_single_for_device(sc->dev, bf->bf_buf_addr, + common->rx_bufsize, DMA_FROM_DEVICE); + return false; + } + +and it's worth noting how that first DMA sync: + + dma_sync_single_for_cpu(..DMA_FROM_DEVICE); + +is there to make sure the CPU can read the DMA buffer (possibly by +copying it from the bounce buffer area, or by doing some cache flush). +The iommu correctly turns that into a "copy from bounce bufer" so that +the driver can look at the state of the packets. + +In the meantime, the device may continue to write to the DMA buffer, but +we at least have a snapshot of the state due to that first DMA sync. + +But that _second_ DMA sync: + + dma_sync_single_for_device(..DMA_FROM_DEVICE); + +is telling the DMA mapping that the CPU wasn't interested in the area +because the packet wasn't there. In the case of a DMA bounce buffer, +that is a no-op. + +Note how it's not a sync for the CPU (the "for_device()" part), and it's +not a sync for data written by the CPU (the "DMA_FROM_DEVICE" part). + +Or rather, it _should_ be a no-op. That's what commit aa6f8dcbab47 +broke: it made the code bounce the buffer unconditionally, and changed +the DMA_FROM_DEVICE to just unconditionally and illogically be +DMA_TO_DEVICE. + +[ Side note: purely within the confines of the swiotlb driver it wasn't + entirely illogical: The reason it did that odd DMA_FROM_DEVICE -> + DMA_TO_DEVICE conversion thing is because inside the swiotlb driver, + it uses just a swiotlb_bounce() helper that doesn't care about the + whole distinction of who the sync is for - only which direction to + bounce. + + So it took the "sync for device" to mean that the CPU must have been + the one writing, and thought it meant DMA_TO_DEVICE. ] + +Also note how the commentary in that commit was wrong, probably due to +that whole confusion, claiming that the commit makes the swiotlb code + + "bounce unconditionally (that is, also + when dir == DMA_TO_DEVICE) in order do avoid synchronising back stale + data from the swiotlb buffer" + +which is nonsensical for two reasons: + + - that "also when dir == DMA_TO_DEVICE" is nonsensical, as that was + exactly when it always did - and should do - the bounce. + + - since this is a sync for the device (not for the CPU), we're clearly + fundamentally not coping back stale data from the bounce buffers at + all, because we'd be copying *to* the bounce buffers. + +So that commit was just very confused. It confused the direction of the +synchronization (to the device, not the cpu) with the direction of the +DMA (from the device). + +Reported-and-bisected-by: Oleksandr Natalenko <[email protected]> +Reported-by: Olha Cherevyk <[email protected]> +Cc: Halil Pasic <[email protected]> +Cc: Christoph Hellwig <[email protected]> +Cc: Kalle Valo <[email protected]> +Cc: Robin Murphy <[email protected]> +Cc: Toke Høiland-Jørgensen <[email protected]> +Cc: Maxime Bizon <[email protected]> +Cc: Johannes Berg <[email protected]> +Signed-off-by: Linus Torvalds <[email protected]> +Signed-off-by: Greg Kroah-Hartman <[email protected]> +--- + Documentation/core-api/dma-attributes.rst | 8 ++++++++ + include/linux/dma-mapping.h | 8 ++++++++ + kernel/dma/swiotlb.c | 23 ++++++++--------------- + 3 files changed, 24 insertions(+), 15 deletions(-) + +--- a/Documentation/core-api/dma-attributes.rst ++++ b/Documentation/core-api/dma-attributes.rst +@@ -130,3 +130,11 @@ accesses to DMA buffers in both privileg + subsystem that the buffer is fully accessible at the elevated privilege + level (and ideally inaccessible or at least read-only at the + lesser-privileged levels). ++ ++DMA_ATTR_OVERWRITE ++------------------ ++ ++This is a hint to the DMA-mapping subsystem that the device is expected to ++overwrite the entire mapped size, thus the caller does not require any of the ++previous buffer contents to be preserved. This allows bounce-buffering ++implementations to optimise DMA_FROM_DEVICE transfers. +--- a/include/linux/dma-mapping.h ++++ b/include/linux/dma-mapping.h +@@ -62,6 +62,14 @@ + #define DMA_ATTR_PRIVILEGED (1UL << 9) + + /* ++ * This is a hint to the DMA-mapping subsystem that the device is expected ++ * to overwrite the entire mapped size, thus the caller does not require any ++ * of the previous buffer contents to be preserved. This allows ++ * bounce-buffering implementations to optimise DMA_FROM_DEVICE transfers. ++ */ ++#define DMA_ATTR_OVERWRITE (1UL << 10) ++ ++/* + * A dma_addr_t can hold any valid DMA or bus address for the platform. It can + * be given to a device to use as a DMA source or target. It is specific to a + * given device and there may be a translation between the CPU physical address +--- a/kernel/dma/swiotlb.c ++++ b/kernel/dma/swiotlb.c +@@ -627,14 +627,10 @@ phys_addr_t swiotlb_tbl_map_single(struc + for (i = 0; i < nr_slots(alloc_size + offset); i++) + mem->slots[index + i].orig_addr = slot_addr(orig_addr, i); + tlb_addr = slot_addr(mem->start, index) + offset; +- /* +- * When dir == DMA_FROM_DEVICE we could omit the copy from the orig +- * to the tlb buffer, if we knew for sure the device will +- * overwirte the entire current content. But we don't. Thus +- * unconditional bounce may prevent leaking swiotlb content (i.e. +- * kernel memory) to user-space. +- */ +- swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE); ++ if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) && ++ (!(attrs & DMA_ATTR_OVERWRITE) || dir == DMA_TO_DEVICE || ++ dir == DMA_BIDIRECTIONAL)) ++ swiotlb_bounce(dev, tlb_addr, mapping_size, DMA_TO_DEVICE); + return tlb_addr; + } + +@@ -701,13 +697,10 @@ void swiotlb_tbl_unmap_single(struct dev + void swiotlb_sync_single_for_device(struct device *dev, phys_addr_t tlb_addr, + size_t size, enum dma_data_direction dir) + { +- /* +- * Unconditional bounce is necessary to avoid corruption on +- * sync_*_for_cpu or dma_ummap_* when the device didn't overwrite +- * the whole lengt of the bounce buffer. +- */ +- swiotlb_bounce(dev, tlb_addr, size, DMA_TO_DEVICE); +- BUG_ON(!valid_dma_direction(dir)); ++ if (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL) ++ swiotlb_bounce(dev, tlb_addr, size, DMA_TO_DEVICE); ++ else ++ BUG_ON(dir != DMA_FROM_DEVICE); + } + + void swiotlb_sync_single_for_cpu(struct device *dev, phys_addr_t tlb_addr,
