[vbox-dev] Handling of race in Linux e1000 driver

[Sylvia Else]

There's a race condition in the Linux e1000 driver, in that it enables 
receiving on the interface before it initialises the buffer size and 
addresses. The window is short, of the order of a millisecond, and probably 
does little harm with real hardware.

Unfortunately, when in bridge networking mode, if there are any broadcast 
packets around, the VirtualBox DevE1000 module will try to send one during 
that window. The hardware specification is silent as to what the hardware 
should do in this situation, but the effect of the interaction between 
VirtualBox  and the driver is to render the network non-functional. The 
dependence on broadcast packets to trigger the failure is no doubt why many 
people have no problem with it.

While this is a bug in the driver, rather than VirtualBox, the latter could 
better handle the situation by discarding packets when the buffer size or 
addresses of zero rather than attempting (but ultimately failing) to 
deliver it.

I propose the following patch, which I provide under the MIT license. I 
have confirmed that this allows the network to function despite the error 
in the driver.


--- orig/VirtualBox-5.2.8/src/VBox/Devices/Network/DevE1000.cpp 2018-02-27 
03:02:05.000000000 +1100
+++ VirtualBox-5.2.8/src/VBox/Devices/Network/DevE1000.cpp      2018-03-12 
14:45:03.028671162 +1100
@@ -2449,7 +2449,36 @@
          PDMDevHlpPhysRead(pThis->CTX_SUFF(pDevIns), e1kDescAddr(RDBAH, 
RDBAL, RDH),
                            &desc, sizeof(desc));
  # endif /* !E1K_WITH_RXD_CACHE */
-        if (pDesc->u64BufAddr)
+        if (!pDesc->u64BufAddr || pThis->u16RxBSize == 0)
+        {
+            /*
+             * Some drivers misguidedly enable the receive function before 
completing
+             * initialisation of the buffer size and/or addresses, leading 
to a race which
+             * can prevent the network coming up properly if we don't 
handle it. If that's the
+             * case then drop the packet. But first, make sure we're not 
using a stale copy
+             * of the offending descriptor. (If the buffer size is zero, 
then this won't
+             * help, but it does no harm.)
+             */
+            pThis->iRxDCurrent = pThis->nRxDFetched = 0;
+            e1kRxDPrefetch(pThis);
+            pDesc = e1kRxDGet(pThis);
+            if(pDesc == NULL)
+            {
+                /* I doubt this can happen, since a buffer was determined 
to exist above. */
+                E1kLog(("%s Out of receive buffers, dropping the packet\n",
+                    pThis->szPrf));
+                break;
+            }
+
+            if(!pDesc->u64BufAddr || pThis->u16RxBSize == 0)
+            {
+                E1kLog(("%s Null or zero sized received buffer, dropping 
the packet\n",
+                    pThis->szPrf));
+                break;
+            }
+        }
+
+        /* Was previously a conditional block. */
          {
              /* Update descriptor */
              pDesc->status        = status;