From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964849AbcCJCHI (ORCPT ); Wed, 9 Mar 2016 21:07:08 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:54819 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752486AbcCJCG5 (ORCPT ); Wed, 9 Mar 2016 21:06:57 -0500 X-AuditID: cbfee68d-f79646d000001355-b3-56e0d6be8732 Date: Thu, 10 Mar 2016 02:06:54 +0000 (GMT) From: EunTaik Lee Subject: [RESEND PATCH v3] staging/android/ion : fix a race condition in the ion driver To: "GiohKimgioh.kim@lge.com" <"GiohKim, "SumitSemwalsumit.semwal@linaro.org" <"SumitSemwal, "DanCarpenterdan.carpenter@oracle.com" <"DanCarpenter, "DmitryKalinkindmitry.kalinkin@gmail.com" <"DmitryKalinkin, "ShawnLinshawn.lin@rock-chips.com" <"ShawnLin, "ShailendraVermashailendra.capricorn@gmail.com" <"ShailendraVerma, "Rohitkumarrohit.kr@samsung.com" <"Rohitkumar, "PaulGortmakerpaul.gortmaker@windriver.com" <"PaulGortmaker, "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" Reply-to: eun.taik.lee@samsung.com MIME-version: 1.0 X-MTR: 20160310015155019@eun.taik.lee Msgkey: 20160310015155019@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: 20160310015155019@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: <1707803446.78031457575607542.JavaMail.weblogic@ep2mlwas05b> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprFJsWRmVeSWpSXmKPExsVy+t8zTd191x6EGZzuNLC4vGsOmwOjx+dN cgGMUQ2MNolFyRmZZakKqXnJ+SmZeem2SqEhbroWSgoZ+cUltkrRRgbGekamJnpGJuZ6lgax VkamSgp5ibmptkoVulC9SgpFyQVAtbmVxUADclL1oOJ6xal5KQ5Z+aUgl+gVJ+YWl+al6yXn 5yoplCXmlAKNUNJPmMqY0X/7GXvBFbOK7cv1GxjnmHYxcnIICahLnNi9hgXElhAwkTg48Soz hC0mceHeerYuRi6gmmWMEu3dM9hgipo77zNBJOYwSmxr3AeWYBFQlfjw4QFYN5uArsT/j13s ILawQLjE4R2HmUBsEYG3nBKP7ohAbFaSmH+4AWwzr4CgxMmZT6CuUJW4s6GbGSKuJrF4yikm iLiExKzpF1ghbF6JGe1PoerlJKZ9XQN1tbTE+VkbGGE+WPz9MVScX+LY7R1QcwQkpp45CFTD AWRrSSy8BVXCJ7Fm4VuokYISp691M8Osatj4mx3mhK0tT8BOYBZQlJjS/ZAdwtaS+PIDEgzI XuEV8JBYvvwAOyisJARaOSSWf3vEDgkrAYlvkw+xTGBUnIWkZxaSubOQzEVWs4CRZRWjaGpB ckFxUnqRIXJsb2KEJMLeHYy3D1gfYhTgYFTi4RWoeRAmxJpYVlyZe4gxGWj1RGYp0eR8YLrN K4k3NDYzsjA1MTU2Mrc0wxA2MbWwMDHCIawkzqso9TNYSCA9sSQ1OzW1ILUovqg0J7X4ECMT B6dUA6NRxjpWva4ejwzPH5fm2it1Hdzbn7pG/EGxYGdmQfPJjK3FWyv45OaeWJf1V+sE7z/1 3gypT22/o0Mqv25SDLOu48m9I/GZWe71n1ULUqe+frXn7csr1a3Pr8xj46lRK+RarOS01oNB N/Pk5rvPygs7v2xSqo+pSb4se71wwgrDbc2+xpXbNJRYijMSDbWYi4oTASb2L1WtAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrNKsWRmVeSWpSXmKPExsVy+t/tft191x6EGZy/rGVxedccNgdGj8+b 5AIYozJsMlITU1KLFFLzkvNTMvPSbZW8g+Od403NDAx1DS0tzJUU8hJzU22VXHwCdN0yc4CG KimUJeaUAoUCEouLlfTtbIryS0tSFTLyi0tslaKNDIz1jExN9IyMDfRMDGKtDA0MjEyBqhIy MvpvP2MvuGJWsX25fgPjHNMuRk4OIQF1iRO717CA2BICJhLNnfeZIGwxiQv31rN1MXIB1cxh lNjWuI8NJMEioCrx4cMDZhCbTUBX4v/HLnYQW1ggXOLwjsNgzSICbzklHt0RgVigJDH/cAPY Al4BQYmTM59ALVOVuLOhmxkiriaxeMopqMUSErOmX2CFsHklZrQ/haqXk5j2dQ0zhC0tcX7W BkaYQxd/fwwV55c4dnsH1BwBialnDgLVcADZWhILb0GV8EmsWfgWaqSgxOlr3cwwqxo2/maH OWFryxOwE5gFFCWmdD9kh7C1JL78gAQDsld4BTwkli8/wD6BUWYWktQsJO2zkLQjq1nAyLKK UTS1ILmgOCm9wlivODG3uDQvXS85P3cTIzjhPFu8g/H/eetDjAIcjEo8vAI1D8KEWBPLiitz DzFKcDArifD6nQEK8aYkVlalFuXHF5XmpBYfYjQFRtREZinR5HxgMswriTc0NjA2NLQ0NzA1 NLJQEucN+LsuTEggPbEkNTs1tSC1CKaPiYNTqoFR87bgHwcWw6hEhdqkVcfdvt68flqb+d2v dNXLN4oCzzM8/myZHyb3+n2ARZLJyvWbA8t+7q5aVWy6raKKQX6j8sHNPnwrdymJSr11zt/5 V/mriFxI+PdLhvt2Hzxd9KNx4/442Zfc5h2GT4r67r3+pnT2jmCndFaKz4QApcXHPNmyjcUW WBYosRRnJBpqMRcVJwIAuzAi0U4DAAA= 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 u2A27Bdr005559 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: -- 1.9.1