Skip to contentSkip to conversation

Fix PVFB backend to validate frontend's frame buffer description

PATCH New Markus Armbruster 3 participants 7 messages May 13, 2008 – May 19, 2008

lore
Share & tools
lore

Markus Armbruster

author
#0 lore raw

A buggy or malicious frontend can describe its shared framebuffer to the backend in a way that makes the backend map an arbitrary amount of guest memory, malloc an arbitrarily large internal buffer, copy arbitrary memory to that buffer, even beyond its end. A domU running a malicious frontend can abuse the former two for denial of service attacks against dom0. It can abuse the third to write arbitrary backend memory. It can abuse all three to terminate or crash the backend. Arbitrary code execution looks quite feasible.

In more detail (ignoring #ifdef CONFIG_STUBDOM code):

The frame buffer is described by the following parameters:

* fb_len (size of shared framebuffer)

* width, height, depth

* row_stride, offset

fb_len is fixed on startup. The frontend can modify the other parameters by sending a XENFB_TYPE_RESIZE event.

xenfb_read_frontend_fb_config() limits fb_len according to backend configuration parameter videoram (from xenstore), if present. I believe videoram is not present by default.

xenfb_map_fb() uses fb_len to map the frontend's framebuffer.

The frontend can make it map arbitrarily much, unless limited by the videoram configuration parameter.

This flaw always existed.

xenfb_register_console() and xenfb_on_fb_event() pass width, height, depth and rowstride to QEMU's DisplayState object. The object sets itself up to work directly on the framebuffer (shared_buf true) if parameters allow that. Else it allocates an internal buffer of size height * width * depth / 8 (shared_buf false).

The frontend can make it allocate arbitrarily much.

This flaw was introduced by the move of PVFB into QEMU (xen-unstable cset 16220 ff).

xenfb_on_fb_event() uses width and height to clip the area of an update event. It then passes that area to xenfb_guest_copy(). xenfb_invalidate() passes the complete screen area to xenfb_guest_copy(). xenfb_guest_copy() copies the argument area (x, y, w, h) into the internal buffer, unless shared_buf is true. This copies h blocks of memory. The i-th copy (counting from zero) copies

w * depth / 8 bytes

from

shared framebuffer + offset + (y + i) * row_stride + x * depth / 8

to

    internal buffer + (y + i) * ds->linesize + x * ds->depth / 8

where ds->linesize and ds->depth are parameters of the internal buffer chosen by the backend.

This copy can be made to read from the shared framebuffer and write to the internal buffer out of bounds. I believe the frontend can abuse this to write arbitrary backend memory.

The flaw in its current form was introduced by the move of PVFB into QEMU (xen-unstable cset 16220 ff). Before, the framebuffer was always shared. Bad things could happen even in the shared case down in the bowels of SDL or LibVNCServer (before move to QEMU) or the QEMU's VNC code (after), I don't know.

The appended patch is *untested*, but very similar patches work with our code.

Regards,

Markus

tools/ioemu/hw/xenfb.c +79 -28
diff -r 0a8fc1a62796 tools/ioemu/hw/xenfb.c
@@ -28,8 +28,6 @@
 #ifndef BTN_LEFT
 #define BTN_LEFT 0x110 /* from <linux/input.h> */
 #endif
-
-// FIXME defend against malicious frontend?
 
 struct xenfb;
 

@@ -484,6 +482,68 @@ void xenfb_shutdown(struct xenfb *xenfb)
 	free(xenfb);
 }
 
