From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MqWBY-00039L-LP for qemu-devel@nongnu.org; Wed, 23 Sep 2009 14:03:24 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MqWBT-00037G-4u for qemu-devel@nongnu.org; Wed, 23 Sep 2009 14:03:23 -0400 Received: from [199.232.76.173] (port=52417 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MqWBS-00037A-M3 for qemu-devel@nongnu.org; Wed, 23 Sep 2009 14:03:18 -0400 Received: from moutng.kundenserver.de ([212.227.126.187]:50564) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MqWBR-00075o-S8 for qemu-devel@nongnu.org; Wed, 23 Sep 2009 14:03:18 -0400 Message-ID: <4ABA62D3.6030900@mail.berlios.de> Date: Wed, 23 Sep 2009 20:02:59 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] eepro100: Don't allow guests to fail assertions References: <1253720562-13104-1-git-send-email-kwolf@redhat.com> In-Reply-To: <1253720562-13104-1-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org Kevin Wolf schrieb: > The idea of using assert() for input validation is rather questionable. > Let's remove it from eepro100, so that guests need to find more > interesting > ways if they want to crash qemu. > > This patch replaces asserts that are directly dependent on > guest-accessible > data by other means of error handling. > > Signed-off-by: Kevin Wolf > --- > hw/eepro100.c | 46 +++++++++++++++++++++++++++++++++++----------- > 1 files changed, 35 insertions(+), 11 deletions(-) > > diff --git a/hw/eepro100.c b/hw/eepro100.c > index d3b7c3d..9099da3 100644 > --- a/hw/eepro100.c > +++ b/hw/eepro100.c > @@ -69,7 +69,7 @@ > > #define TRACE(flag, command) ((flag) ? (command) : (void)0) > > -#define missing(text) assert(!"feature is missing in this emulation: > " text) > +#define missing(text) fprintf(stderr, "eepro100: feature is missing > in this emulation: " text "\n") > > #define MAX_ETH_FRAME_SIZE 1514 > > @@ -662,6 +662,7 @@ static void eepro100_cu_command(EEPRO100State * s, > uint8_t val) > bool bit_s = ((command & 0x4000) != 0); > bool bit_i = ((command & 0x2000) != 0); > bool bit_nc = ((command & 0x0010) != 0); > + bool success = true; > //~ bool bit_sf = ((command & 0x0008) != 0); > uint16_t cmd = command & 0x0007; > s->cu_offset = le32_to_cpu(tx.link); > @@ -688,9 +689,17 @@ static void eepro100_cu_command(EEPRO100State * > s, uint8_t val) > logout > ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD count > %u\n", > tbd_array, tcb_bytes, tx.tbd_count); > - assert(!bit_nc); > + > + if (bit_nc) { > + missing("CmdTx: NC = 0"); > + success = false; > + break; > + } > //~ assert(!bit_sf); > - assert(tcb_bytes <= 2600); > + if (tcb_bytes > 2600) { > + logout("TCB byte count too large, using 2600\n"); > + tcb_bytes = 2600; > + } > /* Next assertion fails for local configuration. */ > //~ assert((tcb_bytes > 0) || (tbd_array != 0xffffffff)); > if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) { > @@ -722,7 +731,6 @@ static void eepro100_cu_command(EEPRO100State * s, > uint8_t val) > uint8_t tbd_count = 0; > if (device_supports_eTxCB(s) && !(s->configuration[6] & BIT(4))) { > /* Extended Flexible TCB. */ > - assert(tcb_bytes == 0); > for (; tbd_count < 2; tbd_count++) { > uint32_t tx_buffer_address = ldl_phys(tbd_address); > uint16_t tx_buffer_size = lduw_phys(tbd_address + 4); > @@ -771,9 +779,11 @@ static void eepro100_cu_command(EEPRO100State * > s, uint8_t val) > break; > default: > missing("undefined command"); > + success = false; > + break; > } > - /* Write new status (success). */ > - stw_phys(cb_address, status | 0x8000 | 0x2000); > + /* Write new status. */ > + stw_phys(cb_address, status | 0x8000 | (success ? 0x2000 : 0)); > if (bit_i) { > /* CU completed action. */ > eepro100_cx_interrupt(s); > @@ -1453,7 +1463,10 @@ static ssize_t nic_receive(VLANClientState *vc, > const uint8_t * buf, size_t size > { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; > > /* TODO: check multiple IA bit. */ > - assert(!(s->configuration[20] & BIT(6))); > + if (s->configuration[20] & BIT(6)) { > + missing("Multiple IA bit"); > + return -1; > + } > > if (s->configuration[8] & 0x80) { > /* CSMA is disabled. */ > @@ -1482,7 +1495,9 @@ static ssize_t nic_receive(VLANClientState *vc, > const uint8_t * buf, size_t size > /* Multicast frame. */ > logout("%p received multicast, len=%d\n", s, size); > /* TODO: check multicast all bit. */ > - assert(!(s->configuration[21] & BIT(3))); > + if (s->configuration[21] & BIT(3)) { > + missing("Multicast All bit"); > + } > int mcast_idx = compute_mcast_idx(buf); > if (!(s->mult[mcast_idx >> 3] & (1 << (mcast_idx & 7)))) { > return size; > @@ -1512,7 +1527,12 @@ static ssize_t nic_receive(VLANClientState *vc, > const uint8_t * buf, size_t size > offsetof(eepro100_rx_t, packet)); > uint16_t rfd_command = le16_to_cpu(rx.command); > uint16_t rfd_size = le16_to_cpu(rx.size); > - assert(size <= rfd_size); > + > + if (size > rfd_size) { > + logout("Receive buffer (%" PRId16 " bytes) too small for data " > + "(%zu bytes); data truncated\n", rfd_size, size); > + size = rfd_size; > + } > if (size < 64) { > rfd_status |= 0x0080; > } > @@ -1524,7 +1544,10 @@ static ssize_t nic_receive(VLANClientState *vc, > const uint8_t * buf, size_t size > /* Early receive interrupt not supported. */ > //~ eepro100_er_interrupt(s); > /* Receive CRC Transfer not supported. */ > - assert(!(s->configuration[18] & 4)); > + if (s->configuration[18] & 4) { > + missing("Receive CRC Transfer"); > + return -1; > + } > /* TODO: check stripping enable bit. */ > //~ assert(!(s->configuration[17] & 1)); > cpu_physical_memory_write(s->ru_base + s->ru_offset + > @@ -1534,7 +1557,8 @@ static ssize_t nic_receive(VLANClientState *vc, > const uint8_t * buf, size_t size > s->ru_offset = le32_to_cpu(rx.link); > if (rfd_command & 0x8000) { > /* EL bit is set, so this was the last frame. */ > - assert(0); > + logout("receive: Running out of frames\n"); > + set_ru_state(s, ru_suspended); > } > if (rfd_command & 0x4000) { > /* S bit is set. */ There are about 10 pending patches for eepro100.c, so the code at savannah is not a good base for new patches of eepro100.c. And there are even more patches in my queue to be sent as soon as the first 10 are integrated. git://repo.or.cz/qemu/ar7.git has my latest version of eepro100.c. Could you please send a patch based on that version (or just wait until the savannah version is up-to-date)? Regards Stefan