From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752034AbcBYWLi (ORCPT ); Thu, 25 Feb 2016 17:11:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43799 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751474AbcBYWLg (ORCPT ); Thu, 25 Feb 2016 17:11:36 -0500 Subject: Re: [PATCH v3] staging/android/ion : fix a race condition in the ion driver To: eun.taik.lee@samsung.com, "gregkh@linuxfoundation.org" , "arve@android.com" , "riandrews@android.com" , "sumit.semwal@linaro.org" , "dan.carpenter@oracle.com" , Rohit Kumar , "sriram@marirs.net.in" , "shawn.lin@rock-chips.com" , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" , "euntaik@gmail.com" References: <610730213.149021456288682499.JavaMail.weblogic@epmlwas08c> From: Laura Abbott Message-ID: <56CF7C13.6000202@redhat.com> Date: Thu, 25 Feb 2016 14:11:31 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <610730213.149021456288682499.JavaMail.weblogic@epmlwas08c> Content-Type: text/plain; charset=euc-kr Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Reviewed-by: Laura Abbott > --- > 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: >