From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752869AbcCBHth (ORCPT ); Wed, 2 Mar 2016 02:49:37 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:55409 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752556AbcCBHtf (ORCPT ); Wed, 2 Mar 2016 02:49:35 -0500 X-AuditID: cbfee68f-f793a6d000001364-df-56d69b0807c2 Date: Wed, 02 Mar 2016 07:49:28 +0000 (GMT) From: EunTaik Lee Subject: Re: Re: [PATCH v3] staging/android/ion : fix a race condition in the ion driver To: Laura Abbott , =?euc-kr?Q?=C0=CC=C0=BA=C5=C3?= , "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" Reply-to: eun.taik.lee@samsung.com MIME-version: 1.0 X-MTR: 20160302074702092@eun.taik.lee Msgkey: 20160302074702092@eun.taik.lee X-EPLocale: ko_KR.euc-kr X-Priority: 3 X-EPWebmail-Msg-Type: personal X-EPWebmail-Reply-Demand: 0 X-EPApproval-Locale: X-EPHeader: ML X-MLAttribute: X-RootMTR: 20160302074702092@eun.taik.lee X-ParentMTR: X-ArchiveUser: EV X-CPGSPASS: Y X-ConfirmMail: N,general Content-type: text/plain; charset=euc-kr MIME-version: 1.0 Message-id: <956383437.73531456904963445.JavaMail.weblogic@ep2mlwas09d> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprLJsWRmVeSWpSXmKPExsVy+t8zLV2O2dfCDL5+ZLe4vGsOmwOjx+dN cgGMUQ2MNolFyRmZZakKqXnJ+SmZeem2SqEhbroWSgoZ+cUltkrRRgbGekamJnpGJuZ6lgax VkamSgp5ibmptkoVulC9SgpFyQVAtbmVxUADclL1oOJ6xal5KQ5Z+aUgl+gVJ+YWl+al6yXn 5yoplCXmlAKNUNJPmMqY8WH1ROaCX24VbfsOMTUw9rh2MXJyCAmoS5zYvYYFxJYQMJHYtGYp G4QtJnHh3nogmwuoZhmjxNuZB5hhihqnLGaBSMxhlLizaClYN4uAisT+n81MIDabgK7E/49d 7CC2sECExJtH/ewgDSICq1kl+t9+ZIFYrSQx/3ADmM0rIChxcuYTqDNUJdYdnscEEVeT2DV7 KitEXEJi1vQLUDavxIz2p1D1chLTvq6Buk5a4vysDYwwLyz+/hgqzi9x7PYOJghbQGLqmYNQ NVoSHds/Qtl8EmsWvoWaKShx+lo3M8yuho2/2WFu2NryBOwGZgFFiSndD9khbC2JLz/2saH7 hVfAXWLn5jtMIM9LCLRySBw7uJMdEloCEt8mH2KZwKg4C0nPLCRzZyGZi6xmASPLKkbR1ILk guKk9CJj5AjfxAhJh/07GO8esD7EKMDBqMTDm/HpapgQa2JZcWXuIcZkoNUTmaVEk/OBSTev JN7Q2MzIwtTE1NjI3NIMQ9jE1MLCxAiHsJI470Kpn8FCAumJJanZqakFqUXxRaU5qcWHGJk4 OKUaGD33MUYcmZR0yHBf5Pu1f3pr18xYdPT/zfKvm5//KGVUffhnz96ZlzJez2sI752QXhP0 46PC8qXfTXyK24X17zTfirzByfBzlejpVRZHl/GsY09l+hq68OPZZexbz/hcmqb09MArbZvj 89IS489zqUmqMKjNPbfs3BfRprcmW16kv320Wpdl2fYjSizFGYmGWsxFxYkALY7P67ADAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrHKsWRmVeSWpSXmKPExsVy+t/tXl2O2dfCDCa+ZLa4vGsOmwOjx+dN cgGMURk2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6hpYW5koKeYm5qbZKLj4Bum6ZOUBD lRTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StFGBsZ6RqYmekbGBnomBrFWhgYGRqZAVQkZ GR9WT2Qu+OVW0bbvEFMDY49rFyMnh5CAusSJ3WtYQGwJAROJximLoWwxiQv31rN1MXIB1cxh lLizaClYgkVARWL/z2YmEJtNQFfi/8cudhBbWCBC4s2jfnaQBhGB1awS/W8/skBsUJKYf7gB zOYVEJQ4OfMJ1AZViXWH5zFBxNUkds2eygoRl5CYNf0ClM0rMaP9KVS9nMS0r2uYIWxpifOz NjDCXLr4+2OoOL/Esds7mCBsAYmpZw5C1WhJdGz/CGXzSaxZ+BZqpqDE6WvdzDC7Gjb+Zoe5 YWvLE7AbmAUUJaZ0P2SHsLUkvvzYx4buF14Bd4mdm+8wTWCUmYUkNQtJ+ywk7chqFjCyrGIU TS1ILihOSq8w0itOzC0uzUvXS87P3cQITjvPFu1g/Hfe+hCjAAejEg9vxqerYUKsiWXFlbmH GCU4mJVEeI17r4UJ8aYkVlalFuXHF5XmpBYfYjQFRtVEZinR5HxgSswriTc0NjA2NLQ0NzA1 NLJQEucN+LsuTEggPbEkNTs1tSC1CKaPiYNTqoGx/Mv9rB62Rbz/J6w4aFy87IlVg8Pp47GS Wta31u2y+707Tfp0RK0Kp+7u9sUvHt5IuP/j3Nf3IYuvdl2oVHblNzhS0LB+y5b478v162Pj Y2u3neO37/ggEcOeHDQzYu2DDY/WXDPc/+VtKLu31+Idb2/NmPY/1umVksmvrSdn9dSf/fZN /5xMlRJLcUaioRZzUXEiAACy61VRAwAA DLP-Filter: Pass X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u227nhTe011386 > From: Laura Abbott [mailto:labbott@redhat.com] > Sent: Friday, February 26, 2016 7:12 AM > 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 > 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 > > 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: > > Anyone else have any comments? Regards, Euntaik