diff mbox

[RESEND,2/2] vnc: Fix heap corruption

Message ID 4D6DC06B.6070308@cn.fujitsu.com
State New
Headers show

Commit Message

Wen Congyang March 2, 2011, 3:58 a.m. UTC 
This bug is reported by Stefan Weil:

Comments

Corentin Chary March 2, 2011, 10:57 a.m. UTC | #1 
On Wed, Mar 2, 2011 at 3:58 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> This bug is reported by Stefan Weil:
> ========
> Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
> a severe bug (heap corruption).
>
> bitmap_clear was called with a wrong argument
> which caused out-of-bound writes to width_mask.
>
> This bug was detected with QEMU running on windows.
> It also occurs with wine:
>
> *** stack smashing detected ***:  terminated
> wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...
>
> The bug is not windows specific!
> ========
>
> The third argument of bitmap_clear() is number of bits to be cleared, but we pass
> the end bits to be cleared to bitmap_clear().
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Reported-by: Stefan Weil <weil@mail.berlios.de>

Acked-by: Corentin Chary <corentin.chary@gmail.com>

> ---
>  ui/vnc.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e3761b0..e7d0b5b 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>     unsigned long width_mask[VNC_DIRTY_WORDS];
>     VncState *vs;
>     int has_dirty = 0;
> +    const size_t width = ds_get_width(vd->ds) / 16;
>
>     struct timeval tv = { 0, 0 };
>
> @@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>      * Check and copy modified bits from guest to server surface.
>      * Update server dirty map.
>      */
> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
> +    bitmap_set(width_mask, 0, width);
> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
>     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
>     guest_row  = vd->guest.ds->data;
>     server_row = vd->server->data;
> --
> 1.7.1
>
>
Stefan Weil March 2, 2011, 6:36 p.m. UTC | #2 
Am 02.03.2011 11:57, schrieb Corentin Chary:
> On Wed, Mar 2, 2011 at 3:58 AM, Wen Congyang <wency@cn.fujitsu.com> wrote:
>> This bug is reported by Stefan Weil:
>> ========
>> Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
>> a severe bug (heap corruption).
>>
>> bitmap_clear was called with a wrong argument
>> which caused out-of-bound writes to width_mask.
>>
>> This bug was detected with QEMU running on windows.
>> It also occurs with wine:
>>
>> *** stack smashing detected ***:  terminated
>> wine: Unhandled illegal instruction at address 0x6115c7 (thread 
>> 0009), starting debugger...
>>
>> The bug is not windows specific!
>> ========
>>
>> The third argument of bitmap_clear() is number of bits to be cleared, 
>> but we pass
>> the end bits to be cleared to bitmap_clear().
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Reported-by: Stefan Weil <weil@mail.berlios.de>
>
> Acked-by: Corentin Chary <corentin.chary@gmail.com>
>

No. I dont't think that the third parameter of bitmap_clear is
ok like that. See my patch for the correct value.

My own patch is also incomplete, so I'll send an update.

Stefan


>> ---
>>   ui/vnc.c |    6 +++---
>>   1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index e3761b0..e7d0b5b 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>>      unsigned long width_mask[VNC_DIRTY_WORDS];
>>      VncState *vs;
>>      int has_dirty = 0;
>> +    const size_t width = ds_get_width(vd->ds) / 16;
>>
>>      struct timeval tv = { 0, 0 };
>>
>> @@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
>>       * Check and copy modified bits from guest to server surface.
>>       * Update server dirty map.
>>       */
>> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
>> +    bitmap_set(width_mask, 0, width);
>> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
>>      cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
>>      guest_row  = vd->guest.ds->data;
>>      server_row = vd->server->data;
>> --
>> 1.7.1
>>
>>
>
>
>
Peter Maydell March 2, 2011, 6:47 p.m. UTC | #3 
On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
> No. I dont't think that the third parameter of bitmap_clear is
> ok like that. See my patch for the correct value.

Wen's patch:

+    const size_t width = ds_get_width(vd->ds) / 16;
[...]
-    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
-    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
-                 VNC_DIRTY_WORDS * BITS_PER_LONG);
+    bitmap_set(width_mask, 0, width);
+    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);

Your patch:

     bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
-                 VNC_DIRTY_WORDS * BITS_PER_LONG);
+                 (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);

Since ui/vnc.h has:

#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))

the third parameter to bitmap_clear is the same value in
both cases, isn't it? Or is this a rounding bug?

-- PMM
Stefan Weil March 2, 2011, 10:01 p.m. UTC | #4 
Am 02.03.2011 19:47, schrieb Peter Maydell:
> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>> No. I dont't think that the third parameter of bitmap_clear is
>> ok like that. See my patch for the correct value.
>
> Wen's patch:
>
> + const size_t width = ds_get_width(vd->ds) / 16;
> [...]
> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
> +    bitmap_set(width_mask, 0, width);
> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - 
> width);
>
> Your patch:
>
> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
> - VNC_DIRTY_WORDS * BITS_PER_LONG);
> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>
> Since ui/vnc.h has:
>
> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>
> the third parameter to bitmap_clear is the same value in
> both cases, isn't it? Or is this a rounding bug?
>
> -- PMM

Because of rounding effects, both values can be different.

The part missing in my patch is correct handling of another
rounding effect:

VNC_DIRTY_WORDS is exact for 32 bit long values (and the
"old" code which used uint32_t until some weeks ago), where
VNC_DIRTY_WORDS = 2560/16/32 = 5.

For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!

