[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-xorg-devel
Subject:    Re: [PATCH] libXrender: avoid possible overflow with multiple members
From:       Julien Cristau <jcristau () debian ! org>
Date:       2013-05-27 15:02:09
Message-ID: 20130527150209.GK12846 () radis ! cristau ! org
[Download RAW message or body]

Hi Dave,

On Mon, May 27, 2013 at 08:56:34 +1000, Dave Airlie wrote:

> From: Dave Airlie <airlied@redhat.com>
> 
> If all of these limits are pushed to their mask, then / 4 won't stop

I assume s/mask/max/

> the malloc from being overflowed.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  src/Xrender.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/Xrender.c b/src/Xrender.c
> index 3102eb2..1c859ea 100644
> --- a/src/Xrender.c
> +++ b/src/Xrender.c
> @@ -459,11 +459,11 @@ XRenderQueryFormats (Display *dpy)
>      if (async_state.major_version == 0 && async_state.minor_version < 6)
>  	rep.numSubpixel = 0;
>  
> -    if ((rep.numFormats < ((INT_MAX / 4) / sizeof (XRenderPictFormat))) &&
> -	(rep.numScreens < ((INT_MAX / 4) / sizeof (XRenderScreen))) &&
> -	(rep.numDepths  < ((INT_MAX / 4) / sizeof (XRenderDepth))) &&
> -	(rep.numVisuals < ((INT_MAX / 4) / sizeof (XRenderVisual))) &&
> -	(rep.numSubpixel < ((INT_MAX / 4) / 4)) &&
> +    if ((rep.numFormats < ((INT_MAX / 8) / sizeof (XRenderPictFormat))) &&
> +	(rep.numScreens < ((INT_MAX / 8) / sizeof (XRenderScreen))) &&
> +	(rep.numDepths  < ((INT_MAX / 8) / sizeof (XRenderDepth))) &&
> +	(rep.numVisuals < ((INT_MAX / 8) / sizeof (XRenderVisual))) &&
> +	(rep.numSubpixel < ((INT_MAX / 8) / 4)) &&
>  	(rep.length < (INT_MAX >> 2)) ) {
>  	xri = Xmalloc (sizeof (XRenderInfo) +
>  		       (rep.numFormats * sizeof (XRenderPictFormat)) +

I don't see the overflow here.

Worst case is each of these is (INT_MAX / 4) - 1.

The first malloc would get 4 * ((INT_MAX / 4) - 1) + sizeof(XRenderInfo)
which is still < UINT_MAX <= SIZE_MAX.

sizeof(XRenderPictFormat) > sizeof(xPictFormInfo)
sizeof(XRenderScreen) > sizeof(xPictScreen)
sizeof(XRenderDepth) > sizeof(xPictDepth)
sizeof(XRenderVisual) > sizeof (xPictVisual)

So the second malloc gets at most 5 * ((INT_MAX / 4) - 1), which
shouldn't overflow UINT_MAX either?

There's an argument for making this all more obviously correct, in any
case...

Cheers,
Julien
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[prev in list] [next in list] [prev in thread] [next in thread]