+static int xenfb_configure_fb(struct xenfb *xenfb, size_t fb_len_lim,
+			      int width, int height, int depth,
+			      size_t fb_len, int offset, int row_stride)
+{
+	size_t mfn_sz = sizeof(*((struct xenfb_page *)0)->pd);
+	size_t pd_len = sizeof(((struct xenfb_page *)0)->pd) / mfn_sz;
+	size_t fb_pages = pd_len * XC_PAGE_SIZE / mfn_sz;
+	size_t fb_len_max = fb_pages * XC_PAGE_SIZE;
+	int max_width, max_height;
+
+	if (fb_len_lim > fb_len_max) {
+		fprintf(stderr,
+			"FB: fb size limit %zu exceeds %zu, corrected\n",
+			fb_len_lim, fb_len_max);
+		fb_len_lim = fb_len_max;
+	}
+	if (fb_len > fb_len_lim) {
+		fprintf(stderr,
+			"FB: frontend fb size %zu limited to %zu\n",
+			fb_len, fb_len_lim);
+	}
+	if (depth != 8 && depth != 16 && depth != 24 && depth != 32) {
+		fprintf(stderr,
+			"FB: can't handle frontend fb depth %d\n",
+			depth);
+		return -1;
+	}
+	if (row_stride < 0 || row_stride > fb_len) {
+		fprintf(stderr,
+			"FB: invalid frontend stride %d\n", row_stride);
+		return -1;
+	}
+	max_width = row_stride / (depth / 8);
+	if (width < 0 || width > max_width) {
+		fprintf(stderr,
+			"FB: invalid frontend width %d limited to %d\n",
+			width, max_width);
+		width = max_width;
+	}
+	if (offset < 0 || offset >= fb_len) {
+		fprintf(stderr,
+			"FB: invalid frontend offset %d (max %zu)\n",
+			offset, fb_len - 1);
+		return -1;
+	}
+	max_height = (fb_len - offset) / row_stride;
+	if (height < 0 || height > max_height) {
+		fprintf(stderr,
+			"FB: invalid frontend height %d limited to %d\n",
+			height, max_height);
+		height = max_height;
+	}
+	xenfb->fb_len = fb_len;
+	xenfb->row_stride = row_stride;
+	xenfb->depth = depth;
+	xenfb->width = width;
+	xenfb->height = height;
+	xenfb->offset = offset;
+	fprintf(stderr, "Framebuffer %dx%dx%d offset %d stride %d\n",
+		width, height, depth, offset, row_stride);
+	return 0;
+}
 
 static void xenfb_on_fb_event(struct xenfb *xenfb)
 {

@@ -514,16 +574,18 @@ static void xenfb_on_fb_event(struct xen
 			    || h != event->update.height) {
 				fprintf(stderr, "%s bogus update clipped\n",
 					xenfb->fb.nodename);
-				break;
 			}
 			xenfb_guest_copy(xenfb, x, y, w, h);
 			break;
 		case XENFB_TYPE_RESIZE:
-			xenfb->width  = event->resize.width;
-			xenfb->height = event->resize.height;
-			xenfb->depth = event->resize.depth;
-			xenfb->row_stride = event->resize.stride;
-			xenfb->offset = event->resize.offset;
+			if (xenfb_configure_fb(xenfb, xenfb->fb_len,
+					       event->resize.width,
+					       event->resize.height,
+					       event->resize.depth,
+					       xenfb->fb_len,
+					       event->resize.offset,
+					       event->resize.stride) < 0)
+				break;
 			dpy_colourdepth(xenfb->ds, xenfb->depth);
 			dpy_resize(xenfb->ds, xenfb->width, xenfb->height, xenfb->row_stride);
 			if (xenfb->ds->shared_buf)

@@ -745,29 +807,18 @@ static int xenfb_read_frontend_fb_config
         xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "1");
         xenfb->refresh_period = -1;
 
-        /* TODO check for permitted ranges */
-        fb_page = xenfb->fb.page;
-        xenfb->depth = fb_page->depth;
-        xenfb->width = fb_page->width;
-        xenfb->height = fb_page->height;
-        /* TODO check for consistency with the above */
-        xenfb->fb_len = fb_page->mem_length;
-        xenfb->row_stride = fb_page->line_length;
-
-        /* Protect against hostile frontend, limit fb_len to max allowed */
         if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d",
                             &videoram) < 0)
                 videoram = 0;
-        videoram = videoram * 1024 * 1024;
-        if (videoram && xenfb->fb_len > videoram) {
-                fprintf(stderr, "Framebuffer requested length of %zd exceeded allowed %d\n",
-                        xenfb->fb_len, videoram);
-                xenfb->fb_len = videoram;
-                if (xenfb->row_stride * xenfb->height > xenfb->fb_len)
-                        xenfb->height = xenfb->fb_len / xenfb->row_stride;
-        }
-        fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n",
-                fb_page->depth, fb_page->width, fb_page->height, fb_page->line_length);
+	fb_page = xenfb->fb.page;
+	if (xenfb_configure_fb(xenfb, videoram * 1024 * 1024U,
+			       fb_page->width, fb_page->height, fb_page->depth,
+			       fb_page->mem_length, 0, fb_page->line_length)
+	    < 0) {
+		errno = EINVAL;
+		return -1;
+	}
+
         if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0)
 		return -1;
#1 lore raw
Quoted 4 lines · Markus Armbruster

A buggy or malicious frontend can describe its shared framebuffer to the backend in a way that makes the backend map an arbitrary amount of

