Neil, While I think this patch will work in the single-threaded case, I'm not a big fan of the api for two reasons: 1) without doing thread-local things it is inherently single-threaded. (This is obviously not an issue for Weston, but requiring all shm buffer reads to be on the Wayland fd thread could be a big issue for others.) 2) it doesn't allow for integration with another SIGBUS/SEGEV handler. My knowledge of thread-local stuff isn't great, so (1) may be a non-issue in the case where we're not fighting with another signal handler. However, I do think it would be good to provide better integration with other handlers. Perhaps a function that takes a "bad" pointer and checks to see if it is in an active pool? Thanks, --Jason Ekstrand <div class="gmail_quote">On Sep 30, 2013 6:52 PM, "Neil Roberts" <<a href="mailto:neil@linux.intel.com">neil@linux.intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div>