LWN: Comments on "A security fix briefly breaks DMA"

A security fix briefly breaks DMA

kpfleming — Thu, 07 Apr 2022 15:16:32 +0000

I'm not sure that's fair.

You tell a NIC it can receive a packet directly into a buffer. It does so. You don't know how large the packet was until you look into the buffer to find out.

The NIC could conceivably communicate the 'bytes written' amount in some other location outside the buffer, but that significantly complicates the data handling.

A security fix briefly breaks DMA

johill — Mon, 04 Apr 2022 15:33:57 +0000

Well in this case we're talking about swiotlb, so imagine driver 1 maps a buffer A and gets buffer A' in swiotlb, fills some data into it (perhaps it's a crypto driver and there's some key data). Then it unmaps because it's no longer in use. Some time later driver 2 maps a buffer B and also gets A' in the swiotlb shadow data, so now all of the stale data from driver 1 is exposed to driver 2's device for DMA.

A security fix briefly breaks DMA

imMute — Mon, 04 Apr 2022 14:51:38 +0000

>I suspect that is potentially the device or its driver that is re-using the buffer for further purposes?

This. It's fairly common for drivers to setup a ring of buffers that get reused. As for how the data can be "unrelated" imagine a NIC driver pulling packets off the device - each packet may very well be destined for entirely different processes, but they all use the same set of buffers over and over again.

A security fix briefly breaks DMA

taladar — Mon, 04 Apr 2022 08:17:14 +0000

That doesn't really matter, if you are the kernel and essentially want a design that works with all hardware you have to expect some terribly designed hardware too.

A security fix briefly breaks DMA

iabervon — Sun, 03 Apr 2022 17:40:40 +0000

That's what I thought, but how does the CPU know that none of the sync happened before the status word was written, with the copy of the status word happening after it was written? You'd need to make sure that the first thing that gets copied during the sync is the status word, and I'm not finding any documentation that the sync happens in any particular order.

I expect that the copy actually happens from low to high, and the status word is at the beginning, so it always works, but relies on a property of the core API that was never promised.

A security fix briefly breaks DMA

Flow — Sun, 03 Apr 2022 09:38:32 +0000

Great summary, as always. However

> It makes sense to zero the bounce buffer before setting it up, but DMA buffers are often used many times, and it is not normal to zero them between operations. So even a bounce buffer that was zeroed at the beginning may accumulate unrelated data and expose it to user space.

I would be thankful if someone could elaborate on the process that is causing unrelated data to accumulate. This is not immediately obvious to me. I suspect that is potentially the device or its driver that is re-using the buffer for further purposes?

A security fix briefly breaks DMA

dvrabel — Sun, 03 Apr 2022 08:54:03 +0000

The device writes the status word (with its RxDone bit) _after_ writing the packet and other metadata. Thus, if the CPU sees RxDone it knows all the other data has been written.

A security fix briefly breaks DMA

farnz — Sun, 03 Apr 2022 08:36:43 +0000

In the bounce buffer case, it can't. But on a lot of systems, especially those in WiFi APs, the DMA is happening into the CPU buffer directly (no bounce buffer), and there it's an optimization to say "hey, packet copied in, resync and continue".

A security fix briefly breaks DMA

iabervon — Sat, 02 Apr 2022 19:55:23 +0000

I don't see how the bounce buffer copy and the ongoing DMA avoid interacting in such a way that a_packet_is_ready() can return true when not all of the packet was in the bounce buffer in time to get copied out. It seems like you'd need a sync operation that let you specify the order that the bounce buffer is read in a way that wouldn't be optimal for a sync operation that was only reliable if the device was no longer writing when you called it.

A security fix briefly breaks DMA

khim — Sat, 02 Apr 2022 16:46:47 +0000

Because it wouldn't solve anything: DMA mechanism itself has no way to check how many bytes were actually written, only the driver can know that.

And using value passed along from the driver to make information unavailable to said driver… what would that achieve? The driver already can ignore the part that it doesn't need.

A security fix briefly breaks DMA

johill — Sat, 02 Apr 2022 12:49:58 +0000

