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

[José Bollo]

That is a really interesting point.
I have two questions about it:
 - Is it normal that the client trucates the buffer? Is your patch
   designed to allow normal operations? or to allow forbiden uses?
 - If it is not "normal", is there good reasons to continue to 
   serve a nasty client?
Just curiousity.
regards
José

On dt, 2013-10-01 at 00:51 +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.
> 
>  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;
> +	}
> +}

-- 
José Bollo
Intel Opensource Technology Center
+33.612.137.110