integer overflow check

[Alan Coopersmith]

On 12/15/14 08:56 AM, jes at posteo.de wrote:
> Hello,
>
> the recent xserver security patches included this patch:
> http://cgit.freedesktop.org/xorg/xserver/commit/?id=6692670fde081bbfe9313f17d84037ae9116702a
>
>
> I wonder why there is no integer overflow check in line 300 of
> http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/dri2/dri2ext.c

Because that wasn't one of the issues Ilja reported to us, and no one else
noticed it during our reviews.

> I'm asking myself why the overflow check is made after the REQUEST_FIXED_SIZE call.
> I read the macro and things should not explode when an overflow will happen.
> The undefined behavior could be avoided by moving the check before the call.

Again, I've got no better answer than no one noticed during our reviews since
we were focusing on making sure the malloc calls didn't overflow, not on
fighting the optimizer deciding to work against us.

> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index 221ec53..63191dc 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -269,10 +269,11 @@ ProcDRI2GetBuffers(ClientPtr client)
>       int status, width, height, count;
>       unsigned int *attachments;
>
> -    REQUEST_FIXED_SIZE(xDRI2GetBuffersReq, stuff->count * 4);
>       if (stuff->count > (INT_MAX / 4))
>           return BadLength;
>
> +    REQUEST_FIXED_SIZE(xDRI2GetBuffersReq, stuff->count * 4);
> +
>       if (!validDrawable(client, stuff->drawable, DixReadAccess | DixWriteAccess,
>                          &pDrawable, &status))
>           return status;
> @@ -297,6 +298,9 @@ ProcDRI2GetBuffersWithFormat(ClientPtr client)
>       int status, width, height, count;
>       unsigned int *attachments;
>
> +    if (stuff->count > (INT_MAX / (2 * 4)))
> +        return BadLength;
> +
>       REQUEST_FIXED_SIZE(xDRI2GetBuffersReq, stuff->count * (2 * 4));
>       if (!validDrawable(client, stuff->drawable, DixReadAccess | DixWriteAccess,
>                          &pDrawable, &status))

Reviewed-by: Alan Coopersmith <alan.coopersmith at oracle.com>

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