[Spice-devel] server/red_parse_qxl: add bitmap consistency check

Submitted by Alon Levy on Sept. 3, 2012, 4:24 p.m.

Details

Message ID 1346689469-2339-1-git-send-email-alevy@redhat.com
State Accepted
Commit 1c4e315e3e8261522d4944a75e4ca3917c505d2b
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy Sept. 3, 2012, 4:24 p.m.
Just checks stride vs width times bpp.

This fixes a potential abort on guest generated bad images in
glz_encoder.

Other files touched to move some consts to red_common, they are
static so no problem to be defined in both red_worker.c and
red_parse_qxl.c .
---
 server/red_common.h    | 18 +++++++++++++++++-
 server/red_parse_qxl.c | 17 +++++++++++++++++
 server/red_worker.c    | 15 ---------------
 3 files changed, 34 insertions(+), 16 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red_common.h b/server/red_common.h
index c563bc0..585b13c 100644
--- a/server/red_common.h
+++ b/server/red_common.h
@@ -25,6 +25,7 @@ 
 #include "common/mem.h"
 #include "common/spice_common.h"
 #include "common/messages.h"
+#include "common/lz_common.h"
 
 #include "spice.h"
 
@@ -35,13 +36,28 @@  enum {
     STREAM_VIDEO_FILTER
 };
 
