[Qemu-devel] [PATCH] xenfb.c: avoid expensive loops when prod <= out_cons

[Qemu-devel] [PATCH] xenfb.c: avoid expensive loops when prod <= out_cons

[Stefano Stabellini]

If the frontend sets out_cons to a value higher than out_prod, it will
cause xenfb_handle_events to loop about 2^32 times. Avoid that by using
better checks at the beginning of the function.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 4e2a27a..f963cf2 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -789,10 +789,11 @@ static void xenfb_handle_events(struct XenFB *xenfb)
 
     prod = page->out_prod;
     out_cons = page->out_cons;
-    if (prod == out_cons)
-	return;
+    if (prod <= out_cons) {
+        return;
+    }
     xen_rmb();		/* ensure we see ring contents up to prod */
-    for (cons = out_cons; cons != prod; cons++) {
+    for (cons = out_cons; cons < prod; cons++) {
 	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);
         uint8_t type = event->type;
 	int x, y, w, h;

Re: [Qemu-devel] [Xen-devel] [PATCH] xenfb.c: avoid expensive loops when prod <= out_cons

[David Vrabel]

On 06/01/16 12:08, Stefano Stabellini wrote:
> If the frontend sets out_cons to a value higher than out_prod, it will
> cause xenfb_handle_events to loop about 2^32 times. Avoid that by using
> better checks at the beginning of the function.

You can't use less than to compare prod and cons because they wrap.

You need to compare (prod - cons) against ring size (or similar) to
check for overflow.  See RING_REQUEST_PROD_OVERFLOW() etc.

David

Re: [Qemu-devel] [PATCH] xenfb.c: avoid expensive loops when prod <= out_cons

[Paul Durrant]

> -----Original Message-----
> From: qemu-devel-bounces+paul.durrant=citrix.com@nongnu.org
> [mailto:qemu-devel-bounces+paul.durrant=citrix.com@nongnu.org] On
> Behalf Of Stefano Stabellini
> Sent: 06 January 2016 12:08
> To: qemu-devel@nongnu.org
> Cc: liuling-it@360.cn; xen-devel@lists.xensource.com; Stefano Stabellini
> Subject: [Qemu-devel] [PATCH] xenfb.c: avoid expensive loops when prod
> <= out_cons
> 
> If the frontend sets out_cons to a value higher than out_prod, it will
> cause xenfb_handle_events to loop about 2^32 times. Avoid that by using
> better checks at the beginning of the function.
> 

What happens when out_prod wraps?

  Paul

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 4e2a27a..f963cf2 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -789,10 +789,11 @@ static void xenfb_handle_events(struct XenFB
> *xenfb)
> 
>      prod = page->out_prod;
>      out_cons = page->out_cons;
> -    if (prod == out_cons)
> -	return;
> +    if (prod <= out_cons) {
> +        return;
> +    }
>      xen_rmb();		/* ensure we see ring contents up to prod */
> -    for (cons = out_cons; cons != prod; cons++) {
> +    for (cons = out_cons; cons < prod; cons++) {
>  	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page,
> cons);
>          uint8_t type = event->type;
>  	int x, y, w, h;

Re: [Qemu-devel] [Xen-devel] [PATCH] xenfb.c: avoid expensive loops when prod <= out_cons

[Stefano Stabellini]

On Wed, 6 Jan 2016, David Vrabel wrote:
> On 06/01/16 12:08, Stefano Stabellini wrote:
> > If the frontend sets out_cons to a value higher than out_prod, it will
> > cause xenfb_handle_events to loop about 2^32 times. Avoid that by using
> > better checks at the beginning of the function.
> 
> You can't use less than to compare prod and cons because they wrap.
> 
> You need to compare (prod - cons) against ring size (or similar) to
> check for overflow.  See RING_REQUEST_PROD_OVERFLOW() etc.

Yes, you are right. I think that the right fix should be:


diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 4e2a27a..594baff 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -789,8 +789,9 @@ static void xenfb_handle_events(struct XenFB *xenfb)
 
     prod = page->out_prod;
     out_cons = page->out_cons;
-    if (prod == out_cons)
-	return;
+    if (prod - out_cons >= XENFB_OUT_RING_LEL) {
+        return;
+    }
     xen_rmb();		/* ensure we see ring contents up to prod */
     for (cons = out_cons; cons != prod; cons++) {
 	union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons);