[Qemu-devel] [PATCH for-2.5] eepro100: Prevent two endless loops

[Qemu-devel] [PATCH for-2.5] eepro100: Prevent two endless loops

[Stefan Weil]

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 <luodalongde@gmail.com>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/net/eepro100.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 60333b7..685a478 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",
-- 
2.1.4

Re: [Qemu-devel] [PATCH for-2.5] eepro100: Prevent two endless loops

[P J P]

+-- On Fri, 20 Nov 2015, Stefan Weil wrote --+
| diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
| index 60333b7..685a478 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;
| +        }

   Yes, looks good. Where is 'lduw_le_pci_dma' routine defined?

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

   Is '16' a lower count for the command list? Earilier Jason mentioned 256. 
Not sure what is an ideal count.

Thank you.
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH for-2.5] eepro100: Prevent two endless loops

[Stefan Weil]

Am 20.11.2015 um 09:39 schrieb P J P:
> +-- On Fri, 20 Nov 2015, Stefan Weil wrote --+
> | diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> | index 60333b7..685a478 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;
> | +        }
>
>    Yes, looks good. Where is 'lduw_le_pci_dma' routine defined?

include/hw/pci/pci.h:    static inline uint##_bits##_t
ld##_l##_pci_dma(PCIDevice *dev,      \

>
> |  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;
> | +
>
>    Is '16' a lower count for the command list? Earilier Jason mentioned 256. 
> Not sure what is an ideal count.

Is there an ideal count? If it is too low, it might break some use cases.
If it is too high, it will take longer until the loop is finished.

I don't think EEPRO100 emulation is used in critical production
applications.
Therefore a lower value and a debug message when this value is exceeded
might be helpful to find out which lowest value is acceptable. If you want
to avoid this risk, the value should be set to 256, 10000, 65536 or any
other higher value. Feel free to change this when you apply the patch.

Stefan

Re: [Qemu-devel] [PATCH for-2.5] eepro100: Prevent two endless loops

[P J P]

+-- On Fri, 20 Nov 2015, Stefan Weil wrote --+
| include/hw/pci/pci.h:    static inline uint##_bits##_t
| ld##_l##_pci_dma(PCIDevice *dev,      \

   I see.
 
| Is there an ideal count? If it is too low, it might break some use cases.
| If it is too high, it will take longer until the loop is finished.

  -> https://url.corp.redhat.com/8255x-manual-pdf

  I tried to look trough the 8255x manual above, it does not have a specific 
value for the count, as it's a linked list of command blocks.

 
| I don't think EEPRO100 emulation is used in critical production 
| applications. Therefore a lower value and a debug message when this value is 
| exceeded might be helpful to find out which lowest value is acceptable. If 
| you want to avoid this risk, the value should be set to 256, 10000, 65536 or 
| any other higher value. Feel free to change this when you apply the patch.

  I guess Jason would be best to decide that.


Thank you.
--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH for-2.5] eepro100: Prevent two endless loops

[Jason Wang]

On 11/20/2015 07:27 PM, P J P wrote:
> +-- On Fri, 20 Nov 2015, Stefan Weil wrote --+
> | include/hw/pci/pci.h:    static inline uint##_bits##_t
> | ld##_l##_pci_dma(PCIDevice *dev,      \
>
>    I see.
>  
> | Is there an ideal count? If it is too low, it might break some use cases.
> | If it is too high, it will take longer until the loop is finished.
>
>   -> https://url.corp.redhat.com/8255x-manual-pdf
>
>   I tried to look trough the 8255x manual above, it does not have a specific 
> value for the count, as it's a linked list of command blocks.
>
>  
> | I don't think EEPRO100 emulation is used in critical production 
> | applications. Therefore a lower value and a debug message when this value is 
> | exceeded might be helpful to find out which lowest value is acceptable. If 
> | you want to avoid this risk, the value should be set to 256, 10000, 65536 or 
> | any other higher value. Feel free to change this when you apply the patch.
>
>   I guess Jason would be best to decide that.
>
>
> Thank you.
> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Apply the patch as is. We could enlarge the limitation if we find it was
too small in the future.

Thanks