On Thu, 12 Dec 2002 22:26:23 +0000 Keith Whitwell <[EMAIL PROTECTED]> wrote:
> Felix Kühling wrote: > > Hi, > > > > while I was messing around with my query programme I found this: if I > > specify an invalid screen as argument to XF86DRIGetClientDriverName the > > Xserver segfaults. I had a quick look at xc/xc/Xserver/GL/dri/xf86dri.c. > > stuff->screen is used as array index without checking. I'm not sure > > though, where would be the right place to fix it. > > > > Other functions in xf86dri.c must be affacted, too. They use > > stuff->screen in the same way. > > Those functions should validate any information they receive over the wire, as > soon as is feasible. The problem is that the request records are all different. So we can't check stuff->screen in ProcXF86DRIDispatch. We have to do it in each request separately. The attached patch does just that. While I grepped around to see how other extensions check this I found one more potential problem in programs/Xserver/GL/glx/glxcmds.c and two suspicious casts of screen from unsigned to int (screen is always unsigned in the request records). They are fixed in the attached patch as well. Regards, Felix __\|/__ ___ ___ ___ __Tschüß_______\_6 6_/___/__ \___/__ \___/___\___You can do anything,___ _____Felix_______\Ä/\ \_____\ \_____\ \______U___just not everything____ [EMAIL PROTECTED] >o<__/ \___/ \___/ at the same time!
Index: dri/xf86dri.c =================================================================== RCS file: /cvsroot/dri/xc/xc/programs/Xserver/GL/dri/xf86dri.c,v retrieving revision 1.10 diff -u -r1.10 xf86dri.c --- dri/xf86dri.c 29 Oct 2002 20:28:57 -0000 1.10 +++ dri/xf86dri.c 13 Dec 2002 15:51:57 -0000 @@ -155,6 +155,11 @@ REQUEST(xXF86DRIQueryDirectRenderingCapableReq); REQUEST_SIZE_MATCH(xXF86DRIQueryDirectRenderingCapableReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; @@ -184,6 +189,10 @@ REQUEST(xXF86DRIOpenConnectionReq); REQUEST_SIZE_MATCH(xXF86DRIOpenConnectionReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } if (!DRIOpenConnection( screenInfo.screens[stuff->screen], &hSAREA, @@ -221,6 +230,10 @@ REQUEST(xXF86DRIAuthConnectionReq); REQUEST_SIZE_MATCH(xXF86DRIAuthConnectionReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } rep.type = X_Reply; rep.length = 0; @@ -242,6 +255,10 @@ { REQUEST(xXF86DRICloseConnectionReq); REQUEST_SIZE_MATCH(xXF86DRICloseConnectionReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } DRICloseConnection( screenInfo.screens[stuff->screen]); @@ -258,6 +275,10 @@ REQUEST(xXF86DRIGetClientDriverNameReq); REQUEST_SIZE_MATCH(xXF86DRIGetClientDriverNameReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } DRIGetClientDriverName( screenInfo.screens[stuff->screen], (int *)&rep.ddxDriverMajorVersion, @@ -295,6 +316,11 @@ REQUEST(xXF86DRICreateContextReq); REQUEST_SIZE_MATCH(xXF86DRICreateContextReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; @@ -329,6 +355,10 @@ { REQUEST(xXF86DRIDestroyContextReq); REQUEST_SIZE_MATCH(xXF86DRIDestroyContextReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } if (!DRIDestroyContext( screenInfo.screens[stuff->screen], stuff->context)) { @@ -348,6 +378,11 @@ REQUEST(xXF86DRICreateDrawableReq); REQUEST_SIZE_MATCH(xXF86DRICreateDrawableReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; @@ -378,6 +413,10 @@ REQUEST(xXF86DRIDestroyDrawableReq); DrawablePtr pDrawable; REQUEST_SIZE_MATCH(xXF86DRIDestroyDrawableReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } if (!(pDrawable = (DrawablePtr)SecurityLookupDrawable( (Drawable)stuff->drawable, @@ -409,6 +448,11 @@ REQUEST(xXF86DRIGetDrawableInfoReq); REQUEST_SIZE_MATCH(xXF86DRIGetDrawableInfoReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; @@ -483,6 +527,11 @@ REQUEST(xXF86DRIGetDeviceInfoReq); REQUEST_SIZE_MATCH(xXF86DRIGetDeviceInfoReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; @@ -528,6 +577,11 @@ DrawablePtr pDrawable; REQUEST_SIZE_MATCH(xXF86DRIOpenFullScreenReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; @@ -554,6 +608,11 @@ DrawablePtr pDrawable; REQUEST_SIZE_MATCH(xXF86DRICloseFullScreenReq); + if (stuff->screen >= screenInfo.numScreens) { + client->errorValue = stuff->screen; + return BadValue; + } + rep.type = X_Reply; rep.length = 0; rep.sequenceNumber = client->sequence; Index: glx/glxcmds.c =================================================================== RCS file: /cvsroot/dri/xc/xc/programs/Xserver/GL/glx/glxcmds.c,v retrieving revision 1.8 diff -u -r1.8 glxcmds.c --- glx/glxcmds.c 25 Nov 2002 19:58:38 -0000 1.8 +++ glx/glxcmds.c 13 Dec 2002 15:52:01 -0000 @@ -761,7 +761,7 @@ int i, p; screen = req->screen; - if (screen > screenInfo.numScreens) { + if (screen >= screenInfo.numScreens) { /* The client library must send a valid screen number. */ client->errorValue = screen; return BadValue; @@ -1466,7 +1466,7 @@ ClientPtr client = cl->client; xGLXQueryExtensionsStringReq *req = (xGLXQueryExtensionsStringReq *) pc; xGLXQueryExtensionsStringReply reply; - GLint screen; + GLuint screen; size_t n, length; const char *ptr; char *buf; @@ -1475,7 +1475,7 @@ /* ** Check if screen exists. */ - if ((screen < 0) || (screen >= screenInfo.numScreens)) { + if (screen >= screenInfo.numScreens) { client->errorValue = screen; return BadValue; } @@ -1511,7 +1511,7 @@ xGLXQueryServerStringReq *req = (xGLXQueryServerStringReq *) pc; xGLXQueryServerStringReply reply; int name; - GLint screen; + GLuint screen; size_t n, length; const char *ptr; char *buf; @@ -1521,7 +1521,7 @@ /* ** Check if screen exists. */ - if ((screen < 0) || (screen >= screenInfo.numScreens)) { + if (screen >= screenInfo.numScreens) { client->errorValue = screen; return BadValue; }