Jason Wang <jasow...@redhat.com> 于2020年7月17日周五 上午11:10写道: > > > On 2020/7/17 上午12:14, Li Qiang wrote: > > Alexander Bulekov reported a UAF bug related e1000e packets send. > > > > -->https://bugs.launchpad.net/qemu/+bug/1886362 > > > > This is because the guest trigger a e1000e packet send and set the > > data's address to e1000e's MMIO address. So when the e1000e do DMA > > it will write the MMIO again and trigger re-entrancy and finally > > causes this UAF. > > > > Paolo suggested to use a bottom half whenever MMIO is doing complicate > > things in here: > > -->https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg03342.html > > > > Reference here: > > 'The easiest solution is to delay processing of descriptors to a bottom > > half whenever MMIO is doing something complicated. This is also better > > for latency because it will free the vCPU thread more quickly and leave > > the work to the I/O thread.' > > > I think several things were missed in this patch (take virtio-net as a > reference), do we need the following things: >
Thanks Jason, In fact I know this, I'm scared for touching this but I want to try. Thanks for your advice. > - Cancel the bh when VM is stopped. Ok. I think add a vm state change notifier for e1000e can address this. > - A throttle to prevent bh from executing too much timer? Ok, I think add a config timeout and add a timer in e1000e can address this. > - A flag to record whether or not this a pending tx (and migrate it?) Is just a flag enough? Could you explain more about the idea behind processing the virtio-net/e1000e using bh like this? For example, if the guest trigger a lot of packets send and if the bh is scheduled in IO thread. So will we lost packets? How we avoid this in virtio-net. Thanks, Li Qiang > > Thanks > > > > > > This patch fixes this UAF. > > > > Signed-off-by: Li Qiang <liq...@163.com> > > --- > > hw/net/e1000e_core.c | 25 +++++++++++++++++-------- > > hw/net/e1000e_core.h | 2 ++ > > 2 files changed, 19 insertions(+), 8 deletions(-) > > > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > > index bcd186cac5..6165b04b68 100644 > > --- a/hw/net/e1000e_core.c > > +++ b/hw/net/e1000e_core.c > > @@ -2423,32 +2423,27 @@ e1000e_set_dbal(E1000ECore *core, int index, > > uint32_t val) > > static void > > e1000e_set_tctl(E1000ECore *core, int index, uint32_t val) > > { > > - E1000E_TxRing txr; > > core->mac[index] = val; > > > > if (core->mac[TARC0] & E1000_TARC_ENABLE) { > > - e1000e_tx_ring_init(core, &txr, 0); > > - e1000e_start_xmit(core, &txr); > > + qemu_bh_schedule(core->tx[0].tx_bh); > > } > > > > if (core->mac[TARC1] & E1000_TARC_ENABLE) { > > - e1000e_tx_ring_init(core, &txr, 1); > > - e1000e_start_xmit(core, &txr); > > + qemu_bh_schedule(core->tx[1].tx_bh); > > } > > } > > > > static void > > e1000e_set_tdt(E1000ECore *core, int index, uint32_t val) > > { > > - E1000E_TxRing txr; > > int qidx = e1000e_mq_queue_idx(TDT, index); > > uint32_t tarc_reg = (qidx == 0) ? TARC0 : TARC1; > > > > core->mac[index] = val & 0xffff; > > > > if (core->mac[tarc_reg] & E1000_TARC_ENABLE) { > > - e1000e_tx_ring_init(core, &txr, qidx); > > - e1000e_start_xmit(core, &txr); > > + qemu_bh_schedule(core->tx[qidx].tx_bh); > > } > > } > > > > @@ -3322,6 +3317,16 @@ e1000e_vm_state_change(void *opaque, int running, > > RunState state) > > } > > } > > > > +static void e1000e_core_tx_bh(void *opaque) > > +{ > > + struct e1000e_tx *tx = opaque; > > + E1000ECore *core = tx->core; > > + E1000E_TxRing txr; > > + > > + e1000e_tx_ring_init(core, &txr, tx - &core->tx[0]); > > + e1000e_start_xmit(core, &txr); > > +} > > + > > void > > e1000e_core_pci_realize(E1000ECore *core, > > const uint16_t *eeprom_templ, > > @@ -3340,6 +3345,8 @@ e1000e_core_pci_realize(E1000ECore *core, > > for (i = 0; i < E1000E_NUM_QUEUES; i++) { > > net_tx_pkt_init(&core->tx[i].tx_pkt, core->owner, > > E1000E_MAX_TX_FRAGS, core->has_vnet); > > + core->tx[i].core = core; > > + core->tx[i].tx_bh = qemu_bh_new(e1000e_core_tx_bh, &core->tx[i]); > > } > > > > net_rx_pkt_init(&core->rx_pkt, core->has_vnet); > > @@ -3367,6 +3374,8 @@ e1000e_core_pci_uninit(E1000ECore *core) > > for (i = 0; i < E1000E_NUM_QUEUES; i++) { > > net_tx_pkt_reset(core->tx[i].tx_pkt); > > net_tx_pkt_uninit(core->tx[i].tx_pkt); > > + qemu_bh_delete(core->tx[i].tx_bh); > > + core->tx[i].tx_bh = NULL; > > } > > > > net_rx_pkt_uninit(core->rx_pkt); > > diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h > > index aee32f7e48..94ddc6afc2 100644 > > --- a/hw/net/e1000e_core.h > > +++ b/hw/net/e1000e_core.h > > @@ -77,6 +77,8 @@ struct E1000Core { > > unsigned char sum_needed; > > bool cptse; > > struct NetTxPkt *tx_pkt; > > + QEMUBH *tx_bh; > > + E1000ECore *core; > > } tx[E1000E_NUM_QUEUES]; > > > > struct NetRxPkt *rx_pkt; >