I have a few comments, I guess. - The indentation in the GetReqExtra definition didn't work out. Looks like it should be tabs and tabs only, there.
- This patch turns a buffer overrun into a null-pointer dereference if there's any caller that hasn't been updated. (While you've hit all the cases I can find in X.Org sources, keep in mind that there are closed-source Xlib extensions out there.) I think that's an improvement, but it's worth calling out in the commit message. - I guess this new behavior should be documented in specs/libX11/AppC.xml. - Your mention of 16K in the commit message confused me because I wasn't sure where you were getting that limit from. Note that Xlib's buffer size is actually runtime-configurable, down to a minimum of 2K. Could you explain it in terms of Xlib's output buffer size, please? - Changing the implementation of GetReqExtra and the rest of Xlibint.h is only acceptable if code compiled using the old implementation is ABI compatible with code compiled using the new implementation. I believe you're good on that count, but I wanted to point it out. On the whole, I believe this is an improvement. With the above corrections, I'd be happy to commit this. Jamey On Mon, Jul 18, 2011 at 08:57:44AM -0700, Kees Cook wrote: > Hi, any comments on this? Seems like kind of a nasty surprise bug... > > Thanks, > > -Kees > > On Sat, Jul 09, 2011 at 12:42:57PM -0700, Kees Cook wrote: > > Two users of GetReqExtra pass arbitrarily sized allocations from the > > caller (ModMap and Host). Adjust the GetReqExtra macro to double-check > > the requested length and invalidate "req" when this happens. Users of > > potentially >16K lengths in GetReqExtra now check "req" and fail if > > something has gone wrong. > > > > https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628 > > > > Signed-off-by: Kees Cook <[email protected]> > > --- > > include/X11/Xlibint.h | 28 ++++++++++++++++++---------- > > src/Host.c | 8 ++++++++ > > src/ModMap.c | 4 ++++ > > 3 files changed, 30 insertions(+), 10 deletions(-) > > > > diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h > > index 2ce356d..81f8cfd 100644 > > --- a/include/X11/Xlibint.h > > +++ b/include/X11/Xlibint.h > > @@ -461,21 +461,29 @@ extern LockInfoPtr _Xglobal_lock; > > WORD64ALIGN\ > > if ((dpy->bufptr + SIZEOF(x##name##Req) + n) > dpy->bufmax)\ > > _XFlush(dpy);\ > > - req = (x##name##Req *)(dpy->last_req = dpy->bufptr);\ > > - req->reqType = X_##name;\ > > - req->length = (SIZEOF(x##name##Req) + n)>>2;\ > > - dpy->bufptr += SIZEOF(x##name##Req) + n;\ > > - dpy->request++ > > + if ((dpy->bufptr + SIZEOF(x##name##Req) + n) <= dpy->bufmax) {\ > > + req = (x##name##Req *)(dpy->last_req = dpy->bufptr);\ > > + req->reqType = X_##name;\ > > + req->length = (SIZEOF(x##name##Req) + n)>>2;\ > > + dpy->bufptr += SIZEOF(x##name##Req) + n;\ > > + dpy->request++;\ > > + } else {\ > > + req = NULL;\ > > + } > > #else > > #define GetReqExtra(name, n, req) \ > > WORD64ALIGN\ > > if ((dpy->bufptr + SIZEOF(x/**/name/**/Req) + n) > dpy->bufmax)\ > > _XFlush(dpy);\ > > - req = (x/**/name/**/Req *)(dpy->last_req = dpy->bufptr);\ > > - req->reqType = X_/**/name;\ > > - req->length = (SIZEOF(x/**/name/**/Req) + n)>>2;\ > > - dpy->bufptr += SIZEOF(x/**/name/**/Req) + n;\ > > - dpy->request++ > > + if ((dpy->bufptr + SIZEOF(x/**/name/**/Req) + n) <= dpy->bufmax) {\ > > + req = (x/**/name/**/Req *)(dpy->last_req = dpy->bufptr);\ > > + req->reqType = X_/**/name;\ > > + req->length = (SIZEOF(x/**/name/**/Req) + n)>>2;\ > > + dpy->bufptr += SIZEOF(x/**/name/**/Req) + n;\ > > + dpy->request++;\ > > + } else {\ > > + req = NULL;\ > > + } > > #endif > > > > > > diff --git a/src/Host.c b/src/Host.c > > index da9923a..3f87731 100644 > > --- a/src/Host.c > > +++ b/src/Host.c > > @@ -83,6 +83,10 @@ XAddHost ( > > > > LockDisplay(dpy); > > GetReqExtra (ChangeHosts, length, req); > > + if (!req) { > > + UnlockDisplay(dpy); > > + return 0; > > + } > > req->mode = HostInsert; > > req->hostFamily = host->family; > > req->hostLength = addrlen; > > @@ -118,6 +122,10 @@ XRemoveHost ( > > > > LockDisplay(dpy); > > GetReqExtra (ChangeHosts, length, req); > > + if (!req) { > > + UnlockDisplay(dpy); > > + return 0; > > + } > > req->mode = HostDelete; > > req->hostFamily = host->family; > > req->hostLength = addrlen; > > diff --git a/src/ModMap.c b/src/ModMap.c > > index c99bfdd..5deb894 100644 > > --- a/src/ModMap.c > > +++ b/src/ModMap.c > > @@ -75,6 +75,10 @@ XSetModifierMapping( > > > > LockDisplay(dpy); > > GetReqExtra(SetModifierMapping, mapSize, req); > > + if (!req) { > > + UnlockDisplay(dpy); > > + return 2; > > + } > > > > req->numKeyPerModifier = modifier_map->max_keypermod; > > > > -- > > 1.7.4.1
signature.asc
Description: Digital signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