snipped out, see inline question below.

Quoted 28 lines

diff -r 0a8fc1a62796 tools/ioemu/hw/xenfb.c

tools/ioemu/hw/xenfb.c +11 -2
@@ -28,8 +28,6 @@
 #ifndef BTN_LEFT
 #define BTN_LEFT 0x110 /* from <linux/input.h> */
 #endif
-
-// FIXME defend against malicious frontend?
 
 struct xenfb;
 

@@ -484,6 +482,68 @@ void xenfb_shutdown(struct xenfb *xenfb)
 	free(xenfb);
 }
 
+static int xenfb_configure_fb(struct xenfb *xenfb, size_t fb_len_lim,
+			      int width, int height, int depth,
+			      size_t fb_len, int offset, int row_stride)
+{
+	size_t mfn_sz = sizeof(*((struct xenfb_page *)0)->pd);
+	size_t pd_len = sizeof(((struct xenfb_page *)0)->pd) / mfn_sz;
+	size_t fb_pages = pd_len * XC_PAGE_SIZE / mfn_sz;
+	size_t fb_len_max = fb_pages * XC_PAGE_SIZE;
+	int max_width, max_height;
+
+	if (fb_len_lim > fb_len_max) {

tools/ioemu/hw/xenfb.c void xenfb_shutdown(struct xenfb *xenfb) · line 505

Quoted 10 lines
tools/ioemu/hw/xenfb.c +9 -0
+		fprintf(stderr,
+			"FB: fb size limit %zu exceeds %zu, corrected\n",
+			fb_len_lim, fb_len_max);
+		fb_len_lim = fb_len_max;
+	}
+	if (fb_len > fb_len_lim) {
+		fprintf(stderr,
+			"FB: frontend fb size %zu limited to %zu\n",
+			fb_len, fb_len_lim);

Do we need to set fb_len to fb_len_lim here?
fb_len = fb_len_lim;

Quoted 115 lines
tools/ioemu/hw/xenfb.c +42 -0
+	}
+	if (depth != 8 && depth != 16 && depth != 24 && depth != 32) {
+		fprintf(stderr,
+			"FB: can't handle frontend fb depth %d\n",
+			depth);
+		return -1;
+	}
+	if (row_stride < 0 || row_stride > fb_len) {
+		fprintf(stderr,
+			"FB: invalid frontend stride %d\n", row_stride);
+		return -1;
+	}
+	max_width = row_stride / (depth / 8);
+	if (width < 0 || width > max_width) {
+		fprintf(stderr,
+			"FB: invalid frontend width %d limited to %d\n",
+			width, max_width);
+		width = max_width;
+	}
+	if (offset < 0 || offset >= fb_len) {
+		fprintf(stderr,
+			"FB: invalid frontend offset %d (max %zu)\n",
+			offset, fb_len - 1);
+		return -1;
+	}
+	max_height = (fb_len - offset) / row_stride;
+	if (height < 0 || height > max_height) {
+		fprintf(stderr,
+			"FB: invalid frontend height %d limited to %d\n",
+			height, max_height);
+		height = max_height;
+	}
+	xenfb->fb_len = fb_len;
+	xenfb->row_stride = row_stride;
+	xenfb->depth = depth;
+	xenfb->width = width;
+	xenfb->height = height;
+	xenfb->offset = offset;
+	fprintf(stderr, "Framebuffer %dx%dx%d offset %d stride %d\n",
+		width, height, depth, offset, row_stride);
+	return 0;
+}
 
 static void xenfb_on_fb_event(struct xenfb *xenfb)

{

tools/ioemu/hw/xenfb.c +17 -26
@@ -514,16 +574,18 @@ static void xenfb_on_fb_event(struct xen
 			    || h != event->update.height) {
 				fprintf(stderr, "%s bogus update clipped\n",
 					xenfb->fb.nodename);
-				break;
 			}
 			xenfb_guest_copy(xenfb, x, y, w, h);
 			break;
 		case XENFB_TYPE_RESIZE:
-			xenfb->width  = event->resize.width;
-			xenfb->height = event->resize.height;
-			xenfb->depth = event->resize.depth;
-			xenfb->row_stride = event->resize.stride;
-			xenfb->offset = event->resize.offset;
+			if (xenfb_configure_fb(xenfb, xenfb->fb_len,
+					       event->resize.width,
+					       event->resize.height,
+					       event->resize.depth,
+					       xenfb->fb_len,
+					       event->resize.offset,
+					       event->resize.stride) < 0)
+				break;
 			dpy_colourdepth(xenfb->ds, xenfb->depth);
 			dpy_resize(xenfb->ds, xenfb->width, xenfb->height, xenfb->row_stride);
 			if (xenfb->ds->shared_buf)

@@ -745,29 +807,18 @@ static int xenfb_read_frontend_fb_config
         xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "1");
         xenfb->refresh_period = -1;
 
