This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


Re: [Xen-devel] Two small patches related to xenfb

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: Re: [Xen-devel] Two small patches related to xenfb
From: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Date: Fri, 26 Sep 2008 15:45:11 +0100
Delivery-date: Fri, 26 Sep 2008 07:47:20 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20080926140548.GC31985@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Newsgroups: chiark.mail.xen.devel
References: <20080926140548.GC31985@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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

> +    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

Also, you should include a Signed-Off-By line like this
    Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
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.


>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

        (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.


Xen-devel mailing list

<Prev in Thread] Current Thread [Next in Thread>