| Message ID | 20230601031859.7115-1-akihiko.odaki@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: Update MemReentrancyGuard for NIC | expand |
On 1/6/23 05:18, Akihiko Odaki wrote: > Recently MemReentrancyGuard was added to DeviceState to record that the > device is engaging in I/O. The network device backend needs to update it > when delivering a packet to a device. > > This implementation follows what bottom half does, but it does not add > a tracepoint for the case that the network device backend started > delivering a packet to a device which is already engaging in I/O. This > is because such reentrancy frequently happens for > qemu_flush_queued_packets() and is insignificant. > > This series consists of two patches. The first patch makes a bulk change to > add a new parameter to qemu_new_nic() and does not contain behavioral changes. > The second patch actually implements MemReentrancyGuard update. /me look at the 'net' API. So the NetReceive* handlers from NetClientInfo process the HW NIC data flow, independently from the CPUs. IIUC MemReentrancyGuard is supposed to protect reentrancy abuse from CPUs. NetReceive* handlers aren't restricted to any particular API, they just consume blob of data. Looking at e1000_receive_iov(), this data is filled into memory using the pci_dma_rw() API. pci_dma_rw() gets the AddressSpace to use calling pci_get_address_space(), which returns PCIDevice::bus_master_as. Then we use the dma_memory_rw(), followed by address_space_rw(). Beh, I fail to see why there is reentrancy checks from this NIC DMA HW path. Maybe the MemoryRegion API isn't the correct place to check for reentrancy abuse and we should do that at the AddressSpace level, keeping DMA ASes clear and only protecting CPU ASes?
On 2023/06/01 16:16, Philippe Mathieu-Daudé wrote: > On 1/6/23 05:18, Akihiko Odaki wrote: >> Recently MemReentrancyGuard was added to DeviceState to record that the >> device is engaging in I/O. The network device backend needs to update it >> when delivering a packet to a device. >> >> This implementation follows what bottom half does, but it does not add >> a tracepoint for the case that the network device backend started >> delivering a packet to a device which is already engaging in I/O. This >> is because such reentrancy frequently happens for >> qemu_flush_queued_packets() and is insignificant. >> >> This series consists of two patches. The first patch makes a bulk >> change to >> add a new parameter to qemu_new_nic() and does not contain behavioral >> changes. >> The second patch actually implements MemReentrancyGuard update. > > /me look at the 'net' API. > > So the NetReceive* handlers from NetClientInfo process the HW NIC > data flow, independently from the CPUs. > > IIUC MemReentrancyGuard is supposed to protect reentrancy abuse from > CPUs. > > NetReceive* handlers aren't restricted to any particular API, they > just consume blob of data. Looking at e1000_receive_iov(), this data > is filled into memory using the pci_dma_rw() API. pci_dma_rw() gets > the AddressSpace to use calling pci_get_address_space(), which returns > PCIDevice::bus_master_as. Then we use the dma_memory_rw(), followed > by address_space_rw(). Beh, I fail to see why there is reentrancy > checks from this NIC DMA HW path. > > Maybe the MemoryRegion API isn't the correct place to check for > reentrancy abuse and we should do that at the AddressSpace level, > keeping DMA ASes clear and only protecting CPU ASes? The involvement of CPU is not essential in my understanding. A typical scenario of DMA reentrancy is like the following: 1) The guest configures the DMA destination address register the device has to the address of another device register. 2) The DMA gets triggered. 3) The device performs the DMA, writing its own register. 4) The write causes reentrancy. 5) The re-entered device code corrupts the device state. I guess 2) is done by CPU in most cases, but sometimes it happen with another cause. In fact, the current reentrancy protection code covers the case that bottom half handlers triggers DMA. The intention of this series is to extend the coverage and handles the case that incoming network traffic triggers DMA. The essence of DMA reentrancy is in 3). This happens when the DMA address space contains the MMIO region of the device and there is no involvement of CPU here.
On 1/6/23 09:41, Akihiko Odaki wrote: > On 2023/06/01 16:16, Philippe Mathieu-Daudé wrote: >> On 1/6/23 05:18, Akihiko Odaki wrote: >>> Recently MemReentrancyGuard was added to DeviceState to record that the >>> device is engaging in I/O. The network device backend needs to update it >>> when delivering a packet to a device. >>> >>> This implementation follows what bottom half does, but it does not add >>> a tracepoint for the case that the network device backend started >>> delivering a packet to a device which is already engaging in I/O. This >>> is because such reentrancy frequently happens for >>> qemu_flush_queued_packets() and is insignificant. >>> >>> This series consists of two patches. The first patch makes a bulk >>> change to >>> add a new parameter to qemu_new_nic() and does not contain behavioral >>> changes. >>> The second patch actually implements MemReentrancyGuard update. >> >> /me look at the 'net' API. >> >> So the NetReceive* handlers from NetClientInfo process the HW NIC >> data flow, independently from the CPUs. >> >> IIUC MemReentrancyGuard is supposed to protect reentrancy abuse from >> CPUs. >> >> NetReceive* handlers aren't restricted to any particular API, they >> just consume blob of data. Looking at e1000_receive_iov(), this data >> is filled into memory using the pci_dma_rw() API. pci_dma_rw() gets >> the AddressSpace to use calling pci_get_address_space(), which returns >> PCIDevice::bus_master_as. Then we use the dma_memory_rw(), followed >> by address_space_rw(). Beh, I fail to see why there is reentrancy >> checks from this NIC DMA HW path. >> >> Maybe the MemoryRegion API isn't the correct place to check for >> reentrancy abuse and we should do that at the AddressSpace level, >> keeping DMA ASes clear and only protecting CPU ASes? > > The involvement of CPU is not essential in my understanding. A typical > scenario of DMA reentrancy is like the following: > 1) The guest configures the DMA destination address register the device > has to the address of another device register. > 2) The DMA gets triggered. > 3) The device performs the DMA, writing its own register. > 4) The write causes reentrancy. > 5) The re-entered device code corrupts the device state. > > I guess 2) is done by CPU in most cases, but sometimes it happen with > another cause. In fact, the current reentrancy protection code covers > the case that bottom half handlers triggers DMA. The intention of this > series is to extend the coverage and handles the case that incoming > network traffic triggers DMA. > > The essence of DMA reentrancy is in 3). This happens when the DMA > address space contains the MMIO region of the device and there is no > involvement of CPU here. OK, thanks for the explanation.
On 2023/06/01 12:18, Akihiko Odaki wrote: > Recently MemReentrancyGuard was added to DeviceState to record that the > device is engaging in I/O. The network device backend needs to update it > when delivering a packet to a device. > > This implementation follows what bottom half does, but it does not add > a tracepoint for the case that the network device backend started > delivering a packet to a device which is already engaging in I/O. This > is because such reentrancy frequently happens for > qemu_flush_queued_packets() and is insignificant. > > This series consists of two patches. The first patch makes a bulk change to > add a new parameter to qemu_new_nic() and does not contain behavioral changes. > The second patch actually implements MemReentrancyGuard update. > > V1 -> V2: Added the 'Fixes: CVE-2023-3019' tag > > Akihiko Odaki (2): > net: Provide MemReentrancyGuard * to qemu_new_nic() > net: Update MemReentrancyGuard for NIC > > include/net/net.h | 2 ++ > hw/net/allwinner-sun8i-emac.c | 3 ++- > hw/net/allwinner_emac.c | 3 ++- > hw/net/cadence_gem.c | 3 ++- > hw/net/dp8393x.c | 3 ++- > hw/net/e1000.c | 3 ++- > hw/net/e1000e.c | 2 +- > hw/net/eepro100.c | 4 +++- > hw/net/etraxfs_eth.c | 3 ++- > hw/net/fsl_etsec/etsec.c | 3 ++- > hw/net/ftgmac100.c | 3 ++- > hw/net/i82596.c | 2 +- > hw/net/igb.c | 2 +- > hw/net/imx_fec.c | 2 +- > hw/net/lan9118.c | 3 ++- > hw/net/mcf_fec.c | 3 ++- > hw/net/mipsnet.c | 3 ++- > hw/net/msf2-emac.c | 3 ++- > hw/net/mv88w8618_eth.c | 3 ++- > hw/net/ne2000-isa.c | 3 ++- > hw/net/ne2000-pci.c | 3 ++- > hw/net/npcm7xx_emc.c | 3 ++- > hw/net/opencores_eth.c | 3 ++- > hw/net/pcnet.c | 3 ++- > hw/net/rocker/rocker_fp.c | 4 ++-- > hw/net/rtl8139.c | 3 ++- > hw/net/smc91c111.c | 3 ++- > hw/net/spapr_llan.c | 3 ++- > hw/net/stellaris_enet.c | 3 ++- > hw/net/sungem.c | 2 +- > hw/net/sunhme.c | 3 ++- > hw/net/tulip.c | 3 ++- > hw/net/virtio-net.c | 6 ++++-- > hw/net/vmxnet3.c | 2 +- > hw/net/xen_nic.c | 4 ++-- > hw/net/xgmac.c | 3 ++- > hw/net/xilinx_axienet.c | 3 ++- > hw/net/xilinx_ethlite.c | 3 ++- > hw/usb/dev-network.c | 3 ++- > net/net.c | 15 +++++++++++++++ > 40 files changed, 90 insertions(+), 41 deletions(-) > Hi Jason, Can you review this series? Regards, Akihiko Odaki
On Thu, Sep 21, 2023 at 3:16 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2023/06/01 12:18, Akihiko Odaki wrote: > > Recently MemReentrancyGuard was added to DeviceState to record that the > > device is engaging in I/O. The network device backend needs to update it > > when delivering a packet to a device. > > > > This implementation follows what bottom half does, but it does not add > > a tracepoint for the case that the network device backend started > > delivering a packet to a device which is already engaging in I/O. This > > is because such reentrancy frequently happens for > > qemu_flush_queued_packets() and is insignificant. > > > > This series consists of two patches. The first patch makes a bulk change to > > add a new parameter to qemu_new_nic() and does not contain behavioral changes. > > The second patch actually implements MemReentrancyGuard update. > > > > V1 -> V2: Added the 'Fixes: CVE-2023-3019' tag > > > > Akihiko Odaki (2): > > net: Provide MemReentrancyGuard * to qemu_new_nic() > > net: Update MemReentrancyGuard for NIC > > > > include/net/net.h | 2 ++ > > hw/net/allwinner-sun8i-emac.c | 3 ++- > > hw/net/allwinner_emac.c | 3 ++- > > hw/net/cadence_gem.c | 3 ++- > > hw/net/dp8393x.c | 3 ++- > > hw/net/e1000.c | 3 ++- > > hw/net/e1000e.c | 2 +- > > hw/net/eepro100.c | 4 +++- > > hw/net/etraxfs_eth.c | 3 ++- > > hw/net/fsl_etsec/etsec.c | 3 ++- > > hw/net/ftgmac100.c | 3 ++- > > hw/net/i82596.c | 2 +- > > hw/net/igb.c | 2 +- > > hw/net/imx_fec.c | 2 +- > > hw/net/lan9118.c | 3 ++- > > hw/net/mcf_fec.c | 3 ++- > > hw/net/mipsnet.c | 3 ++- > > hw/net/msf2-emac.c | 3 ++- > > hw/net/mv88w8618_eth.c | 3 ++- > > hw/net/ne2000-isa.c | 3 ++- > > hw/net/ne2000-pci.c | 3 ++- > > hw/net/npcm7xx_emc.c | 3 ++- > > hw/net/opencores_eth.c | 3 ++- > > hw/net/pcnet.c | 3 ++- > > hw/net/rocker/rocker_fp.c | 4 ++-- > > hw/net/rtl8139.c | 3 ++- > > hw/net/smc91c111.c | 3 ++- > > hw/net/spapr_llan.c | 3 ++- > > hw/net/stellaris_enet.c | 3 ++- > > hw/net/sungem.c | 2 +- > > hw/net/sunhme.c | 3 ++- > > hw/net/tulip.c | 3 ++- > > hw/net/virtio-net.c | 6 ++++-- > > hw/net/vmxnet3.c | 2 +- > > hw/net/xen_nic.c | 4 ++-- > > hw/net/xgmac.c | 3 ++- > > hw/net/xilinx_axienet.c | 3 ++- > > hw/net/xilinx_ethlite.c | 3 ++- > > hw/usb/dev-network.c | 3 ++- > > net/net.c | 15 +++++++++++++++ > > 40 files changed, 90 insertions(+), 41 deletions(-) > > > > Hi Jason, > > Can you review this series? For some reason it falls through the cracks. I've queued this for rc1. Thanks > > Regards, > Akihiko Odaki >