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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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.

-- 

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