| Message ID | 20220527161937.328754-2-alxndr@bu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix dma-reentrancy issues | expand |
Hi Alex, I don't know this code well enough to be certain, but is a flag sufficient here given the intent, or should it be using a more thread-safe method like a rwlock or condition variable? Maybe the device state structure is already protected at some level with a mutex - just not obvious to me from these changes... Thanks, Darren. On Friday, 2022-05-27 at 12:19:35 -04, Alexander Bulekov wrote: > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > This flag should be set/checked prior to calling a device's MemoryRegion > handlers, and set when device code initiates DMA. The purpose of this > flag is to prevent DMA reentrancy issues. E.g.: > sdhci pio -> dma write -> sdhci mmio > nvme bh -> dma write -> nvme mmio > > These issues have led to problems such as stack-exhaustion and > use-after-frees. > > Assumptions: > * Devices do not interact with their own PIO/MMIO memory-regions using > DMA. > > * There is now way for there to be multiple simultaneous accesses to a > device's PIO/MMIO memory-regions, or for multiple threads to perform > DMA accesses simultaneously on behalf of a single device. > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > --- > include/hw/qdev-core.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 92c3d65208..6474dc51fa 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -193,6 +193,9 @@ struct DeviceState { > int instance_id_alias; > int alias_required_for_version; > ResettableState reset; > + > + /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */ > + int engaged_in_io; > }; > > struct DeviceListener { > -- > 2.33.0
On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote: > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > This flag should be set/checked prior to calling a device's MemoryRegion > handlers, and set when device code initiates DMA. The purpose of this > flag is to prevent DMA reentrancy issues. E.g.: > sdhci pio -> dma write -> sdhci mmio > nvme bh -> dma write -> nvme mmio > > These issues have led to problems such as stack-exhaustion and > use-after-frees. > > Assumptions: > * Devices do not interact with their own PIO/MMIO memory-regions using > DMA. If you're trying to protect against malicious guest-controlled DMA operations, you can't assume that. The guest can program a DMA controller to DMA to its own MMIO register bank if it likes. > * There is now way for there to be multiple simultaneous accesses to a > device's PIO/MMIO memory-regions, or for multiple threads to perform > DMA accesses simultaneously on behalf of a single device. This one is generally true because device code runs with the iothread lock held. -- PMM
On 27.05.22 18:19, Alexander Bulekov wrote: > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > This flag should be set/checked prior to calling a device's MemoryRegion > handlers, and set when device code initiates DMA. The purpose of this > flag is to prevent DMA reentrancy issues. E.g.: > sdhci pio -> dma write -> sdhci mmio > nvme bh -> dma write -> nvme mmio > > These issues have led to problems such as stack-exhaustion and > use-after-frees. > > Assumptions: > * Devices do not interact with their own PIO/MMIO memory-regions using > DMA. > > * There is now way for there to be multiple simultaneous accesses to a > device's PIO/MMIO memory-regions, or for multiple threads to perform > DMA accesses simultaneously on behalf of a single device. > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> I think this patch should be squashed into the other ones, it doesn't make particular sense without any actual users.
On 220530 1219, Peter Maydell wrote: > On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote: > > > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > This flag should be set/checked prior to calling a device's MemoryRegion > > handlers, and set when device code initiates DMA. The purpose of this > > flag is to prevent DMA reentrancy issues. E.g.: > > sdhci pio -> dma write -> sdhci mmio > > nvme bh -> dma write -> nvme mmio > > > > These issues have led to problems such as stack-exhaustion and > > use-after-frees. > > > > Assumptions: > > * Devices do not interact with their own PIO/MMIO memory-regions using > > DMA. > > If you're trying to protect against malicious guest-controlled > DMA operations, you can't assume that. The guest can program > a DMA controller to DMA to its own MMIO register bank if it likes. If this is the case, then it seems the only way to fix this class of problems is to rewrite device code so that it is safe for re-entrancy. That seems to require significant upfront work to support behavior that is often not even specified. Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous, incomplete solution. Can we disable re-entracy by default, to fix the security issues, and allow device code to "opt-in" to re-entrancy? -Alex > > > * There is now way for there to be multiple simultaneous accesses to a > > device's PIO/MMIO memory-regions, or for multiple threads to perform > > DMA accesses simultaneously on behalf of a single device. > > This one is generally true because device code runs with > the iothread lock held. > > -- PMM
On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote: > > On 220530 1219, Peter Maydell wrote: > > On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote: > > > > > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > > This flag should be set/checked prior to calling a device's MemoryRegion > > > handlers, and set when device code initiates DMA. The purpose of this > > > flag is to prevent DMA reentrancy issues. E.g.: > > > sdhci pio -> dma write -> sdhci mmio > > > nvme bh -> dma write -> nvme mmio > > > > > > These issues have led to problems such as stack-exhaustion and > > > use-after-frees. > > > > > > Assumptions: > > > * Devices do not interact with their own PIO/MMIO memory-regions using > > > DMA. > > > > If you're trying to protect against malicious guest-controlled > > DMA operations, you can't assume that. The guest can program > > a DMA controller to DMA to its own MMIO register bank if it likes. > > If this is the case, then it seems the only way to fix this class of > problems is to rewrite device code so that it is safe for re-entrancy. > That seems to require significant upfront work to support behavior that > is often not even specified. > Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous, > incomplete solution. > > Can we disable re-entracy by default, to fix the security issues, and > allow device code to "opt-in" to re-entrancy? That's a different question, ie "are there legitimate cases where devices try to DMA to themselves?". I don't know the answer, but I suspect not. -- PMM
On 30/5/22 15:28, Peter Maydell wrote: > On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote: >> >> On 220530 1219, Peter Maydell wrote: >>> On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote: >>>> >>>> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. >>>> This flag should be set/checked prior to calling a device's MemoryRegion >>>> handlers, and set when device code initiates DMA. The purpose of this >>>> flag is to prevent DMA reentrancy issues. E.g.: >>>> sdhci pio -> dma write -> sdhci mmio >>>> nvme bh -> dma write -> nvme mmio >>>> >>>> These issues have led to problems such as stack-exhaustion and >>>> use-after-frees. >>>> >>>> Assumptions: >>>> * Devices do not interact with their own PIO/MMIO memory-regions using >>>> DMA. >>> >>> If you're trying to protect against malicious guest-controlled >>> DMA operations, you can't assume that. The guest can program >>> a DMA controller to DMA to its own MMIO register bank if it likes. >> >> If this is the case, then it seems the only way to fix this class of >> problems is to rewrite device code so that it is safe for re-entrancy. >> That seems to require significant upfront work to support behavior that >> is often not even specified. >> Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous, >> incomplete solution. >> >> Can we disable re-entracy by default, to fix the security issues, and >> allow device code to "opt-in" to re-entrancy? > > That's a different question, ie "are there legitimate cases where > devices try to DMA to themselves?". I don't know the answer, but > I suspect not. There is a niche where it might not be legitimate, but it is (ab)used and Paolo wants to keep such cases working. I already responded to Alexander here: https://lore.kernel.org/qemu-devel/380ea0e5-a006-c570-4ec8-d67e837547ee@redhat.com/
On 220530 1428, Peter Maydell wrote: > On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote: > > > > On 220530 1219, Peter Maydell wrote: > > > On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote: > > > > > > > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > > > This flag should be set/checked prior to calling a device's MemoryRegion > > > > handlers, and set when device code initiates DMA. The purpose of this > > > > flag is to prevent DMA reentrancy issues. E.g.: > > > > sdhci pio -> dma write -> sdhci mmio > > > > nvme bh -> dma write -> nvme mmio > > > > > > > > These issues have led to problems such as stack-exhaustion and > > > > use-after-frees. > > > > > > > > Assumptions: > > > > * Devices do not interact with their own PIO/MMIO memory-regions using > > > > DMA. > > > > > > If you're trying to protect against malicious guest-controlled > > > DMA operations, you can't assume that. The guest can program > > > a DMA controller to DMA to its own MMIO register bank if it likes. > > > > If this is the case, then it seems the only way to fix this class of > > problems is to rewrite device code so that it is safe for re-entrancy. > > That seems to require significant upfront work to support behavior that > > is often not even specified. > > Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous, > > incomplete solution. > > > > Can we disable re-entracy by default, to fix the security issues, and > > allow device code to "opt-in" to re-entrancy? > > That's a different question, ie "are there legitimate cases where > devices try to DMA to themselves?". I don't know the answer, but > I suspect not. Ah, I worded the assumption incorrectly. Should be * There is no valid use-case for a device to interact with its own PIO/MMIO memory-regions using DMA. > > -- PMM
On 220530 1539, Philippe Mathieu-Daudé wrote: > On 30/5/22 15:28, Peter Maydell wrote: > > On Mon, 30 May 2022 at 14:10, Alexander Bulekov <alxndr@bu.edu> wrote: > > > > > > On 220530 1219, Peter Maydell wrote: > > > > On Fri, 27 May 2022 at 17:19, Alexander Bulekov <alxndr@bu.edu> wrote: > > > > > > > > > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > > > > This flag should be set/checked prior to calling a device's MemoryRegion > > > > > handlers, and set when device code initiates DMA. The purpose of this > > > > > flag is to prevent DMA reentrancy issues. E.g.: > > > > > sdhci pio -> dma write -> sdhci mmio > > > > > nvme bh -> dma write -> nvme mmio > > > > > > > > > > These issues have led to problems such as stack-exhaustion and > > > > > use-after-frees. > > > > > > > > > > Assumptions: > > > > > * Devices do not interact with their own PIO/MMIO memory-regions using > > > > > DMA. > > > > > > > > If you're trying to protect against malicious guest-controlled > > > > DMA operations, you can't assume that. The guest can program > > > > a DMA controller to DMA to its own MMIO register bank if it likes. > > > > > > If this is the case, then it seems the only way to fix this class of > > > problems is to rewrite device code so that it is safe for re-entrancy. > > > That seems to require significant upfront work to support behavior that > > > is often not even specified. > > > Simply spot-fixing the fuzzer re-entracy bugs seems like a dangerous, > > > incomplete solution. > > > > > > Can we disable re-entracy by default, to fix the security issues, and > > > allow device code to "opt-in" to re-entrancy? > > > > That's a different question, ie "are there legitimate cases where > > devices try to DMA to themselves?". I don't know the answer, but > > I suspect not. > > There is a niche where it might not be legitimate, but it is (ab)used > and Paolo wants to keep such cases working. I already responded to > Alexander here: > https://lore.kernel.org/qemu-devel/380ea0e5-a006-c570-4ec8-d67e837547ee@redhat.com/ I'm not sure we confirmed that this is actually an example of a device performing DMA to its own MMIO. Unless I am missing something, the BLOAD example simply performs repeated writes to VRAM? That said video-related devices seem like possible candidates where such behavior is conceivable. But even in those cases, the memory regions would likely be ram/rom devices (which are excluded from the re-entrancy check).
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 92c3d65208..6474dc51fa 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -193,6 +193,9 @@ struct DeviceState { int instance_id_alias; int alias_required_for_version; ResettableState reset; + + /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */ + int engaged_in_io; }; struct DeviceListener {
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. This flag should be set/checked prior to calling a device's MemoryRegion handlers, and set when device code initiates DMA. The purpose of this flag is to prevent DMA reentrancy issues. E.g.: sdhci pio -> dma write -> sdhci mmio nvme bh -> dma write -> nvme mmio These issues have led to problems such as stack-exhaustion and use-after-frees. Assumptions: * Devices do not interact with their own PIO/MMIO memory-regions using DMA. * There is now way for there to be multiple simultaneous accesses to a device's PIO/MMIO memory-regions, or for multiple threads to perform DMA accesses simultaneously on behalf of a single device. Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- include/hw/qdev-core.h | 3 +++ 1 file changed, 3 insertions(+)