[PATCH 4/5] Xephyr: integer overflow in XF86DRIOpenConnection()

[Alan Coopersmith]

On 06/ 1/13 03:26 AM, Julien Cristau wrote:
> On Thu, May 23, 2013 at 09:27:29 -0700, Alan Coopersmith wrote:
>
>> busIdStringLength is a CARD32 and needs to be bounds checked before adding
>> one to it to come up with the total size to allocate, to avoid integer
>> overflow leading to underallocation and writing data from the network past
>> the end of the allocated buffer.
>>
>> Reported-by: Ilja Van Sprundel <ivansprundel at ioactive.com>
>> Signed-off-by: Alan Coopersmith <alan.coopersmith at oracle.com>
>> ---
>>   hw/kdrive/ephyr/XF86dri.c |    6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/kdrive/ephyr/XF86dri.c b/hw/kdrive/ephyr/XF86dri.c
>> index 9d742f3..b1f070e 100644
>> --- a/hw/kdrive/ephyr/XF86dri.c
>> +++ b/hw/kdrive/ephyr/XF86dri.c
>> @@ -225,7 +225,11 @@ XF86DRIOpenConnection(Display * dpy, int screen,
>>       }
>>
>>       if (rep.length) {
>> -        if (!(*busIdString = (char *) calloc(rep.busIdStringLength + 1, 1))) {
>> +        if (rep.busIdStringLength < INT_MAX)
>> +            *busIdString = calloc(rep.busIdStringLength + 1, 1);
>> +        else
>> +            *busIdString = NULL;
>> +        if (*busIdString == NULL) {
>>               _XEatData(dpy, ((rep.busIdStringLength + 3) & ~3));
>>               UnlockDisplay(dpy);
>>               SyncHandle();
>
> We might still overflow in the _XEatData call, but I guess that's not so
> much an issue.

Yes, that won't cause to us to overflow a buffer, just not skip far enough
ahead in the request queue to find the next packet.

I do plan on submitting a patch to require Xlib 1.6 & use XEatDataWords
instead soon to cover that case.


-- 
	-Alan Coopersmith-              alan.coopersmith at oracle.com
	 Oracle Solaris Engineering - http://blogs.oracle.com/alanc