From 00837731d254908a841d69298a4f9f077babaf24 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Fri, 20 Nov 2015 08:42:33 +0100 Subject: [PATCH 1/3] eepro100: Prevent two endless loops http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg04592.html shows an example how an endless loop in function action_command can be achieved. During my code review, I noticed a 2nd case which can result in an endless loop. Reported-by: Qinghao Tang Signed-off-by: Stefan Weil Signed-off-by: Jason Wang --- hw/net/eepro100.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index 60333b7fcc..685a4781b9 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -774,6 +774,11 @@ static void tx_command(EEPRO100State *s) #if 0 uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6); #endif + if (tx_buffer_size == 0) { + /* Prevent an endless loop. */ + logout("loop in %s:%u\n", __FILE__, __LINE__); + break; + } tbd_address += 8; TRACE(RXTX, logout ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n", @@ -855,6 +860,10 @@ static void set_multicast_list(EEPRO100State *s) static void action_command(EEPRO100State *s) { + /* The loop below won't stop if it gets special handcrafted data. + Therefore we limit the number of iterations. */ + unsigned max_loop_count = 16; + for (;;) { bool bit_el; bool bit_s; @@ -870,6 +879,13 @@ static void action_command(EEPRO100State *s) #if 0 bool bit_sf = ((s->tx.command & COMMAND_SF) != 0); #endif + + if (max_loop_count-- == 0) { + /* Prevent an endless loop. */ + logout("loop in %s:%u\n", __FILE__, __LINE__); + break; + } + s->cu_offset = s->tx.link; TRACE(OTHER, logout("val=(cu start), status=0x%04x, command=0x%04x, link=0x%08x\n", From ee0428e3acd237e4d555cc54134cea473cab5ee7 Mon Sep 17 00:00:00 2001 From: Andrew Baumann Date: Wed, 18 Nov 2015 11:45:08 -0800 Subject: [PATCH 2/3] tap-win32: skip unexpected nodes during registry enumeration In order to find a named tap device, get_device_guid() enumerates children of HKLM\SYSTEM\CCS\Control\Network\{4D36E972-E325-11CE-BFC1-08002BE10318} (aka NETWORK_CONNECTIONS_KEY). For each child, it then looks for a "Connection" subkey, but if this key doesn't exist, it aborts the entire search. This was observed to fail on at least one Windows 10 machine, where there is an additional child of NETWORK_CONNECTIONS_KEY (named "Descriptions"). Since registry enumeration doesn't guarantee any particular sort order, we should continue to search for matching children rather than aborting the search. Signed-off-by: Andrew Baumann Reviewed-by: Stefan Weil Signed-off-by: Jason Wang --- net/tap-win32.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/tap-win32.c b/net/tap-win32.c index 4e2fa55006..5e5d6db880 100644 --- a/net/tap-win32.c +++ b/net/tap-win32.c @@ -356,7 +356,8 @@ static int get_device_guid( &len); if (status != ERROR_SUCCESS || name_type != REG_SZ) { - return -1; + ++i; + continue; } else { if (is_tap_win32_dev(enum_name)) { From b73c1849148da1229a3c3b336311a8194970b35f Mon Sep 17 00:00:00 2001 From: Andrew Baumann Date: Wed, 18 Nov 2015 11:45:09 -0800 Subject: [PATCH 3/3] tap-win32: disable broken async write path The code under the TUN_ASYNCHRONOUS_WRITES path makes two incorrect assumptions about the behaviour of the WriteFile API for overlapped file handles. First, WriteFile does not update the lpNumberOfBytesWritten parameter when the write completes asynchronously (the number of bytes written is known only when the operation completes). Second, the buffer shouldn't be touched (or freed) until the operation completes. This led to at least one bug where tap_win32_write returned zero bytes written, which in turn caused further writes ("receives") to be disabled for that device. This change disables the asynchronous write path, while keeping most of the code around in case someone sees value in resurrecting it. It also adds some conditional debug output, similar to the read path. Signed-off-by: Andrew Baumann Acked-by: Stefan Weil Signed-off-by: Jason Wang --- net/tap-win32.c | 46 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/net/tap-win32.c b/net/tap-win32.c index 5e5d6db880..7fddb20b51 100644 --- a/net/tap-win32.c +++ b/net/tap-win32.c @@ -77,7 +77,12 @@ //#define DEBUG_TAP_WIN32 -#define TUN_ASYNCHRONOUS_WRITES 1 +/* FIXME: The asynch write path appears to be broken at + * present. WriteFile() ignores the lpNumberOfBytesWritten parameter + * for overlapped writes, with the result we return zero bytes sent, + * and after handling a single packet, receive is disabled for this + * interface. */ +/* #define TUN_ASYNCHRONOUS_WRITES 1 */ #define TUN_BUFFER_SIZE 1560 #define TUN_MAX_BUFFER_COUNT 32 @@ -461,27 +466,48 @@ static int tap_win32_write(tap_win32_overlapped_t *overlapped, BOOL result; DWORD error; +#ifdef TUN_ASYNCHRONOUS_WRITES result = GetOverlappedResult( overlapped->handle, &overlapped->write_overlapped, &write_size, FALSE); if (!result && GetLastError() == ERROR_IO_INCOMPLETE) WaitForSingleObject(overlapped->write_event, INFINITE); +#endif result = WriteFile(overlapped->handle, buffer, size, &write_size, &overlapped->write_overlapped); +#ifdef TUN_ASYNCHRONOUS_WRITES + /* FIXME: we can't sensibly set write_size here, without waiting + * for the IO to complete! Moreover, we can't return zero, + * because that will disable receive on this interface, and we + * also can't assume it will succeed and return the full size, + * because that will result in the buffer being reclaimed while + * the IO is in progress. */ +#error Async writes are broken. Please disable TUN_ASYNCHRONOUS_WRITES. +#else /* !TUN_ASYNCHRONOUS_WRITES */ if (!result) { - switch (error = GetLastError()) - { - case ERROR_IO_PENDING: -#ifndef TUN_ASYNCHRONOUS_WRITES - WaitForSingleObject(overlapped->write_event, INFINITE); -#endif - break; - default: - return -1; + error = GetLastError(); + if (error == ERROR_IO_PENDING) { + result = GetOverlappedResult(overlapped->handle, + &overlapped->write_overlapped, + &write_size, TRUE); } } +#endif + + if (!result) { +#ifdef DEBUG_TAP_WIN32 + LPTSTR msgbuf; + error = GetLastError(); + FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM, + NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + &msgbuf, 0, NULL); + fprintf(stderr, "Tap-Win32: Error WriteFile %d - %s\n", error, msgbuf); + LocalFree(msgbuf); +#endif + return 0; + } return write_size; }