Oh, it probably *shouldn't* be used in this case, practically speaking. But (and credit goes to Halil for quoting this to me in https://lore.kernel.org/r/20220327051502.63fde20a.pasic@linux.ibm.com) the documentation states:

        void
        dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle,
                                size_t size,
                                enum dma_data_direction direction)

        void
        dma_sync_single_for_device(struct device *dev, dma_addr_t dma_handle,
                                   size_t size,
                                   enum dma_data_direction direction)

        void
        dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
                            int nents,
                            enum dma_data_direction direction)

        void
        dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
                               int nents,
                               enum dma_data_direction direction)

Synchronise a single contiguous or scatter/gather mapping for the CPU
and device. With the sync_sg API, all the parameters must be the same
as those passed into the single mapping API. With the sync_single API,
you can use dma_handle and size parameters that aren't identical to
those passed into the single mapping API to do a partial sync.

Which, at least in our reading, seems to imply that for SG all must be the same, for single all but handle/size must be the same. Practically, that doesn't seem to be desired, and it's not even entirely clear what these should mean for DMA_BIDIRECTIONAL.

A security fix briefly breaks DMA

NYKevin — Sat, 02 Apr 2022 06:24:03 +0000

I would tend to expect that the vast majority of hardware falls into one of two cases:

* It's something like a NIC, and the data is eventually going to end up in some sort of userspace buffer (e.g. via read() or recv()). This implies that some part of the kernel is going to have to figure out the size at some point, because you can't leak extraneous data to userspace, so you might as well do it in the driver.
* It's something like a keyboard or capture card, and the data has a fixed or configurable size and/or format, which the driver should already know because it presumably configured the size/format in the first place.

Is there really a lot of variable-sized input hardware where the input *doesn't* eventually end up in userspace?

A security fix briefly breaks DMA

developer122 — Sat, 02 Apr 2022 03:20:55 +0000

This sounds a lot like terrible hardware design.

A security fix briefly breaks DMA

mb — Fri, 01 Apr 2022 18:32:26 +0000

Thanks for the additional insight.

But could you please elaborate, why DMA_BIDIRECTIONAL should be used? I don't quite get why.

A security fix briefly breaks DMA

mb — Fri, 01 Apr 2022 18:30:16 +0000

In most cases it's probably unknown how much the device actually wrote, unless the driver actually looks at the data written.

A security fix briefly breaks DMA

mario-campos — Fri, 01 Apr 2022 17:43:15 +0000

That actually made a lot of sense! THIS is why I read LWN :)

On the matter at hand, though... Why not just create an additional `size_t` parameter in the DMA API and only bounce/copy the necessary data (instead of the full buffer)?

Typo reports

corbet — Fri, 01 Apr 2022 16:59:41 +0000

Fixed. For future reference, please send typo reports to lwn@lwn.net so as to not clutter the comment stream.

A security fix briefly breaks DMA

larkey — Fri, 01 Apr 2022 16:56:28 +0000

There's a small typo in the index link of the article: "DIrect memory access" (note the capital "I").

Really great writeup!

A security fix briefly breaks DMA

johill — Fri, 01 Apr 2022 15:15:19 +0000

Couple of notes, having participated in the thread:

According to the documentation, the call(s) shouldn't even specify DMA_FROM_DEVICE, it/they should specify DMA_BIDIRECTIONAL as the buffer is documented that way. That would probably break ath9k too, haven't checked the exact code though.
According to Linus's thinking on it, however, the DMA_FROM_DEVICE actually makes sense ...
The ownership model doesn't work as documented, at least not for devices such as ath9k, since here the device is writing while the driver is reading (which you touch upon), but that implies sort of "dual ownership" or weaking the ownership to "write ownership" perhaps?
There was a whole sub-discussion about cache invalidate etc., e.g. with a reference to the ARC (I think?) documentation, which was also quite worthwhile IMHO.

In any case, for me one of the take-aways from the thread was that the documented DMA model is actually not applicable in practice (i.e. it's wrong), which might've been worth going into even if nobody has stepped up yet to try to fix it.

A security fix briefly breaks DMA

post-factum — Fri, 01 Apr 2022 14:46:34 +0000

I sincerely hoped LWN would make a nice summary of that LKML thread, and here we are :). Thank you a lot!