Re: [RFC PATCH] staging/android/ion : fix a race condition in the ion driver
From: Laura Abbott
Date: Wed Feb 17 2016 - 13:54:15 EST
On 02/16/2016 10:32 PM, EunTaik Lee wrote:
> There was a use-after-free problem in the ion driver.
>
> The problem is detected as an unaligned access in the
> spin lock functions since it uses load exclusive
> instruction. In some cases it corrupts the slub's
> free pointer which causes a unaligned access to the
> next free pointer.(thus the kmalloc function returns
> a pointer like ffffc0745b4580aa). And it causes lots of other
> hard-to-debug problems.
>
> This symptom is caused since the first member in the
> ion_handle structure is the reference count and the
> ion driver decrements the reference after it has been
> freed.
>
> To fix this problem client->lock mutex is extended
> to protect all the codes that uses the handle.
>
This describes the symptoms very well but what's the actual
race condition?
Also what tree did you generate this against? It doesn't
seem to apply.
Thanks,
Laura
> Signed-off-by: Eun Taik Lee <eun.taik.lee@xxxxxxxxxxx>
> ---
> drivers/staging/android/ion/ion.c | 102 ++++++++++++++++++++++++++++++--------
> 1 file changed, 82 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index e237e9f..cb03b59 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -385,13 +385,22 @@ static void ion_handle_get(struct ion_handle *handle)
> kref_get(&handle->ref);
> }
>
> +static int ion_handle_put_nolock(struct ion_handle *handle)
> +{
> + int ret;
> +
> + ret = kref_put(&handle->ref, ion_handle_destroy);
> +
> + return ret;
> +}
> +
> static int ion_handle_put(struct ion_handle *handle)
> {
> struct ion_client *client = handle->client;
> int ret;
>
> mutex_lock(&client->lock);
> - ret = kref_put(&handle->ref, ion_handle_destroy);
> + ret = ion_handle_put_nolock(handle);
> mutex_unlock(&client->lock);
>
> return ret;
> @@ -415,20 +424,30 @@ static struct ion_handle *ion_handle_lookup(struct ion_client *client,
> return ERR_PTR(-EINVAL);
> }
>
> -static struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
> - int id)
> +static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> + int id)
> {
> struct ion_handle *handle;
>
> - mutex_lock(&client->lock);
> handle = idr_find(&client->idr, id);
> if (handle)
> ion_handle_get(handle);
> - mutex_unlock(&client->lock);
>
> 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)
> {
> @@ -530,7 +549,8 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
> }
> EXPORT_SYMBOL(ion_alloc);
>
> -void ion_free(struct ion_client *client, struct ion_handle *handle)
> +static void ion_free_nolock(struct ion_client *client,
> + struct ion_handle *handle)
> {
> bool valid_handle;
>
> @@ -538,15 +558,24 @@ void ion_free(struct ion_client *client, struct ion_handle *handle)
>
> mutex_lock(&client->lock);
> valid_handle = ion_handle_validate(client, handle);
> -
> if (!valid_handle) {
> WARN(1, "%s: invalid handle passed to free.\n", __func__);
> mutex_unlock(&client->lock);
> return;
> }
> + ion_handle_put_nolock(handle);
> +}
> +
> +void ion_free(struct ion_client *client, struct ion_handle *handle)
> +{
> + BUG_ON(client != handle->client);
> +
> + mutex_lock(&client->lock);
> + ion_free_nolock(client, handle);
> mutex_unlock(&client->lock);
> ion_handle_put(handle);
> }
> +
> EXPORT_SYMBOL(ion_free);
>
> int ion_phys(struct ion_client *client, struct ion_handle *handle,
> @@ -830,6 +859,7 @@ void ion_client_destroy(struct ion_client *client)
> struct rb_node *n;
>
> pr_debug("%s: %d\n", __func__, __LINE__);
> + mutex_lock(&client->lock);
> while ((n = rb_first(&client->handles))) {
> struct ion_handle *handle = rb_entry(n, struct ion_handle,
> node);
> @@ -837,6 +867,7 @@ void ion_client_destroy(struct ion_client *client)
> }
>
> idr_destroy(&client->idr);
> + mutex_unlock(&client->lock);
>
> down_write(&dev->lock);
> if (client->task)
> @@ -1100,7 +1131,7 @@ static struct dma_buf_ops dma_buf_ops = {
> .kunmap = ion_dma_buf_kunmap,
> };
>
> -struct dma_buf *ion_share_dma_buf(struct ion_client *client,
> +static struct dma_buf *ion_share_dma_buf_nolock(struct ion_client *client,
> struct ion_handle *handle)
> {
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> @@ -1108,7 +1139,6 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client,
> struct dma_buf *dmabuf;
> bool valid_handle;
>
> - mutex_lock(&client->lock);
> valid_handle = ion_handle_validate(client, handle);
> if (!valid_handle) {
> WARN(1, "%s: invalid handle passed to share.\n", __func__);
> @@ -1117,7 +1147,6 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client,
> }
> buffer = handle->buffer;
> ion_buffer_get(buffer);
> - mutex_unlock(&client->lock);
>
> exp_info.ops = &dma_buf_ops;
> exp_info.size = buffer->size;
> @@ -1132,14 +1161,26 @@ 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)
> +{
> + struct dma_buf *dmabuf;
> +
> + mutex_lock(&client->lock);
> + dmabuf = ion_share_dma_buf_nolock(client, handle);
> + mutex_unlock(&client->lock);
> + return dmabuf;
> +}
> 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_nolock(struct ion_client *client,
> + struct ion_handle *handle)
> {
> struct dma_buf *dmabuf;
> int fd;
>
> - dmabuf = ion_share_dma_buf(client, handle);
> + dmabuf = ion_share_dma_buf_nolock(client, handle);
> if (IS_ERR(dmabuf))
> return PTR_ERR(dmabuf);
>
> @@ -1149,6 +1190,17 @@ 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)
> +{
> + int fd;
> +
> + mutex_lock(&client->lock);
> + fd = ion_share_dma_buf_fd_nolock(client, handle);
> + mutex_lock(&client->lock);
> +
> + return fd;
> +}
> EXPORT_SYMBOL(ion_share_dma_buf_fd);
>
> struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd)
> @@ -1281,11 +1333,16 @@ static 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);
> - ion_free(client, handle);
> - ion_handle_put(handle);
> + }
> + ion_free_nolock(client, handle);
> + ion_handle_put_nolock(handle);
> + mutex_unlock(&client->lock);
> break;
> }
> case ION_IOC_SHARE:
> @@ -1293,11 +1350,16 @@ static 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;
>