Re: [PATCH] x86emu: improve single-step debugging
This is a seriously old thread, but it's still been hanging around in my inbox, unresolved... What's needed to apply my patch to both Xorg and libx86? Neither code bases have changed in several years, and Xorg is still using its internal copy instead of the Matthew's external libx86. Thanks! -Kees On Fri, Nov 04, 2011 at 12:35:34AM +0100, Guillem Jover wrote: Hi! On Wed, 2011-11-02 at 14:32:01 -0700, Kees Cook wrote: On Fri, Oct 07, 2011 at 02:49:03AM +0200, Guillem Jover wrote: On Thu, 2011-10-06 at 15:49:33 -0700, Alan Coopersmith wrote: On 10/ 6/11 03:36 PM, Kees Cook wrote: This allows for other consumers to do single-step decoding/emulation when using x86emu. Additionally adds a stand-alone Makefile for building out of tree, which is very handy for doing emulation debugging. What ever happened to the plan to move it out to a separate library that Xorg just depends on? We have another user here who is currently just copying the code from Xorg and would prefer to share a library with us. I thought libx86 [0] was supposed to be just that. [0] http://www.codon.org.uk/~mjg59/libx86/ So, until Xorg is linked against libx86, can my patch be applied to the Xorg emulator? From what I can see it is where fixes keep landing. Hmm true. So it probably makes sense to apply it in both, so that they don't get more out-of-sync. Matthew the patch being talked about here can be found at: http://lists.x.org/archives/xorg-devel/2011-October/026108.html What's your opinion? Also, maybe it also would make sense to request that people get patches for x86emu first in libx86, before merging them in the X server? regards, guillem -- Kees Cook@outflux.net ___ 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
Re: [PATCH 1/2] libX11: check size of GetReqExtra after XFlush
Excellent! :) Thanks!
-Kees
On Mon, Jul 22, 2013 at 11:55:23PM -0700, Alan Coopersmith wrote:
Sorry, was distracted long enough that all memory of these patches had been
flushed from my cache, so I had to dig into them again to reunderstand them.
I think they look good enough now, so I've added
Reviewed-by: Alan Coopersmith alan.coopersm...@oracle.com
and pushed both to git master:
To ssh://git.freedesktop.org/git/xorg/lib/libX11
24d3ee0..feb131b master - master
-alan-
On 07/18/13 03:52 PM, Kees Cook wrote:
Re-re-ping. :) Can anyone commit these two patches please?
Thanks!
-Kees
On Sun, Jun 09, 2013 at 11:13:42AM -0700, Kees Cook wrote:
Two users of GetReqExtra pass arbitrarily sized allocations from the
caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
macro) to double-check the requested length and invalidate req when
this happens. Users of GetReqExtra passing lengths greater than the Xlib
buffer size (normally 16K) must check req and fail gracefully instead
of crashing.
Any callers of GetReqExtra that do not check req for NULL
will experience this change, in the pathological case, as a NULL
dereference instead of a buffer overflow. This is an improvement, but
the documentation for GetReqExtra has been updated to reflect the need
to check the value of req after the call.
Bug that manifested the problem:
https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
Signed-off-by: Kees Cook k...@outflux.net
---
specs/libX11/AppC.xml | 4 +++-
src/XlibInt.c | 8
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/specs/libX11/AppC.xml b/specs/libX11/AppC.xml
index df25027..0b37048 100644
--- a/specs/libX11/AppC.xml
+++ b/specs/libX11/AppC.xml
@@ -2468,7 +2468,9 @@ which is the same as
functionGetReq/function
except that it takes an additional argument (the number of
extra bytes to allocate in the output buffer after the request structure).
-This number should always be a multiple of four.
+This number should always be a multiple of four. Note that it is possible
+for req to be set to NULL as a defensive measure if the requested length
+exceeds the Xlib's buffer size (normally 16K).
/para
/sect2
sect2 id=Variable_Length_Arguments
diff --git a/src/XlibInt.c b/src/XlibInt.c
index b06e57b..c3273a8 100644
--- a/src/XlibInt.c
+++ b/src/XlibInt.c
@@ -1733,6 +1733,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t
len)
if (dpy-bufptr + len dpy-bufmax)
_XFlush(dpy);
+/* Request still too large, so do not allow it to overflow. */
+if (dpy-bufptr + len dpy-bufmax) {
+ fprintf(stderr,
+ Xlib: request %d length %zd would exceed buffer size.\n,
+ type, len);
+ /* Changes failure condition from overflow to NULL dereference. */
+ return NULL;
+}
if (len % 4)
fprintf(stderr,
--
1.8.1.2
--
-Alan Coopersmith- alan.coopersm...@oracle.com
Oracle Solaris Engineering - http://blogs.oracle.com/alanc
--
Kees Cook@outflux.net
___
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
Re: [PATCH 1/2] libX11: check size of GetReqExtra after XFlush
Re-re-ping. :) Can anyone commit these two patches please?
Thanks!
-Kees
On Sun, Jun 09, 2013 at 11:13:42AM -0700, Kees Cook wrote:
Two users of GetReqExtra pass arbitrarily sized allocations from the
caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
macro) to double-check the requested length and invalidate req when
this happens. Users of GetReqExtra passing lengths greater than the Xlib
buffer size (normally 16K) must check req and fail gracefully instead
of crashing.
Any callers of GetReqExtra that do not check req for NULL
will experience this change, in the pathological case, as a NULL
dereference instead of a buffer overflow. This is an improvement, but
the documentation for GetReqExtra has been updated to reflect the need
to check the value of req after the call.
Bug that manifested the problem:
https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
Signed-off-by: Kees Cook k...@outflux.net
---
specs/libX11/AppC.xml | 4 +++-
src/XlibInt.c | 8
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/specs/libX11/AppC.xml b/specs/libX11/AppC.xml
index df25027..0b37048 100644
--- a/specs/libX11/AppC.xml
+++ b/specs/libX11/AppC.xml
@@ -2468,7 +2468,9 @@ which is the same as
functionGetReq/function
except that it takes an additional argument (the number of
extra bytes to allocate in the output buffer after the request structure).
-This number should always be a multiple of four.
+This number should always be a multiple of four. Note that it is possible
+for req to be set to NULL as a defensive measure if the requested length
+exceeds the Xlib's buffer size (normally 16K).
/para
/sect2
sect2 id=Variable_Length_Arguments
diff --git a/src/XlibInt.c b/src/XlibInt.c
index b06e57b..c3273a8 100644
--- a/src/XlibInt.c
+++ b/src/XlibInt.c
@@ -1733,6 +1733,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t
len)
if (dpy-bufptr + len dpy-bufmax)
_XFlush(dpy);
+/* Request still too large, so do not allow it to overflow. */
+if (dpy-bufptr + len dpy-bufmax) {
+ fprintf(stderr,
+ Xlib: request %d length %zd would exceed buffer size.\n,
+ type, len);
+ /* Changes failure condition from overflow to NULL dereference. */
+ return NULL;
+}
if (len % 4)
fprintf(stderr,
--
1.8.1.2
--
Kees Cook@outflux.net
___
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
Re: [PATCH 1/2] libX11: check size of GetReqExtra after XFlush
Any comment on these patches? Can someone commit them if they're okay?
Thanks,
-Kees
On Sun, Jun 09, 2013 at 11:13:42AM -0700, Kees Cook wrote:
Two users of GetReqExtra pass arbitrarily sized allocations from the
caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
macro) to double-check the requested length and invalidate req when
this happens. Users of GetReqExtra passing lengths greater than the Xlib
buffer size (normally 16K) must check req and fail gracefully instead
of crashing.
Any callers of GetReqExtra that do not check req for NULL
will experience this change, in the pathological case, as a NULL
dereference instead of a buffer overflow. This is an improvement, but
the documentation for GetReqExtra has been updated to reflect the need
to check the value of req after the call.
Bug that manifested the problem:
https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
Signed-off-by: Kees Cook k...@outflux.net
---
specs/libX11/AppC.xml | 4 +++-
src/XlibInt.c | 8
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/specs/libX11/AppC.xml b/specs/libX11/AppC.xml
index df25027..0b37048 100644
--- a/specs/libX11/AppC.xml
+++ b/specs/libX11/AppC.xml
@@ -2468,7 +2468,9 @@ which is the same as
functionGetReq/function
except that it takes an additional argument (the number of
extra bytes to allocate in the output buffer after the request structure).
-This number should always be a multiple of four.
+This number should always be a multiple of four. Note that it is possible
+for req to be set to NULL as a defensive measure if the requested length
+exceeds the Xlib's buffer size (normally 16K).
/para
/sect2
sect2 id=Variable_Length_Arguments
diff --git a/src/XlibInt.c b/src/XlibInt.c
index b06e57b..c3273a8 100644
--- a/src/XlibInt.c
+++ b/src/XlibInt.c
@@ -1733,6 +1733,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t
len)
if (dpy-bufptr + len dpy-bufmax)
_XFlush(dpy);
+/* Request still too large, so do not allow it to overflow. */
+if (dpy-bufptr + len dpy-bufmax) {
+ fprintf(stderr,
+ Xlib: request %d length %zd would exceed buffer size.\n,
+ type, len);
+ /* Changes failure condition from overflow to NULL dereference. */
+ return NULL;
+}
if (len % 4)
fprintf(stderr,
--
1.8.1.2
--
Kees Cook@outflux.net
___
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
[PATCH v3 0/2] libX11: check size of GetReqExtra after XFlush
Thanks for the feedback! I've split the patch into the two halves and updated return values, etc. I'm not too familiar with the Data API, so I left things as-is for the time-being. If the second patch isn't right, hopefully the first is still useful. :) Thanks, -Kees ___ 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
[PATCH 2/2] libX11: check req when calling GetReqExtra
This fixes the two callers of GetReqExtra to check that req is non-NULL
to avoid crashing now that GetReqExtra does internal bounds-checking on
the resulting buffer sizes.
Additionally updates comment describing return values to use names
instead of only literal values.
Signed-off-by: Kees Cook k...@outflux.net
---
src/Host.c | 8
src/ModMap.c | 10 +++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/Host.c b/src/Host.c
index da9923a..da5e2f7 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 5c5b426..2aa2903 100644
--- a/src/ModMap.c
+++ b/src/ModMap.c
@@ -65,9 +65,9 @@ XGetModifierMapping(register Display *dpy)
/*
* Returns:
- * 0 Success
- * 1 Busy - one or more old or new modifiers are down
- * 2 Failed - one or more new modifiers unacceptable
+ * MappingSuccess (0) Success
+ * MappingBusy (1) Busy - one or more old or new modifiers are down
+ * MappingFailed (2) Failed - one or more new modifiers unacceptable
*/
int
XSetModifierMapping(
@@ -80,6 +80,10 @@ XSetModifierMapping(
LockDisplay(dpy);
GetReqExtra(SetModifierMapping, mapSize, req);
+if (!req) {
+ UnlockDisplay(dpy);
+ return MappingFailed;
+}
req-numKeyPerModifier = modifier_map-max_keypermod;
--
1.8.1.2
___
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
[PATCH 1/2] libX11: check size of GetReqExtra after XFlush
Two users of GetReqExtra pass arbitrarily sized allocations from the
caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
macro) to double-check the requested length and invalidate req when
this happens. Users of GetReqExtra passing lengths greater than the Xlib
buffer size (normally 16K) must check req and fail gracefully instead
of crashing.
Any callers of GetReqExtra that do not check req for NULL
will experience this change, in the pathological case, as a NULL
dereference instead of a buffer overflow. This is an improvement, but
the documentation for GetReqExtra has been updated to reflect the need
to check the value of req after the call.
Bug that manifested the problem:
https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
Signed-off-by: Kees Cook k...@outflux.net
---
specs/libX11/AppC.xml | 4 +++-
src/XlibInt.c | 8
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/specs/libX11/AppC.xml b/specs/libX11/AppC.xml
index df25027..0b37048 100644
--- a/specs/libX11/AppC.xml
+++ b/specs/libX11/AppC.xml
@@ -2468,7 +2468,9 @@ which is the same as
functionGetReq/function
except that it takes an additional argument (the number of
extra bytes to allocate in the output buffer after the request structure).
-This number should always be a multiple of four.
+This number should always be a multiple of four. Note that it is possible
+for req to be set to NULL as a defensive measure if the requested length
+exceeds the Xlib's buffer size (normally 16K).
/para
/sect2
sect2 id=Variable_Length_Arguments
diff --git a/src/XlibInt.c b/src/XlibInt.c
index b06e57b..c3273a8 100644
--- a/src/XlibInt.c
+++ b/src/XlibInt.c
@@ -1733,6 +1733,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t len)
if (dpy-bufptr + len dpy-bufmax)
_XFlush(dpy);
+/* Request still too large, so do not allow it to overflow. */
+if (dpy-bufptr + len dpy-bufmax) {
+ fprintf(stderr,
+ Xlib: request %d length %zd would exceed buffer size.\n,
+ type, len);
+ /* Changes failure condition from overflow to NULL dereference. */
+ return NULL;
+}
if (len % 4)
fprintf(stderr,
--
1.8.1.2
___
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
[PATCH v2] libX11: check size of GetReqExtra after XFlush
Two users of GetReqExtra pass arbitrarily sized allocations from the
caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
macro) to double-check the requested length and invalidate req when this
happens. Users of GetReqExtra passing lengths greater than the Xlib buffer
size (normally 16K) now check req and fail if something has gone wrong.
Any other callers of GetReqExtra that do not check req for NULL
will experience this change, in the pathological case, as a NULL
dereference instead of a buffer overflow. This is an improvement, but
the documentation for GetReqExtra has been updated to reflect the need
to check the value of req after the call.
Prior version of this patch:
http://lists.x.org/archives/xorg-devel/2011-July/023936.html
Bug that manifested the problem:
https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
Signed-off-by: Kees Cook k...@outflux.net
---
specs/libX11/AppC.xml | 4 +++-
src/Host.c| 8
src/ModMap.c | 4
src/XlibInt.c | 8
4 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/specs/libX11/AppC.xml b/specs/libX11/AppC.xml
index df25027..0b37048 100644
--- a/specs/libX11/AppC.xml
+++ b/specs/libX11/AppC.xml
@@ -2468,7 +2468,9 @@ which is the same as
functionGetReq/function
except that it takes an additional argument (the number of
extra bytes to allocate in the output buffer after the request structure).
-This number should always be a multiple of four.
+This number should always be a multiple of four. Note that it is possible
+for req to be set to NULL as a defensive measure if the requested length
+exceeds the Xlib's buffer size (normally 16K).
/para
/sect2
sect2 id=Variable_Length_Arguments
diff --git a/src/Host.c b/src/Host.c
index da9923a..da5e2f7 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 5c5b426..f66e559 100644
--- a/src/ModMap.c
+++ b/src/ModMap.c
@@ -80,6 +80,10 @@ XSetModifierMapping(
LockDisplay(dpy);
GetReqExtra(SetModifierMapping, mapSize, req);
+if (!req) {
+ UnlockDisplay(dpy);
+ return 2;
+}
req-numKeyPerModifier = modifier_map-max_keypermod;
diff --git a/src/XlibInt.c b/src/XlibInt.c
index b06e57b..c3273a8 100644
--- a/src/XlibInt.c
+++ b/src/XlibInt.c
@@ -1733,6 +1733,14 @@ void *_XGetRequest(Display *dpy, CARD8 type, size_t len)
if (dpy-bufptr + len dpy-bufmax)
_XFlush(dpy);
+/* Request still too large, so do not allow it to overflow. */
+if (dpy-bufptr + len dpy-bufmax) {
+ fprintf(stderr,
+ Xlib: request %d length %zd would exceed buffer size.\n,
+ type, len);
+ /* Changes failure condition from overflow to NULL dereference. */
+ return NULL;
+}
if (len % 4)
fprintf(stderr,
--
1.8.1.2
--
Kees Cook@outflux.net
___
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
Re: [PATCH] x86emu: improve single-step debugging
Hi Guillem, On Fri, Oct 07, 2011 at 02:49:03AM +0200, Guillem Jover wrote: On Thu, 2011-10-06 at 15:49:33 -0700, Alan Coopersmith wrote: On 10/ 6/11 03:36 PM, Kees Cook wrote: This allows for other consumers to do single-step decoding/emulation when using x86emu. Additionally adds a stand-alone Makefile for building out of tree, which is very handy for doing emulation debugging. What ever happened to the plan to move it out to a separate library that Xorg just depends on? We have another user here who is currently just copying the code from Xorg and would prefer to share a library with us. I thought libx86 [0] was supposed to be just that. [0] http://www.codon.org.uk/~mjg59/libx86/ So, until Xorg is linked against libx86, can my patch be applied to the Xorg emulator? From what I can see it is where fixes keep landing. Thanks, -Kees -- Kees Cook@outflux.net ___ 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
[PATCH] x86emu: improve single-step debugging
This allows for other consumers to do single-step decoding/emulation
when using x86emu. Additionally adds a stand-alone Makefile for building
out of tree, which is very handy for doing emulation debugging.
Signed-off-by: Kees Cook k...@outflux.net
---
forwarded from https://bugs.freedesktop.org/show_bug.cgi?id=17612
hw/xfree86/x86emu/debug.c |7 ++
hw/xfree86/x86emu/decode.c| 34 +---
hw/xfree86/x86emu/makefile.standalone | 13
hw/xfree86/x86emu/x86emu.h|1 +
hw/xfree86/x86emu/x86emu/debug.h |3 +-
hw/xfree86/x86emu/x86emu/regs.h |1 +
6 files changed, 46 insertions(+), 13 deletions(-)
create mode 100644 hw/xfree86/x86emu/makefile.standalone
diff --git a/hw/xfree86/x86emu/debug.c b/hw/xfree86/x86emu/debug.c
index 5eda908..0c5c784 100644
--- a/hw/xfree86/x86emu/debug.c
+++ b/hw/xfree86/x86emu/debug.c
@@ -177,8 +177,15 @@ void x86emu_decode_printf2 (char *x, int y)
M.x86.enc_str_pos += strlen(temp);
}
+void x86emu_start_instr (void)
+{
+ M.x86.inst_len = 0;
+ M.x86.decoded_buf[0]='\0';
+}
+
void x86emu_end_instr (void)
{
+ M.x86.inst_len = M.x86.enc_pos;
M.x86.enc_str_pos = 0;
M.x86.enc_pos = 0;
}
diff --git a/hw/xfree86/x86emu/decode.c b/hw/xfree86/x86emu/decode.c
index 9339f4c..22a5350 100644
--- a/hw/xfree86/x86emu/decode.c
+++ b/hw/xfree86/x86emu/decode.c
@@ -83,19 +83,13 @@ void x86emu_intr_raise(
}
/
-REMARKS:
-Main execution loop for the emulator. We return from here when the system
-halts, which is normally caused by a stack fault when we return from the
-original real mode call.
+perform single instruction step
+returns true/false value to indicate if the system should remain running
/
-void X86EMU_exec(void)
+inline int X86EMU_single_step(void)
{
u8 op1;
- M.x86.intr = 0;
- DB(x86emu_end_instr();)
-
-for (;;) {
DB(if (CHECK_IP_FETCH())
x86emu_check_ip_access();)
/* If debugging, save the IP and CS values. */
@@ -111,7 +105,7 @@ DB( if (M.x86.R_SP != 0) {
if (M.x86.debug)
printk(Service completed successfully\n);
})
- return;
+ return 0;
}
if (((M.x86.intr INTR_SYNCH) (M.x86.intno == 0 ||
M.x86.intno == 2)) ||
!ACCESS_FLAG(F_IF)) {
@@ -122,9 +116,25 @@ DB( if (M.x86.R_SP != 0) {
(*x86emu_optab[op1])(op1);
if (M.x86.debug DEBUG_EXIT) {
M.x86.debug = ~DEBUG_EXIT;
-return;
+return 0;
}
-}
+
+ return 1;
+}
+
+/
+REMARKS:
+Main execution loop for the emulator. We return from here when the system
+halts, which is normally caused by a stack fault when we return from the
+original real mode call.
+/
+void X86EMU_exec(void)
+{
+ M.x86.intr = 0;
+ DB(x86emu_end_instr();)
+
+ do {
+ } while (X86EMU_single_step());
}
/
diff --git a/hw/xfree86/x86emu/makefile.standalone
b/hw/xfree86/x86emu/makefile.standalone
new file mode 100644
index 000..6b594aa
--- /dev/null
+++ b/hw/xfree86/x86emu/makefile.standalone
@@ -0,0 +1,13 @@
+CC=gcc
+CFLAGS=-Wall -I. -DDEBUG
+
+TARGETS = debug.o decode.o fpu.o ops2.o ops.o prim_ops.o sys.o
+
+all: libx86emu.a
+
+libx86emu.a: $(TARGETS)
+ ar -r $@ $(TARGETS)
+
+clean:
+ rm -f $(TARGETS) libx86emu.a
+
diff --git a/hw/xfree86/x86emu/x86emu.h b/hw/xfree86/x86emu/x86emu.h
index 795e2d6..f16e8ab 100644
--- a/hw/xfree86/x86emu/x86emu.h
+++ b/hw/xfree86/x86emu/x86emu.h
@@ -153,6 +153,7 @@ voidX86EMU_prepareForInt(int num);
/* decode.c */
+inline int X86EMU_single_step(void);
void X86EMU_exec(void);
void X86EMU_halt_sys(void);
diff --git a/hw/xfree86/x86emu/x86emu/debug.h b/hw/xfree86/x86emu/x86emu/debug.h
index 47aacb6..6a08b39 100644
--- a/hw/xfree86/x86emu/x86emu/debug.h
+++ b/hw/xfree86/x86emu/x86emu/debug.h
@@ -151,7 +151,7 @@
SINGLE_STEP()
#ifdef DEBUG
-# define START_OF_INSTR()
+# define START_OF_INSTR() x86emu_start_instr();
# define END_OF_INSTR()EndOfTheInstructionProcedure:
x86emu_end_instr();
# define END_OF_INSTR_NO_TRACE() x86emu_end_instr();
#else
@@ -193,6 +193,7 @@ extern void x86emu_decode_printf (char *x);
extern void x86emu_decode_printf2 (char *x, int y);
extern void x86emu_just_disassemble (void);
extern void x86emu_single_step (void
Re: [PATCH] x86emu: improve single-step debugging
On Thu, Oct 06, 2011 at 03:49:33PM -0700, Alan Coopersmith wrote: On 10/ 6/11 03:36 PM, Kees Cook wrote: This allows for other consumers to do single-step decoding/emulation when using x86emu. Additionally adds a stand-alone Makefile for building out of tree, which is very handy for doing emulation debugging. What ever happened to the plan to move it out to a separate library that Xorg just depends on? We have another user here who is currently just copying the code from Xorg and would prefer to share a library with us. I'm not sure. All the emu projects I could find seemed to be early forks of the one in xorg, so this still looks like the best place to centralize any work on it. That said, yeah, it makes sense to split it out, but that's rather beyond the scope of what I'm trying to do. Hopefully that can be done separately from review of this patch. :) Thanks, -Kees -- Kees Cook@outflux.net ___ 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
Re: [PATCH] libX11: check size of GetReqExtra after XFlush
Hi, Thanks for the feedback! On Mon, Jul 18, 2011 at 10:06:24AM -0700, Jamey Sharp wrote: 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. Oh, hrm, sorry about that. I will fix it up. - 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. True, I will improve the commit message. - I guess this new behavior should be documented in specs/libX11/AppC.xml. Okay, noted. - 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? That was, perhaps, a bit obscure. I will rephrase this in terms of the Xlib buffer size. - 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. Right -- I tried to be careful with this. On the whole, I believe this is an improvement. With the above corrections, I'd be happy to commit this. Thanks, I'll send a v2 shortly. -Kees -- Kees Cook Ubuntu Security Team ___ 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
Re: [PATCH v2] xclipboard: avoid overflow crash when building labels
Hi Julien, On Tue, Jul 19, 2011 at 08:34:48PM +0200, Julien Cristau wrote: On Mon, Jul 18, 2011 at 13:06:51 -0700, Alan Coopersmith wrote: Since XtAsprintf is new in the recently released libXt 1.1.0, you need to update configure.ac to list xt = 1.1.0 in the PKG_CHECK_MODULES so that builders are properly notified of the version dependency. Pushed with that change. Thanks! -Kees -- Kees Cook Ubuntu Security Team ___ 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
Re: [PATCH] libX11: check size of GetReqExtra after XFlush
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 kees.c...@canonical.com
---
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
--
Kees Cook
Ubuntu Security Team
___
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
--
Kees Cook
Ubuntu Security Team
___
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
[PATCH v2] xclipboard: avoid overflow crash when building labels
This replaces sprintf with XtAsprintf to avoid crashing when creating
various potentially large labels.
https://bugs.launchpad.net/ubuntu/+source/x11-apps/+bug/792642
Signed-off-by: Kees Cook kees.c...@canonical.com
---
xclipboard.c |5 +++--
xcutsel.c|8 +---
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/xclipboard.c b/xclipboard.c
index 1fddf4c..62a214c 100644
--- a/xclipboard.c
+++ b/xclipboard.c
@@ -332,13 +332,14 @@ AcceptSaveFile(Widget w, XEvent *e, String *argv,
Cardinal *argc)
XtPopdown (fileDialogShell);
if (!success)
{
- charfailMessage[1024];
+ char*failMessage;
- sprintf (failMessage, Can't open file \%s\, filename);
+ XtAsprintf (failMessage, Can't open file \%s\, filename);
XtSetArg (args[0], XtNlabel, failMessage);
XtSetValues (failDialog, args, 1);
CenterWidgetOnEvent (failDialogShell, e);
XtPopup (failDialogShell, XtGrabNone);
+ XtFree (failMessage);
}
else
{
diff --git a/xcutsel.c b/xcutsel.c
index 690e201..3386b57 100644
--- a/xcutsel.c
+++ b/xcutsel.c
@@ -258,7 +258,7 @@ GetBuffer(Widget w, XtPointer closure, XtPointer callData)
int
main(int argc, char *argv[])
{
-char label[100];
+char *label;
Widget box, button;
XtAppContext appcon;
Widget shell;
@@ -288,19 +288,21 @@ main(int argc, char *argv[])
XtAddCallback( button, XtNcallback, Quit, NULL );
/* %%% hack alert... */
-sprintf(label, *label:copy %s to %d,
+XtAsprintf(label, *label:copy %s to %d,
options.selection_name,
options.buffer);
XrmPutLineResource( rdb, label );
+XtFree(label);
button =
XtCreateManagedWidget(sel-cut, commandWidgetClass, box, NULL, ZERO);
XtAddCallback( button, XtNcallback, GetSelection, NULL );
-sprintf(label, *label:copy %d to %s,
+XtAsprintf(label, *label:copy %d to %s,
options.buffer,
options.selection_name);
XrmPutLineResource( rdb, label );
+XtFree(label);
button =
XtCreateManagedWidget(cut-sel, commandWidgetClass, box, NULL, ZERO);
--
1.7.4.1
--
Kees Cook
Ubuntu Security Team
___
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
[PATCH] libX11: check size of GetReqExtra after XFlush
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 kees.c...@canonical.com
---
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
--
Kees Cook
Ubuntu Security Team
___
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
[PATCH] xhost: check return value of X{Add,Remove}Host
In the ServerInterpreted case, XAddHost and XRemoveHost are capable of
failing when they lack request buffer memory. Notice this situation,
and report correctly.
Signed-off-by: Kees Cook kees.c...@canonical.com
---
xhost.c | 11 +++
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/xhost.c b/xhost.c
index c7faeff..49d4690 100644
--- a/xhost.c
+++ b/xhost.c
@@ -450,7 +450,7 @@ change_host(Display *dpy, char *name, Bool add)
if (family == FamilyServerInterpreted) {
XServerInterpretedAddress siaddr;
- int namelen;
+ int namelen, rc;
cp = strchr(name, ':');
if (cp == NULL || cp == name) {
@@ -472,11 +472,14 @@ change_host(Display *dpy, char *name, Bool add)
siaddr.value = siaddr.type + siaddr.typelength + 1;
siaddr.valuelength = namelen - (siaddr.typelength + 1);
if (add)
- XAddHost(dpy, ha);
+ rc = XAddHost(dpy, ha);
else
- XRemoveHost(dpy, ha);
+ rc = XRemoveHost(dpy, ha);
free(siaddr.type);
- printf( %s %s\n, name, add ? add_msg : remove_msg);
+ printf( %s %s%s\n, name, rc == 1 ? : failed when ,
+ add ? add_msg : remove_msg);
+ if (rc != 1)
+ return 0;
return 1;
}
--
1.7.4.1
--
Kees Cook
Ubuntu Security Team
___
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
[PATCH] xclipboard: avoid overflow crash when building labels
This replaces sprintf with snprintf to avoid crashing when creating
various labels.
https://bugs.launchpad.net/ubuntu/+source/x11-apps/+bug/792642
Signed-off-by: Kees Cook kees.c...@canonical.com
---
xclipboard.c |3 ++-
xcutsel.c|4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/xclipboard.c b/xclipboard.c
index 1fddf4c..c6610e9 100644
--- a/xclipboard.c
+++ b/xclipboard.c
@@ -334,7 +334,8 @@ AcceptSaveFile(Widget w, XEvent *e, String *argv, Cardinal
*argc)
{
charfailMessage[1024];
- sprintf (failMessage, Can't open file \%s\, filename);
+ snprintf (failMessage, sizeof (failMessage),
+ Can't open file \%s\, filename);
XtSetArg (args[0], XtNlabel, failMessage);
XtSetValues (failDialog, args, 1);
CenterWidgetOnEvent (failDialogShell, e);
diff --git a/xcutsel.c b/xcutsel.c
index 690e201..7f33668 100644
--- a/xcutsel.c
+++ b/xcutsel.c
@@ -288,7 +288,7 @@ main(int argc, char *argv[])
XtAddCallback( button, XtNcallback, Quit, NULL );
/* %%% hack alert... */
-sprintf(label, *label:copy %s to %d,
+snprintf(label, sizeof(label), *label:copy %s to %d,
options.selection_name,
options.buffer);
XrmPutLineResource( rdb, label );
@@ -297,7 +297,7 @@ main(int argc, char *argv[])
XtCreateManagedWidget(sel-cut, commandWidgetClass, box, NULL, ZERO);
XtAddCallback( button, XtNcallback, GetSelection, NULL );
-sprintf(label, *label:copy %d to %s,
+snprintf(label, sizeof(label), *label:copy %d to %s,
options.buffer,
options.selection_name);
XrmPutLineResource( rdb, label );
--
1.7.4.1
--
Kees Cook
Ubuntu Security Team
___
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
Re: [PATCH] xclipboard: avoid overflow crash when building labels
Hi, On Fri, Jul 08, 2011 at 08:34:48PM +0200, Julien Cristau wrote: On Fri, Jul 8, 2011 at 11:01:45 -0700, Kees Cook wrote: This replaces sprintf with snprintf to avoid crashing when creating various labels. https://bugs.launchpad.net/ubuntu/+source/x11-apps/+bug/792642 Signed-off-by: Kees Cook kees.c...@canonical.com --- xclipboard.c |3 ++- xcutsel.c|4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) asprintf or XtAsprintf maybe? I've been told in the past not use asprintf because it isn't portable, and I do not find XtAsprintf in the include files: ../xclipboard/xclipboard.c:337:2: warning: implicit declaration of function ‘XtAsprintf’ ... ../xclipboard/xclipboard.c:337: undefined reference to `XtAsprintf' collect2: ld returned 1 exit status etc I'm not sure how I should proceed. :P Thanks, -Kees -- Kees Cook Ubuntu Security Team ___ 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
