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(-)

-- 
2.40.1

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
>

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.