| Message ID | 20211215182421.418374-1-philmd@redhat.com |
|---|---|
| Headers | show |
| Series | physmem: Have flaview API check bus permission from MemTxAttrs argument | expand |
On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote: > This series aim to kill a recent class of bug, the infamous > "DMA reentrancy" issues found by Alexander while fuzzing. I took a look at how to protect DMA transactions in VIRTIO devices. It will require setting the MemTxAttrs for address_space_ld/st_le/be_cached calls. Errors on write (store) can be ignored. Errors on read (load) are a bit more questionable since the device performs some operation based on the loaded value, but at this point the driver has already caused the device to do something no correct driver does (as of today, it could change in the future...) so undefined device behavior might be okay. It would be easier to be confident if there was a single place to disable DMA re-entrancy for a device. The currently proposed API requires per-device code audits and fixes. It leaves decisions to the developer of each device. This will be a lot of work to fix and we cannot be confident that everything has been covered since this is an opt-in mechanism. For these reasons it seems likely that DMA re-entrancy issues will continue to creep in. I think the only way to rule out this class of bugs is to implement a centralized change that doesn't involve fixing every DMA access in QEMU. Thoughts? Stefan
On 220124 1630, Stefan Hajnoczi wrote: > On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote: > > This series aim to kill a recent class of bug, the infamous > > "DMA reentrancy" issues found by Alexander while fuzzing. > > I took a look at how to protect DMA transactions in VIRTIO devices. It > will require setting the MemTxAttrs for address_space_ld/st_le/be_cached > calls. Errors on write (store) can be ignored. Errors on read (load) are > a bit more questionable since the device performs some operation based > on the loaded value, but at this point the driver has already caused the > device to do something no correct driver does (as of today, it could > change in the future...) so undefined device behavior might be okay. > > It would be easier to be confident if there was a single place to > disable DMA re-entrancy for a device. The currently proposed API > requires per-device code audits and fixes. It leaves decisions to the > developer of each device. This will be a lot of work to fix and we > cannot be confident that everything has been covered since this is an > opt-in mechanism. > > For these reasons it seems likely that DMA re-entrancy issues will > continue to creep in. I think the only way to rule out this class of > bugs is to implement a centralized change that doesn't involve fixing > every DMA access in QEMU. > > Thoughts? Hi Stefan, Do you have some ideas about how to do this centrally? There were at least two attempts to do this in a centralized way, but it seems there is some worry that edge cases will break. However, I'm not sure there were any concrete examples of such breakages. [1] https://lore.kernel.org/all/20210824120153.altqys6jjiuxh35p@sirius.home.kraxel.org/ [2] https://lore.kernel.org/all/20211217030858.834822-1-alxndr@bu.edu/ (AFAIK Neither handles the BH->DMA->MMIO case, at the moment) -Alex > > Stefan
On Mon, Jan 24, 2022 at 11:50:10AM -0500, Alexander Bulekov wrote: > On 220124 1630, Stefan Hajnoczi wrote: > > On Wed, Dec 15, 2021 at 07:24:18PM +0100, Philippe Mathieu-Daudé wrote: > > > This series aim to kill a recent class of bug, the infamous > > > "DMA reentrancy" issues found by Alexander while fuzzing. > > > > I took a look at how to protect DMA transactions in VIRTIO devices. It > > will require setting the MemTxAttrs for address_space_ld/st_le/be_cached > > calls. Errors on write (store) can be ignored. Errors on read (load) are > > a bit more questionable since the device performs some operation based > > on the loaded value, but at this point the driver has already caused the > > device to do something no correct driver does (as of today, it could > > change in the future...) so undefined device behavior might be okay. > > > > It would be easier to be confident if there was a single place to > > disable DMA re-entrancy for a device. The currently proposed API > > requires per-device code audits and fixes. It leaves decisions to the > > developer of each device. This will be a lot of work to fix and we > > cannot be confident that everything has been covered since this is an > > opt-in mechanism. > > > > For these reasons it seems likely that DMA re-entrancy issues will > > continue to creep in. I think the only way to rule out this class of > > bugs is to implement a centralized change that doesn't involve fixing > > every DMA access in QEMU. > > > > Thoughts? > > Hi Stefan, > Do you have some ideas about how to do this centrally? > There were at least two attempts to do this in a centralized way, but it > seems there is some worry that edge cases will break. However, I'm > not sure there were any concrete examples of such breakages. > > [1] https://lore.kernel.org/all/20210824120153.altqys6jjiuxh35p@sirius.home.kraxel.org/ > [2] https://lore.kernel.org/all/20211217030858.834822-1-alxndr@bu.edu/ > (AFAIK Neither handles the BH->DMA->MMIO case, at the moment) Regressions are the problem with defaulting to RAM-only DMA. There's no way to avoid the risk if we change the default. On the other hand, it's the only way to squash this class of bugs - most existing devices just aren't written to cope with DMA re-entrancy. The approach in your patch sounds good to me, but I haven't followed the discussions so maybe there were valid reasons to look for alternatives. Stefan
This series aim to kill a recent class of bug, the infamous "DMA reentrancy" issues found by Alexander while fuzzing. Introduce the 'memory' field in MemTxAttrs, allowing to restrict a controller to memories (and not devices). If a transaction permission is not allowed (for example access to device), we return the specific MEMTX_BUS_ERROR. Permissions are checked in after the flatview is resolved, and before the access is done, in a new function: flatview_access_allowed(). Since v2 [1]: - Addressed review comments: - reword arm_gicv3 description (pm215) - merged patches 3/4/5 (peterx & dhildenb) - simplify flatview_access_allowed() logic (stefanha) - drop MEMTXPERM enum and reword following AMBA terminology (edgar) Since v1 ("hw: Forbid DMA write accesses to MMIO regions") [2]: - rewrite based on Peter / Stefan feedbacks Based on "hw: Let the DMA API take a MemTxAttrs argument" [3]. Based-on: <20210702092439.989969-1-philmd@redhat.com> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg831168.html [2] https://www.mail-archive.com/qemu-block@nongnu.org/msg72924.html [3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg820359.html Philippe Mathieu-Daudé (3): hw/intc/arm_gicv3: Check for !MEMTX_OK instead of MEMTX_ERROR softmmu/physmem: Simplify flatview_write and address_space_access_valid softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_BUS_ERROR include/exec/memattrs.h | 9 +++++++ hw/intc/arm_gicv3_redist.c | 4 +-- softmmu/physmem.c | 54 +++++++++++++++++++++++++++++++------- 3 files changed, 55 insertions(+), 12 deletions(-)