+static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
+    LZ_IMAGE_TYPE_INVALID,
+    LZ_IMAGE_TYPE_PLT1_LE,
+    LZ_IMAGE_TYPE_PLT1_BE,
+    LZ_IMAGE_TYPE_PLT4_LE,
+    LZ_IMAGE_TYPE_PLT4_BE,
+    LZ_IMAGE_TYPE_PLT8,
+    LZ_IMAGE_TYPE_RGB16,
+    LZ_IMAGE_TYPE_RGB24,
+    LZ_IMAGE_TYPE_RGB32,
+    LZ_IMAGE_TYPE_RGBA,
+    LZ_IMAGE_TYPE_A8
+};
+
 static inline int bitmap_fmt_is_rgb(uint8_t fmt)
 {
     static const int BITMAP_FMT_IS_RGB[SPICE_BITMAP_FMT_ENUM_END] =
                                         {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1};
 
     if (fmt >= SPICE_BITMAP_FMT_ENUM_END) {
-        spice_warning("fmt >= SPICE_BITMAP_FMT_ENUM_END");
+        spice_warning("fmt >= SPICE_BITMAP_FMT_ENUM_END; %d >= %d",
+                      fmt, SPICE_BITMAP_FMT_ENUM_END);
         return 0;
     }
     return BITMAP_FMT_IS_RGB[fmt];
diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
index bf57709..b893add 100644
--- a/server/red_parse_qxl.c
+++ b/server/red_parse_qxl.c
@@ -21,6 +21,7 @@ 
 
 #include <stdbool.h>
 #include <inttypes.h>
+#include "common/lz_common.h"
 #include "red_common.h"
 #include "red_memslots.h"
 #include "red_parse_qxl.h"
@@ -327,6 +328,19 @@  static SpiceChunks *red_get_image_data_chunked(RedMemSlotInfo *slots, int group_
     return data;
 }
 
+static int bitmap_consistent(SpiceBitmap *bitmap)
+{
+    int type = MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[bitmap->format];
+    int bpp = RGB_BYTES_PER_PIXEL[type];
+
+    if (bitmap->stride < bitmap->x * bpp) {
+        spice_error("image stride too small for width: %d < %d * %d\n",
+                    bitmap->stride, bitmap->x, bpp);
+        return FALSE;
+    }
+    return TRUE;
+}
+
 // This is based on SPICE_BITMAP_FMT_*, copied from server/red_worker.c
 // to avoid a possible unoptimization from making it non static.
 static const int BITMAP_FMT_IS_RGB[] = {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1};
@@ -382,6 +396,9 @@  static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
         red->u.bitmap.x      = qxl->bitmap.x;
         red->u.bitmap.y      = qxl->bitmap.y;
         red->u.bitmap.stride = qxl->bitmap.stride;
+        if (!bitmap_consistent(&red->u.bitmap)) {
+            goto error;
+        }
         if (qxl->bitmap.palette) {
             QXLPalette *qp;
             int i, num_ents;
diff --git a/server/red_worker.c b/server/red_worker.c
index 133ba94..eee9915 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -5887,20 +5887,6 @@  static inline int _stride_is_extra(SpiceBitmap *bitmap)
     return 0;
 }
 
-static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
-    LZ_IMAGE_TYPE_INVALID,
-    LZ_IMAGE_TYPE_PLT1_LE,
-    LZ_IMAGE_TYPE_PLT1_BE,
-    LZ_IMAGE_TYPE_PLT4_LE,
-    LZ_IMAGE_TYPE_PLT4_BE,
-    LZ_IMAGE_TYPE_PLT8,
-    LZ_IMAGE_TYPE_RGB16,
-    LZ_IMAGE_TYPE_RGB24,
-    LZ_IMAGE_TYPE_RGB32,
-    LZ_IMAGE_TYPE_RGBA,
-    LZ_IMAGE_TYPE_A8
-};
-
 typedef struct compress_send_data_t {
     void*    comp_buf;
     uint32_t comp_buf_size;
@@ -5908,7 +5894,6 @@  typedef struct compress_send_data_t {
     int is_lossy;
 } compress_send_data_t;
 
-
 static inline int red_glz_compress_image(DisplayChannelClient *dcc,
                                          SpiceImage *dest, SpiceBitmap *src, Drawable *drawable,
                                          compress_send_data_t* o_comp_data)

Comments

Looks good, aCK.

On 09/03/2012 06:24 PM, Alon Levy wrote:
> Just checks stride vs width times bpp.
>
> This fixes a potential abort on guest generated bad images in
> glz_encoder.
>
> Other files touched to move some consts to red_common, they are
> static so no problem to be defined in both red_worker.c and
> red_parse_qxl.c .
> ---
>   server/red_common.h    | 18 +++++++++++++++++-
>   server/red_parse_qxl.c | 17 +++++++++++++++++
>   server/red_worker.c    | 15 ---------------
>   3 files changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/server/red_common.h b/server/red_common.h
> index c563bc0..585b13c 100644
> --- a/server/red_common.h
> +++ b/server/red_common.h
> @@ -25,6 +25,7 @@
>   #include "common/mem.h"
>   #include "common/spice_common.h"
>   #include "common/messages.h"
> +#include "common/lz_common.h"
>
>   #include "spice.h"
>
> @@ -35,13 +36,28 @@ enum {
>       STREAM_VIDEO_FILTER
>   };
>
> +static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
> +    LZ_IMAGE_TYPE_INVALID,
> +    LZ_IMAGE_TYPE_PLT1_LE,
> +    LZ_IMAGE_TYPE_PLT1_BE,
> +    LZ_IMAGE_TYPE_PLT4_LE,
> +    LZ_IMAGE_TYPE_PLT4_BE,
> +    LZ_IMAGE_TYPE_PLT8,
> +    LZ_IMAGE_TYPE_RGB16,
> +    LZ_IMAGE_TYPE_RGB24,
> +    LZ_IMAGE_TYPE_RGB32,
> +    LZ_IMAGE_TYPE_RGBA,
> +    LZ_IMAGE_TYPE_A8
> +};
> +
>   static inline int bitmap_fmt_is_rgb(uint8_t fmt)
>   {
>       static const int BITMAP_FMT_IS_RGB[SPICE_BITMAP_FMT_ENUM_END] =
>                                           {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1};
>
>       if (fmt >= SPICE_BITMAP_FMT_ENUM_END) {
> -        spice_warning("fmt >= SPICE_BITMAP_FMT_ENUM_END");
> +        spice_warning("fmt >= SPICE_BITMAP_FMT_ENUM_END; %d >= %d",
> +                      fmt, SPICE_BITMAP_FMT_ENUM_END);
>           return 0;
>       }
>       return BITMAP_FMT_IS_RGB[fmt];
> diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
> index bf57709..b893add 100644
> --- a/server/red_parse_qxl.c
> +++ b/server/red_parse_qxl.c
> @@ -21,6 +21,7 @@
>
>   #include <stdbool.h>
>   #include <inttypes.h>
> +#include "common/lz_common.h"
>   #include "red_common.h"
>   #include "red_memslots.h"
>   #include "red_parse_qxl.h"
> @@ -327,6 +328,19 @@ static SpiceChunks *red_get_image_data_chunked(RedMemSlotInfo *slots, int group_
>       return data;
>   }
>
> +static int bitmap_consistent(SpiceBitmap *bitmap)
> +{
> +    int type = MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[bitmap->format];
> +    int bpp = RGB_BYTES_PER_PIXEL[type];
> +
> +    if (bitmap->stride < bitmap->x * bpp) {
> +        spice_error("image stride too small for width: %d < %d * %d\n",
> +                    bitmap->stride, bitmap->x, bpp);
> +        return FALSE;
> +    }
> +    return TRUE;
> +}
> +
>   // This is based on SPICE_BITMAP_FMT_*, copied from server/red_worker.c
>   // to avoid a possible unoptimization from making it non static.
>   static const int BITMAP_FMT_IS_RGB[] = {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1};
> @@ -382,6 +396,9 @@ static SpiceImage *red_get_image(RedMemSlotInfo *slots, int group_id,
>           red->u.bitmap.x      = qxl->bitmap.x;
>           red->u.bitmap.y      = qxl->bitmap.y;
>           red->u.bitmap.stride = qxl->bitmap.stride;
> +        if (!bitmap_consistent(&red->u.bitmap)) {
> +            goto error;
> +        }
>           if (qxl->bitmap.palette) {
>               QXLPalette *qp;
>               int i, num_ents;
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 133ba94..eee9915 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -5887,20 +5887,6 @@ static inline int _stride_is_extra(SpiceBitmap *bitmap)
>       return 0;
>   }
>
> -static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
> -    LZ_IMAGE_TYPE_INVALID,
> -    LZ_IMAGE_TYPE_PLT1_LE,
> -    LZ_IMAGE_TYPE_PLT1_BE,
> -    LZ_IMAGE_TYPE_PLT4_LE,
> -    LZ_IMAGE_TYPE_PLT4_BE,
> -    LZ_IMAGE_TYPE_PLT8,
> -    LZ_IMAGE_TYPE_RGB16,
> -    LZ_IMAGE_TYPE_RGB24,
> -    LZ_IMAGE_TYPE_RGB32,
> -    LZ_IMAGE_TYPE_RGBA,
> -    LZ_IMAGE_TYPE_A8
> -};
> -
>   typedef struct compress_send_data_t {
>       void*    comp_buf;
>       uint32_t comp_buf_size;
> @@ -5908,7 +5894,6 @@ typedef struct compress_send_data_t {
>       int is_lossy;
>   } compress_send_data_t;
>
> -
>   static inline int red_glz_compress_image(DisplayChannelClient *dcc,
>                                            SpiceImage *dest, SpiceBitmap *src, Drawable *drawable,
>                                            compress_send_data_t* o_comp_data)
>
On 09/03/2012 07:24 PM, Alon Levy wrote:
> Just checks stride vs width times bpp.
>
> This fixes a potential abort on guest generated bad images in
> glz_encoder.
>
> Other files touched to move some consts to red_common, they are
> static so no problem to be defined in both red_worker.c and
> red_parse_qxl.c .

Hi Alon,

I think it's better to share this const array by making it non-static, and
declare it as an extern in red_common.h

Moving the static const array to red_common.h makes each
source file contain it's own copy of the array.

Regards,
     Uri.
> ---
>   server/red_common.h    | 18 +++++++++++++++++-
>   server/red_parse_qxl.c | 17 +++++++++++++++++
>   server/red_worker.c    | 15 ---------------
>   3 files changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/server/red_common.h b/server/red_common.h
> index c563bc0..585b13c 100644
> --- a/server/red_common.h
> +++ b/server/red_common.h
> @@ -25,6 +25,7 @@
>   #include "common/mem.h"
>   #include "common/spice_common.h"
>   #include "common/messages.h"
> +#include "common/lz_common.h"
>
>   #include "spice.h"
>
> @@ -35,13 +36,28 @@ enum {
>       STREAM_VIDEO_FILTER
>   };
>
> +static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
> +    LZ_IMAGE_TYPE_INVALID,
> +    LZ_IMAGE_TYPE_PLT1_LE,
> +    LZ_IMAGE_TYPE_PLT1_BE,
> +    LZ_IMAGE_TYPE_PLT4_LE,
> +    LZ_IMAGE_TYPE_PLT4_BE,
> +    LZ_IMAGE_TYPE_PLT8,
> +    LZ_IMAGE_TYPE_RGB16,
> +    LZ_IMAGE_TYPE_RGB24,
> +    LZ_IMAGE_TYPE_RGB32,
> +    LZ_IMAGE_TYPE_RGBA,
> +    LZ_IMAGE_TYPE_A8
> +};
> +
>   static inline int bitmap_fmt_is_rgb(uint8_t fmt)
>   {
>       static const int BITMAP_FMT_IS_RGB[SPICE_BITMAP_FMT_ENUM_END] =
>                                           {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1};
>
>       if (fmt>= SPICE_BITMAP_FMT_ENUM_END) {
> -        spice_warning("fmt>= SPICE_BITMAP_FMT_ENUM_END");
> +        spice_warning("fmt>= SPICE_BITMAP_FMT_ENUM_END; %d>= %d",
> +                      fmt, SPICE_BITMAP_FMT_ENUM_END);
>           return 0;
>       }
>       return BITMAP_FMT_IS_RGB[fmt];
>
> On 09/03/2012 07:24 PM, Alon Levy wrote:
> > Just checks stride vs width times bpp.
> >
> > This fixes a potential abort on guest generated bad images in
> > glz_encoder.
> >
> > Other files touched to move some consts to red_common, they are
> > static so no problem to be defined in both red_worker.c and
> > red_parse_qxl.c .
> 
> Hi Alon,
> 
> I think it's better to share this const array by making it
> non-static, and
> declare it as an extern in red_common.h
> 
> Moving the static const array to red_common.h makes each
> source file contain it's own copy of the array.

OK.

> 
> Regards,
>      Uri.
> > ---
> >   server/red_common.h    | 18 +++++++++++++++++-
> >   server/red_parse_qxl.c | 17 +++++++++++++++++
> >   server/red_worker.c    | 15 ---------------
> >   3 files changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/server/red_common.h b/server/red_common.h
> > index c563bc0..585b13c 100644
> > --- a/server/red_common.h
> > +++ b/server/red_common.h
> > @@ -25,6 +25,7 @@
> >   #include "common/mem.h"
> >   #include "common/spice_common.h"
> >   #include "common/messages.h"
> > +#include "common/lz_common.h"
> >
> >   #include "spice.h"
> >
> > @@ -35,13 +36,28 @@ enum {
> >       STREAM_VIDEO_FILTER
> >   };
> >
> > +static const LzImageType MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[] = {
> > +    LZ_IMAGE_TYPE_INVALID,
> > +    LZ_IMAGE_TYPE_PLT1_LE,
> > +    LZ_IMAGE_TYPE_PLT1_BE,
> > +    LZ_IMAGE_TYPE_PLT4_LE,
> > +    LZ_IMAGE_TYPE_PLT4_BE,
> > +    LZ_IMAGE_TYPE_PLT8,
> > +    LZ_IMAGE_TYPE_RGB16,
> > +    LZ_IMAGE_TYPE_RGB24,
> > +    LZ_IMAGE_TYPE_RGB32,
> > +    LZ_IMAGE_TYPE_RGBA,
> > +    LZ_IMAGE_TYPE_A8
> > +};
> > +
> >   static inline int bitmap_fmt_is_rgb(uint8_t fmt)
> >   {
> >       static const int BITMAP_FMT_IS_RGB[SPICE_BITMAP_FMT_ENUM_END]
> >       =
> >                                           {0, 0, 0, 0, 0, 0, 1, 1,
> >                                           1, 1, 1};
> >
> >       if (fmt>= SPICE_BITMAP_FMT_ENUM_END) {
> > -        spice_warning("fmt>= SPICE_BITMAP_FMT_ENUM_END");
> > +        spice_warning("fmt>= SPICE_BITMAP_FMT_ENUM_END; %d>= %d",
> > +                      fmt, SPICE_BITMAP_FMT_ENUM_END);
> >           return 0;
> >       }
> >       return BITMAP_FMT_IS_RGB[fmt];
> >
> 
>