Bug in subr_bus_dma.c's bus_dmamap_load() when multiple dma maps are created from a single tag on x86

[Hans Petter Selasky]

On 06/14/18 10:59, Pratyush Yadav wrote:
> Hi everyone,
> 
> I was looking at the busdma code for x86 and I notice that the claim
> in the bus_dma(9) man page might not be correct: "Multiple maps can be
> associated with one DMA tag."
> 
> Looking at the code of sys/x86/x86/busdma_bounce.c (which I believe is
> the default busdma implementation for x86), I realize that a map does
> not necessarily use all the segments of the dma tag (specified by the
> parameter nsegments on tag creation).
> 
> So how many segments does a map actually use? Looking at
> busdma_bounce.c:644,690 it seems we can calculate that from the
> pointer segp. It contains the index of starting segment on entrace to
> the load function, and the ending segment on exit.
> 
> Sounds all fine so far. But then if we look at map_complete()
> (busdma_bounce.c:908), it returns the entire segments array of the dma
> *tag*. I emphasize tag because the tag's segments array holds the
> segments of all the maps created with the tag.
> 
> Going a layer up, sys/kern/subr_bus_dma.c:328 simply passes nsegs(=
> -1) in the segp parameter to load, not bothering to check if some
> segments have been used already by other maps. It then calculates the
> number of segments used using the value of nsegs after return and
> calls map_complete() to get the segments array. It then passes the
> segments array to the driver callback with the number of segments,
> nsegs.
> 
> Effectively, to the driver it seems that the map's segments start from
> index 0 and go till nsegs - 1. This is not true if another map has
> been loaded in the meantime with the same tag.
> 
> We are possibly over-writing the previous map's segments.
> busdma_bounce.c:669 simply uses segs[0] if the value of segp is -1,
> which is what bus_dmamap_load() in subr_bus_dma.c:328 passes to
> _bus_dmamap_load_buffer(). segs[0] would also be used by any
> subsequent map's load. This means the previous map's values are
> over-written.
> 
> This might cause problems when the driver is in its callback using the
> segments array and another map is loaded.
> 
> Unfortunately, I do not posses the knowledge or experience with
> FreeBSD's drivers (drivers in general) to test for this bug.
> 
> I have not looked at the code in other architectures. The same problem
> might present itself there as well.
> 
> If I made a mistake somewhere in my reasoning, let me know.


Hi,

Use of bus_dmamap_load() on the same tag must be serialized using a 
mutex. Maybe it is not so clearly documented. Else like you see the 
temporary segs[] list may be overwritten.

--HPS