-        /* TODO check for permitted ranges */
-        fb_page = xenfb->fb.page;
-        xenfb->depth = fb_page->depth;
-        xenfb->width = fb_page->width;
-        xenfb->height = fb_page->height;
-        /* TODO check for consistency with the above */
-        xenfb->fb_len = fb_page->mem_length;
-        xenfb->row_stride = fb_page->line_length;
-
-        /* Protect against hostile frontend, limit fb_len to max allowed */
         if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d",
                             &videoram) < 0)
                 videoram = 0;
-        videoram = videoram * 1024 * 1024;
-        if (videoram && xenfb->fb_len > videoram) {
-                fprintf(stderr, "Framebuffer requested length of %zd exceeded allowed %d\n",
-                        xenfb->fb_len, videoram);
-                xenfb->fb_len = videoram;
-                if (xenfb->row_stride * xenfb->height > xenfb->fb_len)
-                        xenfb->height = xenfb->fb_len / xenfb->row_stride;
-        }
-        fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n",
-                fb_page->depth, fb_page->width, fb_page->height, fb_page->line_length);
+	fb_page = xenfb->fb.page;
+	if (xenfb_configure_fb(xenfb, videoram * 1024 * 1024U,
+			       fb_page->width, fb_page->height, fb_page->depth,
+			       fb_page->mem_length, 0, fb_page->line_length)
+	    < 0) {
+		errno = EINVAL;
+		return -1;
+	}
+
         if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0)
 		return -1;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#2 lore raw
Quoted 11 lines · Pat Campbell
Quoted 4 lines · Markus Armbruster

A buggy or malicious frontend can describe its shared framebuffer to the backend in a way that makes the backend map an arbitrary amount of

snipped out, see inline question below.

Quoted 5 lines

diff -r 0a8fc1a62796 tools/ioemu/hw/xenfb.c

tools/ioemu/hw/xenfb.c +0 -0
@@ -28,8 +28,6 @@

[....]

