[Qemu-devel] [PATCH] eepro100: Don't allow guests to fail assertions

[Qemu-devel] [PATCH] eepro100: Don't allow guests to fail assertions

[Kevin Wolf]

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 <kwolf@redhat.com>
---
 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. */
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH] eepro100: Don't allow guests to fail assertions

[Stefan Weil]

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 <kwolf@redhat.com>
> ---
> 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

Re: [Qemu-devel] [PATCH] eepro100: Don't allow guests to fail assertions

[Kevin Wolf]

Am 23.09.2009 20:02, schrieb Stefan Weil:
> 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 <kwolf@redhat.com>
>> ---
>> hw/eepro100.c | 46 +++++++++++++++++++++++++++++++++++-----------
>> 1 files changed, 35 insertions(+), 11 deletions(-)
>>
> 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)?

Seems this is just what must happen when patches accumulate on the list
without being committed. I'll try to rebase on top of your patches.
Thanks for the git URL, saves me some work of applying everything manually.

Apart from that, have you had a look at the actual changes? In some
places I'm not completely sure what's the right thing to do. I just know
that assert is the wrong one.

Kevin

Re: [Qemu-devel] [PATCH] eepro100: Don't allow guests to fail assertions

[Kevin Wolf]

Am 23.09.2009 20:02, schrieb Stefan Weil:
> 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.

On second thought, your 10+ patches are only for master, I assume? Then
this patch should be applied in its current version to the stable
branch(es). I'll still rebase for master, of course.

Kevin

Re: [Qemu-devel] [PATCH] eepro100: Don't allow guests to fail assertions

[Stefan Weil]

Kevin Wolf schrieb:
> Am 23.09.2009 20:02, schrieb Stefan Weil:
>> 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.
>
> On second thought, your 10+ patches are only for master, I assume? Then
> this patch should be applied in its current version to the stable
> branch(es). I'll still rebase for master, of course.
>
> Kevin
>

The patches are needed for the stable branches as well because
they fix multicast, endianess and other issues which are really
very important for users who want to use this driver.

But patch integration through mailing lists and a handful of
bottle necks is very time consuming, so I don't dare to dream
of fixing the stable branches :-)

Stefan