[PATCH] server: Add API to protect access to an SHM buffer

[Neil Roberts]

Linux will let you mmap a region of a file that is larger than the
size of the file. If you then try to read from that region the process
will get a SIGBUS signal. Currently the clients can use this to crash
a compositor because it can create a pool and lie about the size of
the file which will cause the compositor to try and read past the end
of it. The compositor can't simply check the size of the file to
verify that it is big enough because then there is a race condition
where the client may truncate the file after the check is performed.

This patch adds the following two public functions in the server API
which can be used wrap access to an SHM buffer:

void wl_shm_buffer_begin_access(struct wl_shm_buffer *buffer);
void wl_shm_buffer_end_access(struct wl_shm_buffer *buffer);

In-between calls to these functions there will be a signal handler for
SIGBUS. If the signal is caught then the buffer is remapped to an
anonymous private buffer at the same address which allows the
compositor to continue without crashing. The end_access function will
then post an error to the buffer resource.
---

This problem was pointed out earlier by wm4 on #wayland and it seems
like quite a thorny issue. Here is a first attempt at a patch which
does seem to work for Weston but I think there are plenty of issues to
think about. It might not be such a good idea to be messing with
signal handlers if the compositor itself also wants to handle SIGBUS.
There are probably some threading concerns here too.

I think you could do the patch without adding any extra API and just
make it automatically work out which region to remap if you had a list
of wl_shm_pools. The signal handler gets passed the address so it
could check which pool contains it and just remap that one. However
the end_access function seems like a nice place to post an event back
to the client when it fails so I'm not sure which way is better. It's
also probably a good idea to keep the signal handler as simple as
possible.

 src/wayland-server.h |  6 ++++
 src/wayland-shm.c    | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/src/wayland-server.h b/src/wayland-server.h
index c93987d..b371c9e 100644
--- a/src/wayland-server.h
+++ b/src/wayland-server.h
@@ -412,6 +412,12 @@ wl_resource_get_destroy_listener(struct wl_resource *resource,
 
 struct wl_shm_buffer;
 
+void
+wl_shm_buffer_begin_access(struct wl_shm_buffer *buffer);
+
+void
+wl_shm_buffer_end_access(struct wl_shm_buffer *buffer);
+
 struct wl_shm_buffer *
 wl_shm_buffer_get(struct wl_resource *resource);
 
diff --git a/src/wayland-shm.c b/src/wayland-shm.c
index eff29c3..95b1f33 100644
--- a/src/wayland-shm.c
+++ b/src/wayland-shm.c
@@ -32,15 +32,22 @@
 #include <string.h>
 #include <sys/mman.h>
 #include <unistd.h>
+#include <assert.h>
+#include <signal.h>
 
 #include "wayland-private.h"
 #include "wayland-server.h"
 
+static struct wl_shm_pool *wl_shm_current_pool;
+static struct sigaction wl_shm_old_sigbus_action;
+
 struct wl_shm_pool {
 	struct wl_resource *resource;
 	int refcount;
 	char *data;
 	int size;
+	int access_count;
+	int fallback_mapping_used;
 };
 
 struct wl_shm_buffer {
@@ -219,6 +226,7 @@ shm_create_pool(struct wl_client *client, struct wl_resource *resource,
 
 	pool->refcount = 1;
 	pool->size = size;
+	pool->fallback_mapping_used = 0;
 	pool->data = mmap(NULL, size,
 			  PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
 	if (pool->data == MAP_FAILED) {
@@ -368,3 +376,82 @@ wl_shm_buffer_get_height(struct wl_shm_buffer *buffer)
 {
 	return buffer->height;
 }
+
+static void
+unset_sigbus_handler(void)
+{
+	sigaction(SIGBUS, &wl_shm_old_sigbus_action, NULL);
+}
+
+static void
+sigbus_handler(int signum, siginfo_t *info, void *context)
+{
+	struct wl_shm_pool *pool = wl_shm_current_pool;
+
+	unset_sigbus_handler();
+
+	/* If the offending address is outside the mapped space for
+	 * the pool then the error is a real problem so we'll reraise
+	 * the signal */
+	if ((char *) info->si_addr < pool->data ||
+	    (char *) info->si_addr >= pool->data + pool->size)
+		raise(signum);
+
+	pool->fallback_mapping_used = 1;
+
+	/* This should replace the previous mapping */
+	if (mmap(pool->data, pool->size,
+		 PROT_READ | PROT_WRITE,
+		 MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
+		 0, 0) == (void *) -1)
+		raise(signum);
+}
+
+static void
+set_sigbus_handler(void)
+{
+	struct sigaction new_action = {
+		.sa_sigaction = sigbus_handler,
+		.sa_flags = SA_SIGINFO | SA_NODEFER
+	};
+
+	sigemptyset(&new_action.sa_mask);
+
+	sigaction(SIGBUS, &new_action, &wl_shm_old_sigbus_action);
+}
+
+WL_EXPORT void
+wl_shm_buffer_begin_access(struct wl_shm_buffer *buffer)
+{
+	struct wl_shm_pool *pool = buffer->pool;
+
+	assert(wl_shm_current_pool == NULL || wl_shm_current_pool == pool);
+
+	if (pool->access_count == 0) {
+		wl_shm_current_pool = pool;
+
+		if (!pool->fallback_mapping_used)
+			set_sigbus_handler();
+	}
+
+	pool->access_count++;
+}
+
+WL_EXPORT void
+wl_shm_buffer_end_access(struct wl_shm_buffer *buffer)
+{
+	struct wl_shm_pool *pool = buffer->pool;
+
+	assert(pool->access_count >= 1);
+
+	if (--pool->access_count == 0) {
+		if (pool->fallback_mapping_used) {
+			wl_resource_post_error(buffer->resource,
+					       WL_SHM_ERROR_INVALID_FD,
+					       "error accessing SHM buffer");
+		} else {
+			unset_sigbus_handler();
+		}
+		wl_shm_current_pool = NULL;
+	}
+}
-- 
1.8.3.1