[Qemu-devel] [PATCH] e1000: Avoid infinite loop in processing transmit descriptor

[Qemu-devel] [PATCH] e1000: Avoid infinite loop in processing transmit descriptor

[Stefan Hajnoczi]

From: P J P <pjp@fedoraproject.org>

While processing transmit descriptors, it could lead to an infinite
loop if 'bytes' was to become zero; Add a check to avoid it.

[The guest can force 'bytes' to 0 by setting the hdr_len and mss
descriptor fields to 0.
--Stefan]

Signed-off-by: P J P <pjp@fedoraproject.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/net/e1000.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 5c6bcd0..09c9e9d 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -740,7 +740,8 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
                 memmove(tp->data, tp->header, tp->hdr_len);
                 tp->size = tp->hdr_len;
             }
-        } while (split_size -= bytes);
+            split_size -= bytes;
+        } while (bytes && split_size);
     } else if (!tp->tse && tp->cptse) {
         // context descriptor TSE is not set, while data descriptor TSE is set
         DBGOUT(TXERR, "TCP segmentation error\n");
-- 
2.4.3

Re: [Qemu-devel] [PATCH] e1000: Avoid infinite loop in processing transmit descriptor

[Thomas Huth]

On 04/09/15 18:21, Stefan Hajnoczi wrote:
> From: P J P <pjp@fedoraproject.org>
> 
> While processing transmit descriptors, it could lead to an infinite
> loop if 'bytes' was to become zero; Add a check to avoid it.
> 
> [The guest can force 'bytes' to 0 by setting the hdr_len and mss
> descriptor fields to 0.
> --Stefan]

I wonder whether we should log an LOG_GUEST_ERROR in that case since
this sounds like a problem in the guest ... ?

> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> index 5c6bcd0..09c9e9d 100644
> --- a/hw/net/e1000.c
> +++ b/hw/net/e1000.c
> @@ -740,7 +740,8 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
>                  memmove(tp->data, tp->header, tp->hdr_len);
>                  tp->size = tp->hdr_len;
>              }
> -        } while (split_size -= bytes);
> +            split_size -= bytes;
> +        } while (bytes && split_size);
>      } else if (!tp->tse && tp->cptse) {
>          // context descriptor TSE is not set, while data descriptor TSE is set
>          DBGOUT(TXERR, "TCP segmentation error\n");

Looks sane ... (but IMHO code would be more readable though if it would
break out of the loop already earlier, as soon as it is clear that
bytes == 0, so that e.g. the pci_dma_read(..., 0) is not called at all).

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] e1000: Avoid infinite loop in processing transmit descriptor

[Stefan Hajnoczi]

On Mon, Sep 07, 2015 at 11:04:47AM +0200, Thomas Huth wrote:
> On 04/09/15 18:21, Stefan Hajnoczi wrote:
> > From: P J P <pjp@fedoraproject.org>
> > 
> > While processing transmit descriptors, it could lead to an infinite
> > loop if 'bytes' was to become zero; Add a check to avoid it.
> > 
> > [The guest can force 'bytes' to 0 by setting the hdr_len and mss
> > descriptor fields to 0.
> > --Stefan]
> 
> I wonder whether we should log an LOG_GUEST_ERROR in that case since
> this sounds like a problem in the guest ... ?

e1000.c seems to use DBGOUT(TXERR, ...) but doesn't do any
rate-limiting, which can pose a problem if a malicious guest keeps
triggering a warning message to fill up the disk on the host :(.

> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index 5c6bcd0..09c9e9d 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -740,7 +740,8 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc *dp)
> >                  memmove(tp->data, tp->header, tp->hdr_len);
> >                  tp->size = tp->hdr_len;
> >              }
> > -        } while (split_size -= bytes);
> > +            split_size -= bytes;
> > +        } while (bytes && split_size);
> >      } else if (!tp->tse && tp->cptse) {
> >          // context descriptor TSE is not set, while data descriptor TSE is set
> >          DBGOUT(TXERR, "TCP segmentation error\n");
> 
> Looks sane ... (but IMHO code would be more readable though if it would
> break out of the loop already earlier, as soon as it is clear that
> bytes == 0, so that e.g. the pci_dma_read(..., 0) is not called at all).

I agree.  P J P probably chose this approach because it is the smallest
change.  I'm happy enough as-is and you've posted your Reviewed-by too
so let's leave it.

Re: [Qemu-devel] [PATCH] e1000: Avoid infinite loop in processing transmit descriptor

[Stefan Hajnoczi]

On Fri, Sep 04, 2015 at 05:21:06PM +0100, Stefan Hajnoczi wrote:
> From: P J P <pjp@fedoraproject.org>
> 
> While processing transmit descriptors, it could lead to an infinite
> loop if 'bytes' was to become zero; Add a check to avoid it.
> 
> [The guest can force 'bytes' to 0 by setting the hdr_len and mss
> descriptor fields to 0.
> --Stefan]
> 
> Signed-off-by: P J P <pjp@fedoraproject.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/net/e1000.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Thanks, applied to my net tree:
https://github.com/stefanha/qemu/commits/net

Stefan

Re: [Qemu-devel] [PATCH] e1000: Avoid infinite loop in processing transmit descriptor

[pjp]

  Hello Stefan,

> On Wednesday, 9 September 2015 2:40 PM, Stefan Hajnoczi wrote:
> P J P probably chose this approach because it is the smallest
> change.

  True.

> On Wednesday, 9 September 2015 2:40 PM, Stefan Hajnoczi wrote:
>>   hw/net/e1000.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> Thanks, applied to my net tree:
> https://github.com/stefanha/qemu/commits/net


  Thank you so much, I appreciate it.


Thank you.

---Regards
   -P J P
http://feedmug.com