From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757681AbcBXEia (ORCPT ); Tue, 23 Feb 2016 23:38:30 -0500 Received: from mailout4.samsung.com ([203.254.224.34]:53659 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756563AbcBXEiJ (ORCPT ); Tue, 23 Feb 2016 23:38:09 -0500 X-AuditID: cbfee68e-f793c6d00000136c-66-56cd33af6c05 Date: Wed, 24 Feb 2016 04:38:07 +0000 (GMT) From: EunTaik Lee Subject: [PATCH v3] staging/android/ion : fix a race condition in the ion driver To: Laura Abbott , "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" , EunTaik Lee Reply-to: eun.taik.lee@samsung.com MIME-version: 1.0 X-MTR: 20160224043114377@eun.taik.lee Msgkey: 20160224043114377@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: 20160223111717837@eun.taik.lee X-ParentMTR: 20160223111717837@eun.taik.lee X-ArchiveUser: EV X-CPGSPASS: Y X-ConfirmMail: N,general Content-type: text/plain; charset=euc-kr MIME-version: 1.0 Message-id: <610730213.149021456288682499.JavaMail.weblogic@epmlwas08c> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprFJsWRmVeSWpSXmKPExsVy+t8zPd31xmfDDKbt57K4vGsOmwOjx+dN cgGMUQ2MNolFyRmZZakKqXnJ+SmZeem2SqEhbroWSgoZ+cUltkrRRgbGekamJnpGJuZ6lgax VkamSgp5ibmptkoVulC9SgpFyQVAtbmVxUADclL1oOJ6xal5KQ5Z+aUgl+gVJ+YWl+al6yXn 5yoplCXmlAKNUNJPmMqYsbtrG2tBi0XFm0k2DYwnzLoYOTmEBNQlTuxewwJiSwiYSPy5sJcV whaTuHBvPVsXIxdQzTJGiRl3HrLBFN2e3ccIkZjDKHH372SgBAcHi4CqxIa94iA1bAK6Ev8/ drGD2MICARI7G6eB1YsITGeVuHx3JiPEZiWJ+YcbwDbzCghKnJz5BOoKVYkbxzYwQsTVJL7N uQ8Vl5CYNf0C1HW8EjPan0LF5SSmfV3DDGFLS5yfBdEL8sHi74+h4vwSx27vYIKwBSSmnjkI VaMlcf7jRKjHdCSmr/kFVSMocfpaNzPMroaNv9lhbtja8gTsBmYBRYkp3Q/ZIWwtiS8/9rGh +4VXwF3izbu5UHM6OSTm9WmC2CxAN3ybfIhlAqPiLCQts5CMnYVkLLKaBYwsqxhFUwuSC4qT 0ouMkGN7EyMkEfbtYLx5wPoQowAHoxIP74MNZ8KEWBPLiitzDzEmA62eyCwlmpwPTLd5JfGG xmZGFqYmpsZG5pZmGMImphYWJkY4hJXEeROkfgYLCaQnlqRmp6YWpBbFF5XmpBYfYmTi4JRq YJx7nZ+x6oUb67G5p3270uI5jBIOXfjx5sCyi5nBDzatdrFWPSXELebfd7GX/fHq8OQLlU9C vZ5sMlIMFhPrqj871fGSmOv9kM/dXItssox8FHtZeLYdU9pwesa/O90L44NeNwXcCp+/QXAj R9hrPnfLdp/+FZ9yUs7zWgldKDpTvu1rbd/vPUosxRmJhlrMRcWJAFKDCgitAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrDKsWRmVeSWpSXmKPExsVy+t/tXt31xmfDDCbdYbO4vGsOmwOjx+dN cgGMURk2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6hpYW5koKeYm5qbZKLj4Bum6ZOUBD lRTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StFGBsZ6RqYmekbGBnomBrFWhgYGRqZAVQkZ Gbu7trEWtFhUvJlk08B4wqyLkZNDSEBd4sTuNSwgtoSAicTt2X2MELaYxIV769m6GLmAauYw Stz9OxnI4eBgEVCV2LBXHKSGTUBX4v/HLnYQW1ggQGJn4zRGkHoRgemsEpfvzmSEWKAkMf9w A9gCXgFBiZMzn0AtU5W4cWwDI0RcTeLbnPtQcQmJWdMvsELYvBIz2p9CxeUkpn1dwwxhS0uc n7UB7tDF3x9Dxfkljt3ewQRhC0hMPXMQqkZL4vzHiWwQto7E9DW/oGoEJU5f62aG2dWw8Tc7 zA1bW56A3cAsoCgxpfshO4StJfHlxz42dL/wCrhLvHk3l3kCo8wsJKlZSNpnIWlHVrOAkWUV o2hqQXJBcVJ6hbFecWJucWleul5yfu4mRnDKebZ4B+P/89aHGAU4GJV4eC02nwkTYk0sK67M PcQowcGsJMKbyX02TIg3JbGyKrUoP76oNCe1+BCjKTCmJjJLiSbnA9NhXkm8obGBsaGhpbmB qaGRhZI4b8DfdWFCAumJJanZqakFqUUwfUwcnFINjKrr+aNmTuB9+b2v5L6435Lf+xu1Jfcd fn1FrWT2Mv/zXPnKAUk3Zha4Gn74MGWFyfnGLQqFm87F/Tau+OLcfjXXsvDMwqkeTI9ative zu34s20DX2okj4je693i9rkPPwZMfJxyd18+97aLKXty2y7UO+9RmLrL4rTi+aRDikviMqf+ ObZiN7MSS3FGoqEWc1FxIgAMqth1TwMAAA== 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 u1O4cZNS018298 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 --- 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