From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1428041AbcBSMDz (ORCPT ); Fri, 19 Feb 2016 07:03:55 -0500 Received: from mailout3.samsung.com ([203.254.224.33]:53030 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1427031AbcBSMDf (ORCPT ); Fri, 19 Feb 2016 07:03:35 -0500 X-AuditID: cbfee68f-f793a6d000001364-c2-56c7049547b2 Date: Fri, 19 Feb 2016 12:03:33 +0000 (GMT) From: EunTaik Lee Subject: [PATCH v2] 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 Lee , euntaik@gmail.com Reply-to: eun.taik.lee@samsung.com MIME-version: 1.0 X-MTR: 20160219115856894@eun.taik.lee Msgkey: 20160219115856894@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: 20160218080500317@eun.taik.lee X-ParentMTR: 20160219062743225@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: <754762754.1155471455883408801.JavaMail.weblogic@epmlwas08c> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprNJsWRmVeSWpSXmKPExsVy+t8zXd2pLMfDDM5ukrW4vGsOmwOjx+dN cgGMUQ2MNolFyRmZZakKqXnJ+SmZeem2SqEhbroWSgoZ+cUltkrRRgbGekamJnpGJuZ6lgax VkamSgp5ibmptkoVulC9SgpFyQVAtbmVxUADclL1oOJ6xal5KQ5Z+aUgl+gVJ+YWl+al6yXn 5yoplCXmlAKNUNJPmMqYcfneXbaCnuiKGdMbWBsY70R0MXJyCAmoS5zYvYYFxJYQMJFo7bwA ZYtJXLi3nq2LkQuoZhmjxOF3z1lhil4/bmCGaJ7DKHHhkTCIzSKgKrF+4Vt2EJtNQFfi/8cu MFtYIEDi1oVn7CCDRATaWCXeXt0H1awkMf9wA9g2XgFBiZMznwDZHEALVCXaLnpChNUkLs54 zgSxV0Ji1vQLUDfwSsxofwp1qJzEtK9rmCFsaYnzszYwwjyw+PtjqDi/xLHbO6DmCEhMPXMQ qkZL4uvfI1AzdSSW3jwGFReUOH2tmxlmV8PG3+wwN2xteQJWzyygKDGl+yE7hK0l8eXHPjZU r4DYHhJHvi8E+11CoJVD4uuZ2cyQwBKQ+Db5EMsERsVZSHpmIZk7C8lcZDULGFlWMYqmFiQX FCelFxkjR/cmRkgq7N/BePeA9SFGAQ5GJR7eCr1jYUKsiWXFlbmHGJOBVk9klhJNzgcm3LyS eENjMyMLUxNTYyNzSzMMYRNTCwsTIxzCSuK8C6V+BgsJpCeWpGanphakFsUXleakFh9iZOLg lGpgnDm3JIlhPufMoy07lzReMD3t8lKH2aF9RT279OnrBVUznk7ZUFK7fW91aeKRaRlRF1IU PmR970t+qWgcJL5NU7llRrdMhGSj1QJDTc1HRflb9l9/FlQ982bcpxRFS+s4lus3tF2XLLfo nl3x6gTjD4ktyxIncKxTTWd/9vjtFTcukVTxA42ZSizFGYmGWsxFxYkATxzkba4DAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrDKsWRmVeSWpSXmKPExsVy+t/tXt2pLMfDDBauk7S4vGsOmwOjx+dN cgGMURk2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6hpYW5koKeYm5qbZKLj4Bum6ZOUBD lRTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StFGBsZ6RqYmekbGBnomBrFWhgYGRqZAVQkZ GZfv3WUr6ImumDG9gbWB8U5EFyMnh5CAusSJ3WtYQGwJAROJ148bmCFsMYkL99azQdTMYZS4 8EgYxGYRUJVYv/AtO4jNJqAr8f9jF5gtLBAgcevCMyCbi0NEoI1V4u3VfcwQzUoS8w83gC3g FRCUODnzCZDNAbRAVaLtoidEWE3i4oznTBB7JSRmTb/ACmHzSsxofwp1m5zEtK9roG6Tljg/ awMjzJ2Lvz+GivNLHLu9A2qOgMTUMweharQkvv49AjVTR2LpzWNQcUGJ09e6mWF2NWz8zQ5z w9aWJ2D1zAKKElO6H7JD2FoSX37sY0P1CojtIXHk+0L2CYwys5CkZiFpn4WkHVnNAkaWVYyi qQXJBcVJ6RUmesWJucWleel6yfm5mxjBKefZkh2MDResDzEKcDAq8fBeMDgWJsSaWFZcmXuI UYKDWUmE99JXoBBvSmJlVWpRfnxRaU5q8SFGU2BMTWSWEk3OB6bDvJJ4Q2MDY0NDS3MDU0Mj CyVx3oC/68KEBNITS1KzU1MLUotg+pg4OKUaGHlSPuww7Ht2ZsKZPyucv6497CUrc0ftfn2b ov+frSs2sPCHS+8quPiOXUqsKbeicVFJulR19tHq2P/TrnftfrSUVaFcqGWuw06jGXs+G19U cDmjL/1XI99QKLSS5Uflf97e98YPQpu+z6k5tO3i+5tsu+Zy1sfqr/JfJbV8/n/hvvvCjha/ 7ymxFGckGmoxFxUnAgB49TPWTwMAAA== 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 u1JC40SO021240 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 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 | 102 ++++++++++++++++++++++++++++++-------- 1 file changed, 82 insertions(+), 20 deletions(-) diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index e237e9f..c6fbe48 100644 --- 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_nolock(struct ion_handle *handle) +{ + int ret; + + ret = kref_put(&handle->ref, ion_handle_destroy); + + return ret; +} + static 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, - int id) +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,7 +549,8 @@ 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; @@ -538,15 +558,24 @@ void ion_free(struct ion_client *client, struct ion_handle *handle) 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); int ion_phys(struct ion_client *client, struct ion_handle *handle, @@ -830,6 +859,7 @@ void ion_client_destroy(struct ion_client *client) struct rb_node *n; pr_debug("%s: %d\n", __func__, __LINE__); + mutex_lock(&client->lock); while ((n = rb_first(&client->handles))) { struct ion_handle *handle = rb_entry(n, struct ion_handle, node); @@ -837,6 +867,7 @@ void ion_client_destroy(struct ion_client *client) } idr_destroy(&client->idr); + mutex_unlock(&client->lock); down_write(&dev->lock); if (client->task) @@ -1100,7 +1131,7 @@ static struct dma_buf_ops dma_buf_ops = { .kunmap = ion_dma_buf_kunmap, }; -struct dma_buf *ion_share_dma_buf(struct ion_client *client, +static struct dma_buf *ion_share_dma_buf_nolock(struct ion_client *client, struct ion_handle *handle) { DEFINE_DMA_BUF_EXPORT_INFO(exp_info); @@ -1108,7 +1139,6 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client, struct dma_buf *dmabuf; bool valid_handle; - mutex_lock(&client->lock); valid_handle = ion_handle_validate(client, handle); if (!valid_handle) { WARN(1, "%s: invalid handle passed to share.\n", __func__); @@ -1117,7 +1147,6 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client, } buffer = handle->buffer; ion_buffer_get(buffer); - mutex_unlock(&client->lock); exp_info.ops = &dma_buf_ops; exp_info.size = buffer->size; @@ -1132,14 +1161,26 @@ struct dma_buf *ion_share_dma_buf(struct ion_client *client, return dmabuf; } + +struct dma_buf *ion_share_dma_buf(struct ion_client *client, + struct ion_handle *handle) +{ + struct dma_buf *dmabuf; + + mutex_lock(&client->lock); + dmabuf = ion_share_dma_buf_nolock(client, handle); + mutex_unlock(&client->lock); + return dmabuf; +} EXPORT_SYMBOL(ion_share_dma_buf); -int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle) +static int ion_share_dma_buf_fd_nolock(struct ion_client *client, + struct ion_handle *handle) { struct dma_buf *dmabuf; int fd; - dmabuf = ion_share_dma_buf(client, handle); + dmabuf = ion_share_dma_buf_nolock(client, handle); if (IS_ERR(dmabuf)) return PTR_ERR(dmabuf); @@ -1149,6 +1190,17 @@ int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle) return fd; } + +int ion_share_dma_buf_fd(struct ion_client *client, struct ion_handle *handle) +{ + int fd; + + mutex_lock(&client->lock); + fd = ion_share_dma_buf_fd_nolock(client, handle); + mutex_unlock(&client->lock); + + return fd; +} EXPORT_SYMBOL(ion_share_dma_buf_fd); struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd) @@ -1281,11 +1333,16 @@ 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: @@ -1293,11 +1350,16 @@ 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); - data.fd.fd = ion_share_dma_buf_fd(client, handle); - ion_handle_put(handle); + } + data.fd.fd = ion_share_dma_buf_fd_nolock(client, handle); + ion_handle_put_nolock(handle); + mutex_unlock(&client->lock); if (data.fd.fd < 0) ret = data.fd.fd; break; -- 1.9.1