[Spice-devel,v2] red-parse-qxl: Check consistency of QXL_DRAW_COPY operations

Submitted by Francois Gouget on May 27, 2016, 7:59 a.m.

Details

Message ID E1b6Cfn-0006fL-0A@amboise
State New
Headers show

Not browsing as part of any series.

Patch hide | download patch | download mbox

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 0dafbef..d1f0ea7 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -682,6 +682,18 @@  static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
         return 1;
     }
     red_get_rect_ptr(&red->src_area, &qxl->src_area);
+    /* The source area should not extend outside the source bitmap or have
+     * swapped coordinates.
+     */
+    if (red->src_bitmap->descriptor.type == SPICE_IMAGE_TYPE_BITMAP &&
+        (red->src_area.left < 0 ||
+         red->src_area.left > red->src_area.right ||
+         red->src_area.right > red->src_bitmap->u.bitmap.x ||
+         red->src_area.top < 0 || red->src_area.top > red->src_area.bottom ||
+         red->src_area.bottom > red->src_bitmap->u.bitmap.y)) {
+        red_put_image(red->src_bitmap);
+        return 1;
+    }
     red->rop_descriptor  = qxl->rop_descriptor;
     red->scale_mode      = qxl->scale_mode;
     red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);

Comments

> 
> The source area should not extend outside the source bitmap, or have
> swapped coordinates.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  server/red-parse-qxl.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

I checked and if this function return error the resource is correctly
released.

> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 0dafbef..d1f0ea7 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -682,6 +682,18 @@ static int red_get_copy_ptr(RedMemSlotInfo *slots, int
> group_id,
>          return 1;
>      }
>      red_get_rect_ptr(&red->src_area, &qxl->src_area);
> +    /* The source area should not extend outside the source bitmap or have
> +     * swapped coordinates.
> +     */
> +    if (red->src_bitmap->descriptor.type == SPICE_IMAGE_TYPE_BITMAP &&

Why this check? I think should be valid for any kind of image, even
jpeg, lz or whatever.
Maybe would be worth to have a function to check any rect so
could be reused.

> +        (red->src_area.left < 0 ||
> +         red->src_area.left > red->src_area.right ||
> +         red->src_area.right > red->src_bitmap->u.bitmap.x ||
> +         red->src_area.top < 0 || red->src_area.top > red->src_area.bottom
> ||
> +         red->src_area.bottom > red->src_bitmap->u.bitmap.y)) {
> +        red_put_image(red->src_bitmap);

Mm... this make me think you didn't test the code.. this cause
a double free in the current code

> +        return 1;
> +    }
>      red->rop_descriptor  = qxl->rop_descriptor;
>      red->scale_mode      = qxl->scale_mode;
>      red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);

Frediano
> 
> > 
> > The source area should not extend outside the source bitmap, or have
> > swapped coordinates.
> > 
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> >  server/red-parse-qxl.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> 
> I checked and if this function return error the resource is correctly
> released.
> 
> > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > index 0dafbef..d1f0ea7 100644
> > --- a/server/red-parse-qxl.c
> > +++ b/server/red-parse-qxl.c
> > @@ -682,6 +682,18 @@ static int red_get_copy_ptr(RedMemSlotInfo *slots, int
> > group_id,
> >          return 1;
> >      }
> >      red_get_rect_ptr(&red->src_area, &qxl->src_area);
> > +    /* The source area should not extend outside the source bitmap or have
> > +     * swapped coordinates.
> > +     */
> > +    if (red->src_bitmap->descriptor.type == SPICE_IMAGE_TYPE_BITMAP &&
> 
> Why this check? I think should be valid for any kind of image, even
> jpeg, lz or whatever.
> Maybe would be worth to have a function to check any rect so
> could be reused.
> 

Sorry, the check is for the red->src_bitmap->u.bitmap access.

> > +        (red->src_area.left < 0 ||
> > +         red->src_area.left > red->src_area.right ||
> > +         red->src_area.right > red->src_bitmap->u.bitmap.x ||
> > +         red->src_area.top < 0 || red->src_area.top > red->src_area.bottom
> > ||
> > +         red->src_area.bottom > red->src_bitmap->u.bitmap.y)) {
> > +        red_put_image(red->src_bitmap);
> 
> Mm... this make me think you didn't test the code.. this cause
> a double free in the current code
> 
> > +        return 1;
> > +    }
> >      red->rop_descriptor  = qxl->rop_descriptor;
> >      red->scale_mode      = qxl->scale_mode;
> >      red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);
> 

Frediano
On Fri, 27 May 2016, Frediano Ziglio wrote:

