Two small patches related to xenfb

Two small patches related to xenfb

[Rafal Wojtczuk]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

Hello,
Two minor issues:
row_stride_div0.patch: a malicious frontend can send row_stride==0 and force
qemu-dm to perform division by 0
vnc_resize_doublecheck.patch: there is an unchecked multiplication when
calculating framebuffer size. Cs 17630 sanitizes framebuffer dimensions
passed by the frontend, so most probably no integer overflow can happen, but
there should be a check for overflow close to the actual computation (to
make code review easier and to cope with other codepaths in the future). 

Diffs against xen-3.2-testing.hg.

Regards,
Rafal Wojtczuk
Principal Researcher
Invisible Things Lab


[-- Attachment #2: row_stride_div0.patch --]
[-- Type: text/plain, Size: 400 bytes --]

--- xen-3.2-testing.hg/tools/ioemu/hw/xenfb.c.orig	2008-09-26 11:16:55.000000000 +0200
+++ xen-3.2-testing.hg/tools/ioemu/hw/xenfb.c	2008-09-26 11:18:01.000000000 +0200
@@ -496,7 +496,7 @@
 			depth);
 		return -1;
 	}
-	if (row_stride < 0 || row_stride > fb_len) {
+	if (row_stride <= 0 || row_stride > fb_len) {
 		fprintf(stderr,
 			"FB: invalid frontend stride %d\n", row_stride);
 		return -1;

[-- Attachment #3: vnc_resize_doublecheck.patch --]
[-- Type: text/plain, Size: 920 bytes --]

--- xen-3.2-testing.hg/tools/ioemu/vnc.c.orig	2008-09-26 11:14:21.000000000 +0200
+++ xen-3.2-testing.hg/tools/ioemu/vnc.c	2008-09-26 11:37:25.000000000 +0200
@@ -329,13 +329,23 @@
 
     vnc_write_s32(vs, encoding);
 }
-
+static int mult_overflows(int x, int y)
+{
+    if (x<=0 || y<=0 || x*y<=0 || x>((unsigned int)(-1))/y)
+        return 1;
+    else return 0;
+}
 static void vnc_dpy_resize(DisplayState *ds, int w, int h)
 {
     int size_changed;
     VncState *vs = ds->opaque;
     int o;
 
+    if (mult_overflows(w, h) || mult_overflows(w*h, vs->depth) ||
+        mult_overflows(h, sizeof(vs->dirty_row[0])) {
+        fprintf(stderr, "vnc: suspicious vnc_dpy_resize arguments, exiting\n");
+        exit(1);
+    }
     ds->data = realloc(ds->data, w * h * vs->depth);
     vs->old_data = realloc(vs->old_data, w * h * vs->depth);
     vs->dirty_row = realloc(vs->dirty_row, h * sizeof(vs->dirty_row[0]));

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Two small patches related to xenfb

[Ian Jackson]

Rafal Wojtczuk writes ("[Xen-devel] Two small patches related to xenfb"):
> Two minor issues:
> row_stride_div0.patch: a malicious frontend can send row_stride==0 and force
> qemu-dm to perform division by 0
> vnc_resize_doublecheck.patch: there is an unchecked multiplication when
> calculating framebuffer size. Cs 17630 sanitizes framebuffer dimensions
> passed by the frontend, so most probably no integer overflow can happen, but
> there should be a check for overflow close to the actual computation (to
> make code review easier and to cope with other codepaths in the future). 

Thanks.  Your patch wasn't quite right in a couple of ways but I have
fixed it up and applied it to qemu-xen-unstable.  I'll cherry pick it
into 3.3 after I get a successful test run.

> Diffs against xen-3.2-testing.hg.

Thanks but xen-3.2 is pretty much out of our support life cycle now.
We would suggest you use 3.3 instead.

> +static int mult_overflows(int x, int y)
> +{
> +    if (x<=0 || y<=0 || x*y<=0 || x>((unsigned int)(-1))/y)

This isn't correct.
  * The use of (unsigned int)(-1) is strange; you should use INT_MAX
    (and not ~(unsigned int)0 either, as we wanted the figure for
    _signed_ overflow).
  * Multiplying x*y is undefined behaviour if it's an overflow;
    that can in theory lead the compiler to `prove' that you know
    that there is no overflow and conclude that the other tests cannot
    fail.  You should not do this test at all.  The division is
    sufficient.

> +    if (mult_overflows(w, h) || mult_overflows(w*h, vs->depth) ||
> +        mult_overflows(h, sizeof(vs->dirty_row[0])) {

Evidently you didn't compile this because it has a missing
parenthesis.

Also, you should include a Signed-Off-By line like this
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
except with your name and email address.  This indicates that you are
certifying the code as suitable (copyright-wise and so on) for
inclusion in Xen.  You can find the precise meaning in the Linux
upstream kernel tree (Documentation/SubmittingPatches).

In this case, and since your patch was so small, I took your message
to grant the relevant permissions.

Ian.

>From Documentation/SubmittingPatches:

        Developer's Certificate of Origin 1.1

        By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

-- 

Re: Two small patches related to xenfb

[Gerd Hoffmann]

Rafal Wojtczuk wrote:
> Hello,
> Two minor issues:
> row_stride_div0.patch: a malicious frontend can send row_stride==0 and force
> qemu-dm to perform division by 0

Ok.

> vnc_resize_doublecheck.patch: there is an unchecked multiplication when
> calculating framebuffer size. Cs 17630 sanitizes framebuffer dimensions
> passed by the frontend, so most probably no integer overflow can happen, but
> there should be a check for overflow close to the actual computation (to
> make code review easier and to cope with other codepaths in the future). 

If bogous values can make it through the sanity checks in
xenfb_configure_fb() then those sanity checks must be fixed.

Adding another check somewhere else certainly doesn't make review
easier.  In contrast it makes error handling more complicated because
there are multiple places where you have to deal with errors instead of
just one functions which does all sanity checks.

cheers,
  Gerd