> From: Laura Abbott [mailto:labb...@redhat.com] > Sent: Friday, February 26, 2016 7:12 AM > To: eun.taik....@samsung.com; gre...@linuxfoundation.org; a...@android.com; > riandr...@android.com; sumit.sem...@linaro.org; dan.carpen...@oracle.com; > Rohit Kumar <rohit...@samsung.com>; sri...@marirs.net.in; shawn.lin@rock- > chips.com; de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org; > eunt...@gmail.com > 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....@samsung.com> > > Reviewed-by: Laura Abbott <labb...@redhat.com> > > > --- > > 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
