Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 3fedc0cd authored by Greg Hackmann's avatar Greg Hackmann Committed by Greg Kroah-Hartman
Browse files

staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free 



This patch is 4.9.y only.  Kernels 4.12 and later are unaffected, since
all the underlying ion_handle infrastructure has been ripped out.

The ION_IOC_{MAP,SHARE} ioctls drop and reacquire client->lock several
times while operating on one of the client's ion_handles.  This creates
windows where userspace can call ION_IOC_FREE on the same client with
the same handle, and effectively make the kernel drop its own reference.
For example:

- thread A: ION_IOC_ALLOC creates an ion_handle with refcount 1
- thread A: starts ION_IOC_MAP and increments the refcount to 2
- thread B: ION_IOC_FREE decrements the refcount to 1
- thread B: ION_IOC_FREE decrements the refcount to 0 and frees the
            handle
- thread A: continues ION_IOC_MAP with a dangling ion_handle * to
            freed memory

Fix this by holding client->lock for the duration of
ION_IOC_{MAP,SHARE}, preventing the concurrent ION_IOC_FREE.  Also
remove ion_handle_get_by_id(), since there's literally no way to use it
safely.

Cc: stable@vger.kernel.org # v4.11-
Signed-off-by: default avatarGreg Hackmann <ghackmann@google.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent e85b9fbd

drivers/staging/android/ion/ion-ioctl.c

+8 −4
Original line number Diff line number Diff line
@@ -128,11 +128,15 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) 
	{

		struct ion_handle *handle;



		handle = ion_handle_get_by_id(client, data.handle.handle);

		if (IS_ERR(handle))

		mutex_lock(&client->lock);

		handle = ion_handle_get_by_id_nolock(client, data.handle.handle);

		if (IS_ERR(handle)) {

			mutex_unlock(&client->lock);

			return PTR_ERR(handle);

		data.fd.fd = ion_share_dma_buf_fd(client, handle);

		ion_handle_put(handle);

		}

		data.fd.fd = ion_share_dma_buf_fd_nolock(client, handle);

		ion_handle_put_nolock(handle);

		mutex_unlock(&client->lock);

		if (data.fd.fd < 0)

			ret = data.fd.fd;

		break;

drivers/staging/android/ion/ion.c

+29 −19
Original line number Diff line number Diff line
@@ -352,18 +352,6 @@ struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client, 
	return handle ? handle : ERR_PTR(-EINVAL);

}



struct ion_handle *ion_handle_get_by_id(struct ion_client *client,

					       int id)

{

	struct ion_handle *handle;



	mutex_lock(&client->lock);

	handle = ion_handle_get_by_id_nolock(client, id);

	mutex_unlock(&client->lock);



	return handle;

}



static bool ion_handle_validate(struct ion_client *client,

				struct ion_handle *handle)

{
@@ -1029,23 +1017,27 @@ static struct dma_buf_ops dma_buf_ops = { 
	.kunmap = ion_dma_buf_kunmap,

};



struct dma_buf *ion_share_dma_buf(struct ion_client *client,

				  struct ion_handle *handle)

static struct dma_buf *__ion_share_dma_buf(struct ion_client *client,

					   struct ion_handle *handle,

					   bool lock_client)

{

	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);

	struct ion_buffer *buffer;

	struct dma_buf *dmabuf;

	bool valid_handle;



	if (lock_client)

		mutex_lock(&client->lock);

	valid_handle = ion_handle_validate(client, handle);

	if (!valid_handle) {

		WARN(1, "%s: invalid handle passed to share.\n", __func__);

		if (lock_client)

			mutex_unlock(&client->lock);

		return ERR_PTR(-EINVAL);

	}

	buffer = handle->buffer;

	ion_buffer_get(buffer);

	if (lock_client)

		mutex_unlock(&client->lock);



	exp_info.ops = &dma_buf_ops;
@@ -1061,14 +1053,21 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client, 


	return dmabuf;

}



struct dma_buf *ion_share_dma_buf(struct ion_client *client,

				  struct ion_handle *handle)

{

	return __ion_share_dma_buf(client, handle, true);

}

EXPORT_SYMBOL(ion_share_dma_buf);



int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)

static int __ion_share_dma_buf_fd(struct ion_client *client,

				  struct ion_handle *handle, bool lock_client)

{

	struct dma_buf *dmabuf;

	int fd;



	dmabuf = ion_share_dma_buf(client, handle);

	dmabuf = __ion_share_dma_buf(client, handle, lock_client);

	if (IS_ERR(dmabuf))

		return PTR_ERR(dmabuf);
@@ -1078,8 +1077,19 @@ int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle) 


	return fd;

}



int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle)

{

	return __ion_share_dma_buf_fd(client, handle, true);

}

EXPORT_SYMBOL(ion_share_dma_buf_fd);



int ion_share_dma_buf_fd_nolock(struct ion_client *client,

				struct ion_handle *handle)

{

	return __ion_share_dma_buf_fd(client, handle, false);

}



struct ion_handle *ion_import_dma_buf(struct ion_client *client,

				      struct dma_buf *dmabuf)

{

drivers/staging/android/ion/ion_priv.h

+3 −3
Original line number Diff line number Diff line
@@ -463,11 +463,11 @@ void ion_free_nolock(struct ion_client *client, struct ion_handle *handle); 


int ion_handle_put_nolock(struct ion_handle *handle);



struct ion_handle *ion_handle_get_by_id(struct ion_client *client,

						int id);



int ion_handle_put(struct ion_handle *handle);



int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query);



int ion_share_dma_buf_fd_nolock(struct ion_client *client,

				struct ion_handle *handle);



#endif /* _ION_PRIV_H */