[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Two small patches related to xenfb
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |