| Message ID | 1453209440-16455-1-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/19/2016 09:17 PM, Laszlo Ersek wrote: > The start_xmit() and e1000_receive_iov() functions implement DMA transfers > iterating over a set of descriptors that the guest's e1000 driver > prepares: > > - the TDLEN and RDLEN registers store the total size of the descriptor > area, > > - while the TDH and RDH registers store the offset (in whole tx / rx > descriptors) into the area where the transfer is supposed to start. > > Each time a descriptor is processed, the TDH and RDH register is bumped > (as appropriate for the transfer direction). > > QEMU already contains logic to deal with bogus transfers submitted by the > guest: > > - Normally, the transmit case wants to increase TDH from its initial value > to TDT. (TDT is allowed to be numerically smaller than the initial TDH > value; wrapping at or above TDLEN bytes to zero is normal.) The failsafe > that QEMU currently has here is a check against reaching the original > TDH value again -- a complete wraparound, which should never happen. > > - In the receive case RDH is increased from its initial value until > "total_size" bytes have been received; preferably in a single step, or > in "s->rxbuf_size" byte steps, if the latter is smaller. However, null > RX descriptors are skipped without receiving data, while RDH is > incremented just the same. QEMU tries to prevent an infinite loop > (processing only null RX descriptors) by detecting whether RDH assumes > its original value during the loop. (Again, wrapping from RDLEN to 0 is > normal.) > > What both directions miss is that the guest could program TDLEN and RDLEN > so low, and the initial TDH and RDH so high, that these registers will > immediately be truncated to zero, and then never reassume their initial > values in the loop -- a full wraparound will never occur. > > The condition that expresses this is: > > xdh_start >= s->mac_reg[XDLEN] / sizeof(desc) > > i.e., TDH or RDH start out after the last whole rx or tx descriptor that > fits into the TDLEN or RDLEN sized area. > > This condition could be checked before we enter the loops, but > pci_dma_read() / pci_dma_write() knows how to fill in buffers safely for > bogus DMA addresses, so we just extend the existing failsafes with the > above condition. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Petr Matousek <pmatouse@redhat.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Prasad Pandit <ppandit@redhat.com> > Cc: Michael Roth <mdroth@linux.vnet.ibm.com> > Cc: Jason Wang <jasowang@redhat.com> > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1296044 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > Reviewed-by: Jason Wang <jasowang@redhat.com> > --- > > Notes: > Regarding the public posting: we made an honest effort to vet this > vulnerability, and the impact seems low -- no host side reads/writes, > "just" a DoS (infinite loop). We decided the patch could be posted > publicly, for the usual review process. Jason and Prasad checked the > patch in the internal discussion already, but comments, improvements > etc. are clearly welcome. The CVE request is underway. Thanks. > > hw/net/e1000.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index bec06e9..34d0823 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -908,7 +908,8 @@ start_xmit(E1000State *s) > * bogus values to TDT/TDLEN. > * there's nothing too intelligent we could do about this. > */ > - if (s->mac_reg[TDH] == tdh_start) { > + if (s->mac_reg[TDH] == tdh_start || > + tdh_start >= s->mac_reg[TDLEN] / sizeof(desc)) { > DBGOUT(TXERR, "TDH wraparound @%x, TDT %x, TDLEN %x\n", > tdh_start, s->mac_reg[TDT], s->mac_reg[TDLEN]); > break; > @@ -1165,7 +1166,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt) > if (++s->mac_reg[RDH] * sizeof(desc) >= s->mac_reg[RDLEN]) > s->mac_reg[RDH] = 0; > /* see comment in start_xmit; same here */ > - if (s->mac_reg[RDH] == rdh_start) { > + if (s->mac_reg[RDH] == rdh_start || > + rdh_start >= s->mac_reg[RDLEN] / sizeof(desc)) { > DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n", > rdh_start, s->mac_reg[RDT], s->mac_reg[RDLEN]); > set_ics(s, 0, E1000_ICS_RXO); Applied in my -net. Thanks
22.01.2016 06:09, Jason Wang wrote: > On 01/19/2016 09:17 PM, Laszlo Ersek wrote: >> The start_xmit() and e1000_receive_iov() functions implement DMA transfers >> iterating over a set of descriptors that the guest's e1000 driver >> prepares: ... > Applied in my -net. This is CVE-2016-1981, btw. /mjt
On 01/22/2016 02:11 PM, Michael Tokarev wrote: > 22.01.2016 06:09, Jason Wang wrote: >> On 01/19/2016 09:17 PM, Laszlo Ersek wrote: >>> The start_xmit() and e1000_receive_iov() functions implement DMA transfers >>> iterating over a set of descriptors that the guest's e1000 driver >>> prepares: > ... >> Applied in my -net. > This is CVE-2016-1981, btw. > > /mjt > Add this into commit log. Thanks
On 01/22/16 07:15, Jason Wang wrote: > > > On 01/22/2016 02:11 PM, Michael Tokarev wrote: >> 22.01.2016 06:09, Jason Wang wrote: >>> On 01/19/2016 09:17 PM, Laszlo Ersek wrote: >>>> The start_xmit() and e1000_receive_iov() functions implement DMA transfers >>>> iterating over a set of descriptors that the guest's e1000 driver >>>> prepares: >> ... >>> Applied in my -net. >> This is CVE-2016-1981, btw. >> >> /mjt >> > > Add this into commit log. Thanks guys! Laszlo
Hello Jason, On 01/22/16 07:15, Jason Wang wrote: > > > On 01/22/2016 02:11 PM, Michael Tokarev wrote: >> 22.01.2016 06:09, Jason Wang wrote: >>> On 01/19/2016 09:17 PM, Laszlo Ersek wrote: >>>> The start_xmit() and e1000_receive_iov() functions implement DMA transfers >>>> iterating over a set of descriptors that the guest's e1000 driver >>>> prepares: >> ... >>> Applied in my -net. >> This is CVE-2016-1981, btw. >> >> /mjt >> > > Add this into commit log. do you plan to send a PULL req soon? The patch is not really urgent, but it would help me move forward with my queue. Thanks! Laszlo
On 01/28/2016 02:35 AM, Laszlo Ersek wrote: > Hello Jason, > > On 01/22/16 07:15, Jason Wang wrote: >> >> On 01/22/2016 02:11 PM, Michael Tokarev wrote: >>> 22.01.2016 06:09, Jason Wang wrote: >>>> On 01/19/2016 09:17 PM, Laszlo Ersek wrote: >>>>> The start_xmit() and e1000_receive_iov() functions implement DMA transfers >>>>> iterating over a set of descriptors that the guest's e1000 driver >>>>> prepares: >>> ... >>>> Applied in my -net. >>> This is CVE-2016-1981, btw. >>> >>> /mjt >>> >> Add this into commit log. > do you plan to send a PULL req soon? The patch is not really urgent, but > it would help me move forward with my queue. > > Thanks! > Laszlo > Plan to send it next Tuesday. But if you wish, I can send it tomorrow. Thanks
On 01/28/16 06:47, Jason Wang wrote: > > > On 01/28/2016 02:35 AM, Laszlo Ersek wrote: >> Hello Jason, >> >> On 01/22/16 07:15, Jason Wang wrote: >>> >>> On 01/22/2016 02:11 PM, Michael Tokarev wrote: >>>> 22.01.2016 06:09, Jason Wang wrote: >>>>> On 01/19/2016 09:17 PM, Laszlo Ersek wrote: >>>>>> The start_xmit() and e1000_receive_iov() functions implement DMA transfers >>>>>> iterating over a set of descriptors that the guest's e1000 driver >>>>>> prepares: >>>> ... >>>>> Applied in my -net. >>>> This is CVE-2016-1981, btw. >>>> >>>> /mjt >>>> >>> Add this into commit log. >> do you plan to send a PULL req soon? The patch is not really urgent, but >> it would help me move forward with my queue. >> >> Thanks! >> Laszlo >> > > Plan to send it next Tuesday. But if you wish, I can send it tomorrow. Next Tuesday is perfectly fine, thank you! Laszlo
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index bec06e9..34d0823 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -908,7 +908,8 @@ start_xmit(E1000State *s) * bogus values to TDT/TDLEN. * there's nothing too intelligent we could do about this. */ - if (s->mac_reg[TDH] == tdh_start) { + if (s->mac_reg[TDH] == tdh_start || + tdh_start >= s->mac_reg[TDLEN] / sizeof(desc)) { DBGOUT(TXERR, "TDH wraparound @%x, TDT %x, TDLEN %x\n", tdh_start, s->mac_reg[TDT], s->mac_reg[TDLEN]); break; @@ -1165,7 +1166,8 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt) if (++s->mac_reg[RDH] * sizeof(desc) >= s->mac_reg[RDLEN]) s->mac_reg[RDH] = 0; /* see comment in start_xmit; same here */ - if (s->mac_reg[RDH] == rdh_start) { + if (s->mac_reg[RDH] == rdh_start || + rdh_start >= s->mac_reg[RDLEN] / sizeof(desc)) { DBGOUT(RXERR, "RDH wraparound @%x, RDT %x, RDLEN %x\n", rdh_start, s->mac_reg[RDT], s->mac_reg[RDLEN]); set_ics(s, 0, E1000_ICS_RXO);