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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel