Re: Re: [PATCH v3] staging/android/ion : fix a race condition in the ion driver
From: EunTaik Lee
Date: Wed Mar 02 2016 - 02:49:40 EST
> From: Laura Abbott [mailto:labbott@xxxxxxxxxx]
> Sent: Friday, February 26, 2016 7:12 AM
> To: eun.taik.lee@xxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; arve@xxxxxxxxxxx;
> riandrews@xxxxxxxxxxx; sumit.semwal@xxxxxxxxxx; dan.carpenter@xxxxxxxxxx;
> Rohit Kumar <rohit.kr@xxxxxxxxxxx>; sriram@xxxxxxxxxxxxx; shawn.lin@rock-
> chips.com; devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> euntaik@xxxxxxxxx
> Subject: Re: [PATCH v3] staging/android/ion : fix a race condition in the
> ion driver
>
> On 02/23/2016 08:38 PM, EunTaik Lee wrote:
> > There is a use-after-free problem in the ion driver.
> > This is caused by a race condition in the ion_ioctl() function.
> >
> > A handle has ref count of 1 and two tasks on different cpus calls
> > ION_IOC_FREE simultaneously.
> >
> > cpu 0 cpu 1
> > -------------------------------------------------------
> > ion_handle_get_by_id()
> > (ref == 2)
> > ion_handle_get_by_id()
> > (ref == 3)
> >
> > ion_free()
> > (ref == 2)
> >
> > ion_handle_put()
> > (ref == 1)
> >
> > ion_free()
> > (ref == 0 so ion_handle_destroy() is
> > called
> > and the handle is freed.)
> >
> > ion_handle_put() is called and it
> > decreases the slub's next free pointer
> >
> > 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 mis-aligned access to the next free pointer.(kmalloc 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.
> >
> > Signed-off-by: Eun Taik Lee <eun.taik.lee@xxxxxxxxxxx>
>
> Reviewed-by: Laura Abbott <labbott@xxxxxxxxxx>
>
> > ---
> > changes in v3:
> > 1. remove ion_handle_put in ion_free
> > 2. remove unnecessary protection in IOC_ION_SHARE/IOC_ION_MAP
> > changes in v2 :
> > 1. add problem description in the comment
> > 2. fix un-matching mutex_lock/unlock pair in ion_share_dma_buf()
> >
> > drivers/staging/android/ion/ion.c | 55 ++++++++++++++++++++++++++++++-
> --------
> > 1 file changed, 42 insertions(+), 13 deletions(-)
> > mode change 100644 => 100755 drivers/staging/android/ion/ion.c
> >
> > diff --git a/drivers/staging/android/ion/ion.c
> > b/drivers/staging/android/ion/ion.c
> > old mode 100644
> > new mode 100755
> > index e237e9f..1958d58
> > --- 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(struct ion_handle *handle)
> > +static int ion_handle_put_nolock(struct ion_handle *handle) {
> > + int ret;
> > +
> > + ret = kref_put(&handle->ref, ion_handle_destroy);
> > +
> > + return ret;
> > +}
> > +
> > +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,
> > +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,22 +549,28 @@ 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;
> >
> > BUG_ON(client != handle->client);
> >
> > - 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);
> >
> > @@ -1281,11 +1306,15 @@ 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:
> >
Anyone else have any comments?
Regards,
Euntaik