diff mbox

e1000: eliminate infinite loops on out-of-bounds transfer start

Message ID 1453209440-16455-1-git-send-email-lersek@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Laszlo Ersek Jan. 19, 2016, 1:17 p.m. UTC 
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(-)

Comments

Jason Wang Jan. 22, 2016, 3:09 a.m. UTC | #1 
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
Michael Tokarev Jan. 22, 2016, 6:11 a.m. UTC | #2 
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
Jason Wang Jan. 22, 2016, 6:15 a.m. UTC | #3 
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
Laszlo Ersek Jan. 22, 2016, 9:09 a.m. UTC | #4 
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
Laszlo Ersek Jan. 27, 2016, 6:35 p.m. UTC | #5 
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
Jason Wang Jan. 28, 2016, 5:47 a.m. UTC | #6 
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
Laszlo Ersek Jan. 28, 2016, 11:09 a.m. UTC | #7 
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 mbox

Patch

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