[PATCH] server: Add API to protect access to an SHM buffer
Kristian Høgsberg
hoegsberg at gmail.com
Tue Nov 12 15:50:08 PST 2013
On Tue, Oct 01, 2013 at 12:51:29AM +0100, Neil Roberts wrote:
> 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.
Hi Neil,
I almost forgot about these. We should fix this for 1.4.
I think the begin/end access pattern is a good compromise. It would
be nice to not have to do that, but I don't like the idea of having to
traverse that list of wl_shm_pools in a signal handler. Jason is
right though that we need to make it thread safe by using TLS for the
current pool.
Finally, I think we should just leave the signal handler installed.
We can do it on demand in the first call to begin_access.
If we run into a problem with compositors that want to handle their
own SIGBUS signals, we can provide new API to prevent
libwayland-server from installing the signal handler as well as an
entry point the compositor can call from its SIGBUS handler to deal
with wayland SIGBUS cases.
Kristian
> 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
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list