Quoted 15 lines
Quoted 15 lines
tools/ioemu/hw/xenfb.c +15 -0
+static int xenfb_configure_fb(struct xenfb *xenfb, size_t fb_len_lim,
+			      int width, int height, int depth,
+			      size_t fb_len, int offset, int row_stride)
+{
+	size_t mfn_sz = sizeof(*((struct xenfb_page *)0)->pd);
+	size_t pd_len = sizeof(((struct xenfb_page *)0)->pd) / mfn_sz;
+	size_t fb_pages = pd_len * XC_PAGE_SIZE / mfn_sz;
+	size_t fb_len_max = fb_pages * XC_PAGE_SIZE;
+	int max_width, max_height;
+
+	if (fb_len_lim > fb_len_max) {
+		fprintf(stderr,
+			"FB: fb size limit %zu exceeds %zu, corrected\n",
+			fb_len_lim, fb_len_max);
+		fb_len_lim = fb_len_max;
Quoted 8 lines

tools/ioemu/hw/xenfb.c void xenfb_shutdown(struct xenfb *xenfb) · line 504

Quoted 6 lines
tools/ioemu/hw/xenfb.c +5 -0
+	}
+	if (fb_len > fb_len_lim) {
+		fprintf(stderr,
+			"FB: frontend fb size %zu limited to %zu\n",
+			fb_len, fb_len_lim);

Do we need to set fb_len to fb_len_lim here?
fb_len = fb_len_lim;

Yes, we do! Good catch, thank you. I have no idea how that got lost. I'll post a patch.

[...]

author
#3 lore raw

PATCH 1/1 ioemu: Fix PVFB backend to limit frame buffer size

In reply to Markus Armbruster

The recent fix to validate the frontend's frame buffer description neglected to limit the frame buffer size correctly. This lets a malicious frontend make the backend attempt to map an arbitrary amount of guest memory, which could be useful for a denial of service attack against dom0.

Signed-off-by Markus Armbruster <[email protected]>
tools/ioemu/hw/xenfb.c +1 -0
diff -r 53195719f762 tools/ioemu/hw/xenfb.c
@@ -502,6 +502,7 @@ static int xenfb_configure_fb(struct xen
 		fprintf(stderr,
 			"FB: frontend fb size %zu limited to %zu\n",
 			fb_len, fb_len_lim);
+		fb_len = fb_len_lim;
 	}
 	if (depth != 8 && depth != 16 && depth != 24 && depth != 32) {
 		fprintf(stderr,
author
#4 lore raw

PATCH 1/1 ioemu: Fix interpretation of missing or zero vfb videoram

Changeset 17289:d97e61001d81: introduced vfb configuration parameter videoram, defaulting to zero. Value zero was interpreted as unlimited. Changeset 17630:53195719f762 accidentally dropped the special case for zero, which broke guests that don't specify videoram, or specify videoram=0. Restore the old behavior.

Signed-off-by Markus Armbruster <[email protected]>
tools/ioemu/hw/xenfb.c +1 -1
diff -r e66aefdfedcc tools/ioemu/hw/xenfb.c
@@ -498,7 +498,7 @@ static int xenfb_configure_fb(struct xen
 			fb_len_lim, fb_len_max);
 		fb_len_lim = fb_len_max;
 	}
-	if (fb_len > fb_len_lim) {
+	if (fb_len_lim && fb_len > fb_len_lim) {
 		fprintf(stderr,
 			"FB: frontend fb size %zu limited to %zu\n",
 			fb_len, fb_len_lim);
#5 lore raw
In reply to Markus Armbruster

Hi,

Sorry for late reply.
I found that with this patch applied, qemu-dm got stuck while creating a domain that had vfb but no 'videoram' parameter in its configuration. It seems when no 'videoram' is specified, videoram is set to 0 and xenfb_configure_fb() fails. It results in qemu-dm abort.
Is this an expected behavior? I think we should allow guest domains to use at least a few MBs of videoram by default.

Thanks,
--Yosuke

Quoted 44 lines · Markus Armbruster
tools/ioemu/hw/xenfb.c +9 -20
@@ -745,29 +807,18 @@ static int xenfb_read_frontend_fb_config
         xenfb_xs_printf(xenfb->xsh, xenfb->fb.nodename, "request-update", "1");
         xenfb->refresh_period = -1;
 
-        /* TODO check for permitted ranges */
-        fb_page = xenfb->fb.page;
-        xenfb->depth = fb_page->depth;
-        xenfb->width = fb_page->width;
-        xenfb->height = fb_page->height;
-        /* TODO check for consistency with the above */
-        xenfb->fb_len = fb_page->mem_length;
-        xenfb->row_stride = fb_page->line_length;
-
-        /* Protect against hostile frontend, limit fb_len to max allowed */
         if (xenfb_xs_scanf1(xenfb->xsh, xenfb->fb.nodename, "videoram", "%d",
                             &videoram) < 0)
                 videoram = 0;
-        videoram = videoram * 1024 * 1024;
-        if (videoram && xenfb->fb_len > videoram) {
-                fprintf(stderr, "Framebuffer requested length of %zd exceeded allowed %d\n",
-                        xenfb->fb_len, videoram);
-                xenfb->fb_len = videoram;
-                if (xenfb->row_stride * xenfb->height > xenfb->fb_len)
-                        xenfb->height = xenfb->fb_len / xenfb->row_stride;
-        }
-        fprintf(stderr, "Framebuffer depth %d width %d height %d line %d\n",
-                fb_page->depth, fb_page->width, fb_page->height, fb_page->line_length);
+	fb_page = xenfb->fb.page;
+	if (xenfb_configure_fb(xenfb, videoram * 1024 * 1024U,
+			       fb_page->width, fb_page->height, fb_page->depth,
+			       fb_page->mem_length, 0, fb_page->line_length)
+	    < 0) {
+		errno = EINVAL;
+		return -1;
+	}
+
         if (xenfb_map_fb(xenfb, xenfb->fb.otherend_id) < 0)
 		return -1;

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

author
#6 lore raw
Quoted 10 lines · Yosuke Iwamatsu

Hi,

Sorry for late reply.
I found that with this patch applied, qemu-dm got stuck while creating a domain that had vfb but no 'videoram' parameter in its configuration. It seems when no 'videoram' is specified, videoram is set to 0 and xenfb_configure_fb() fails. It results in qemu-dm abort.
Is this an expected behavior? I think we should allow guest domains to use at least a few MBs of videoram by default.

Quoted 2 lines

Thanks,
--Yosuke

No, this is not expected. Thanks for testing. I'll post a patch.