> > 
> > The source area should not extend outside the source bitmap, or have
> > swapped coordinates.
> > 
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> >  server/red-parse-qxl.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> 
> I checked and if this function return error the resource is correctly
> released.

Yes. So the original patch was correct.


> > +        (red->src_area.left < 0 ||
> > +         red->src_area.left > red->src_area.right ||
> > +         red->src_area.right > red->src_bitmap->u.bitmap.x ||
> > +         red->src_area.top < 0 || red->src_area.top > red->src_area.bottom
> > ||
> > +         red->src_area.bottom > red->src_bitmap->u.bitmap.y)) {
> > +        red_put_image(red->src_bitmap);
> 
> Mm... this make me think you didn't test the code.. this cause
> a double free in the current code

I tested the original patch but I failed to retest the error condition 
after adding the red_put_image() call. I have now done that, found the 
double free, and so I recommend going back to the original patch.
> On Fri, 27 May 2016, Frediano Ziglio wrote:
> 
> > > 
> > > The source area should not extend outside the source bitmap, or have
> > > swapped coordinates.
> > > 
> > > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > > ---
> > >  server/red-parse-qxl.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > 
> > I checked and if this function return error the resource is correctly
> > released.
> 
> Yes. So the original patch was correct.
> 
> 
> > > +        (red->src_area.left < 0 ||
> > > +         red->src_area.left > red->src_area.right ||
> > > +         red->src_area.right > red->src_bitmap->u.bitmap.x ||
> > > +         red->src_area.top < 0 || red->src_area.top >
> > > red->src_area.bottom
> > > ||
> > > +         red->src_area.bottom > red->src_bitmap->u.bitmap.y)) {
> > > +        red_put_image(red->src_bitmap);
> > 
> > Mm... this make me think you didn't test the code.. this cause
> > a double free in the current code
> 
> I tested the original patch but I failed to retest the error condition
> after adding the red_put_image() call. I have now done that, found the
> double free, and so I recommend going back to the original patch.
> 

Can I suggest 

@@ -682,6 +682,20 @@ static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
         return 1;
     }
     red_get_rect_ptr(&red->src_area, &qxl->src_area);
+    /* The source area should not extend outside the source bitmap or have
+     * swapped coordinates.
+     */
+    if (red->src_area.left < 0 ||
+        red->src_area.left > red->src_area.right ||
+        red->src_area.top < 0 ||
+        red->src_area.top > red->src_area.bottom) {
+        return 1;
+    }
+    if (red->src_bitmap->descriptor.type == SPICE_IMAGE_TYPE_BITMAP &&
+        (red->src_area.right > red->src_bitmap->u.bitmap.x ||
+         red->src_area.bottom > red->src_bitmap->u.bitmap.y)) {
+        return 1;
+    }
     red->rop_descriptor  = qxl->rop_descriptor;
     red->scale_mode      = qxl->scale_mode;
     red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);

?

Frediano
On Wed, 1 Jun 2016, Frediano Ziglio wrote:
[...]
> Can I suggest 
> 
> @@ -682,6 +682,20 @@ static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
>          return 1;
>      }
>      red_get_rect_ptr(&red->src_area, &qxl->src_area);
> +    /* The source area should not extend outside the source bitmap or have
> +     * swapped coordinates.
> +     */
> +    if (red->src_area.left < 0 ||
> +        red->src_area.left > red->src_area.right ||
> +        red->src_area.top < 0 ||
> +        red->src_area.top > red->src_area.bottom) {
> +        return 1;
> +    }
> +    if (red->src_bitmap->descriptor.type == SPICE_IMAGE_TYPE_BITMAP &&
> +        (red->src_area.right > red->src_bitmap->u.bitmap.x ||
> +         red->src_area.bottom > red->src_bitmap->u.bitmap.y)) {
> +        return 1;
> +    }
>      red->rop_descriptor  = qxl->rop_descriptor;
>      red->scale_mode      = qxl->scale_mode;
>      red_get_qmask_ptr(slots, group_id, &red->mask, &qxl->mask, flags);
> 
> ?

Sure. As I said it's not clear to me what the exact rules are so I'm 
open to variants.

The other cases like SPICE_IMAGE_TYPE_SURFACE, SPICE_IMAGE_TYPE_JPEG, 
SPICE_IMAGE_TYPE_SURFACE probably also have their own x/y dimensions but 
these are not accessible. So I guess the end users code bits will have 
to be careful in these cases.

Interestingly SPICE_IMAGE_TYPE_QUIC is just a bunch of SpiceChunks so it 
does not have its own x/y dimensions. It does have a data_size field in 
bytes which we should maybe check if we knew the bpp.