Stefan W.
Stefan Weil March 2, 2011, 10:27 p.m. UTC | #5 
Am 02.03.2011 23:01, schrieb Stefan Weil:
> Am 02.03.2011 19:47, schrieb Peter Maydell:
>> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>>> No. I dont't think that the third parameter of bitmap_clear is
>>> ok like that. See my patch for the correct value.
>>
>> Wen's patch:
>>
>> + const size_t width = ds_get_width(vd->ds) / 16;
>> [...]
>> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
>> +    bitmap_set(width_mask, 0, width);
>> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG 
>> - width);
>>
>> Your patch:
>>
>> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>>
>> Since ui/vnc.h has:
>>
>> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>>
>> the third parameter to bitmap_clear is the same value in
>> both cases, isn't it? Or is this a rounding bug?
>>
>> -- PMM
>
> Because of rounding effects, both values can be different.
>
> The part missing in my patch is correct handling of another
> rounding effect:
>
> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
> "old" code which used uint32_t until some weeks ago), where
> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>
> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
>
> Stefan W.


Is bitmap_clear() really needed here? Meanwhile I think it is not,
so this might be a new patch variant...
Peter Maydell March 2, 2011, 10:40 p.m. UTC | #6 
On 2 March 2011 22:01, Stefan Weil <weil@mail.berlios.de> wrote:
> The part missing in my patch is correct handling of another
> rounding effect:
>
> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
> "old" code which used uint32_t until some weeks ago), where
> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>
> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!

Yes, I noticed that after I posted. Given that we have arrays
like
     unsigned long dirty[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
rounding down rather than up looks a bit suspicious...

(Can we just make VNC_MAX_WIDTH a multiple of 64, or is it
baked into the VNC protocol?)

-- PMM
Wen Congyang March 3, 2011, 1:37 a.m. UTC | #7 
At 03/03/2011 06:27 AM, Stefan Weil Write:
> Am 02.03.2011 23:01, schrieb Stefan Weil:
>> Am 02.03.2011 19:47, schrieb Peter Maydell:
>>> On 2 March 2011 18:36, Stefan Weil <weil@mail.berlios.de> wrote:
>>>> No. I dont't think that the third parameter of bitmap_clear is
>>>> ok like that. See my patch for the correct value.
>>>
>>> Wen's patch:
>>>
>>> + const size_t width = ds_get_width(vd->ds) / 16;
>>> [...]
>>> -    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
>>> -    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>>> -                 VNC_DIRTY_WORDS * BITS_PER_LONG);
>>> +    bitmap_set(width_mask, 0, width);
>>> +    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG
>>> - width);
>>>
>>> Your patch:
>>>
>>> bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
>>> - VNC_DIRTY_WORDS * BITS_PER_LONG);
>>> + (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
>>>
>>> Since ui/vnc.h has:
>>>
>>> #define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
>>>
>>> the third parameter to bitmap_clear is the same value in
>>> both cases, isn't it? Or is this a rounding bug?
>>>
>>> -- PMM
>>
>> Because of rounding effects, both values can be different.
>>
>> The part missing in my patch is correct handling of another
>> rounding effect:
>>
>> VNC_DIRTY_WORDS is exact for 32 bit long values (and the
>> "old" code which used uint32_t until some weeks ago), where
>> VNC_DIRTY_WORDS = 2560/16/32 = 5.
>>
>> For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
>>
>> Stefan W.
> 
> 
> Is bitmap_clear() really needed here? Meanwhile I think it is not,
> so this might be a new patch variant...

I do not know why we call bitmap_clear() hear. I only know it is
the same as the old code:

-static inline void vnc_set_bits(uint32_t *d, int n, int nb_words)
-{
-    int j;
-
-    j = 0;
-    while (n >= 32) {
-        d[j++] = -1;
-        n -= 32;
-    }
-    if (n > 0)
-        d[j++] = (1 << n) - 1;
-    while (j < nb_words)               <=== bitmap_clear()
-        d[j++] = 0;
-}

-    vnc_set_bits(width_mask, (ds_get_width(vd->ds) / 16), VNC_DIRTY_WORDS);
+    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
+    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
+                 VNC_DIRTY_WORDS * BITS_PER_LONG);


> 
> 
>
diff mbox

Patch

========
Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced
a severe bug (heap corruption).

bitmap_clear was called with a wrong argument
which caused out-of-bound writes to width_mask.

This bug was detected with QEMU running on windows.
It also occurs with wine:

*** stack smashing detected ***:  terminated
wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...

The bug is not windows specific!
========

The third argument of bitmap_clear() is number of bits to be cleared, but we pass
the end bits to be cleared to bitmap_clear().

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Reported-by: Stefan Weil <weil@mail.berlios.de>

---
 ui/vnc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index e3761b0..e7d0b5b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2390,6 +2390,7 @@  static int vnc_refresh_server_surface(VncDisplay *vd)
     unsigned long width_mask[VNC_DIRTY_WORDS];
     VncState *vs;
     int has_dirty = 0;
+    const size_t width = ds_get_width(vd->ds) / 16;
 
     struct timeval tv = { 0, 0 };
 
@@ -2403,9 +2404,8 @@  static int vnc_refresh_server_surface(VncDisplay *vd)
      * Check and copy modified bits from guest to server surface.
      * Update server dirty map.
      */
-    bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
-    bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
-                 VNC_DIRTY_WORDS * BITS_PER_LONG);
+    bitmap_set(width_mask, 0, width);
+    bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
     cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds);
     guest_row  = vd->guest.ds->data;
     server_row = vd->server->data;