rust-embedded / embedonomicon

How to bootstrap support for a no_std target
https://docs.rust-embedded.org/embedonomicon/
Apache License 2.0
206 stars 33 forks source link

The proposed DMA API is unsafe #64

Open teskje opened 4 years ago

teskje commented 4 years ago

It appears that the DMA API described in Chapter 8 - DMA is unsafe.

Consider this example. It is basically the motivating example from the "Immovable buffers" section, but with the complete read_exact API from the end of the chapter, including Pining and the 'static bound. The example shows that it is still possible to pass a stack-allocated array into this (supposedly safe) read_exact function, which means the DMA operation will corrupt the stack. All you need to do is wrap the array in a newtype and impl DerefMut for it.

I believe the root of the issue is a misunderstanding of the Pin API. Contrary to intuition, Pin does in most cases not pin the pointed-to data in memory: If the target type implements Unpin, Pin does nothing at all:

Many types are always freely movable, even when pinned, because they do not rely on having a stable address. This includes all the basic types (like bool, i32, and references) as well as types consisting solely of these types. Types that do not care about pinning implement the Unpin auto-trait, which cancels the effect of Pin<P>.

Basically all types we care about implement Unpin (I believe only self-referential types are supposed not to?). Which means Pining the buffer passed into the DMA doesn't help us make the code safe.

The DMA chapter also mentions the StableDeref trait as an alternative. As far as I see, using this would actually lead to a safe API, so we should change the chapter accordingly.

teskje commented 4 years ago

I thought about the use of StableDeref a bit more. The guarantees it provides with regard to DerefMut are a bit hard to grasp. For example, Vec implements StableDeref, even though it can re-allocate if you push to it. My understanding is that the stable address guarantee for DerefMut types only holds as long as you don't call &mut self methods on the type itself. But the documentation doesn't explicitly state that.

Let's take a step back. Here is what I gather are the minimum requirements a generic safe DMA buffer type has to fulfill:

  1. It must be a pointer. Otherwise the buffer would be part of the Transfer struct and move around with it on the stack, which we cannot allow (see 3).
  2. The referenced buffer must be valid (i.e. not freed) for the length of the transfer. Otherwise the compiler could re-use the buffer space for other data, while the DMA was still reading from/writing to it.
  3. The referenced buffer must be at a stable address for the length of the transfer. Same reason as 2. Additionally, data returned from DMA reads would be incomplete when the buffer moved in the meantime.

Requirements 2 and 3 must be true even if we take mem::forget into account: If the Transfer is mem::forgetten, we won't be able to stop the DMA since the destructor is not run. So the buffer needs to stay valid and stable afterwards.

Looking at the types StableDeref is implemented for out of the box, it looks like all of them fulfill the above requirements. So provided I'm not missing an essential requirement above, using StableDeref for DMA buffers should be fine. In fact, it seems to be exactly what we need in terms of guarantees.