From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933793AbcBQGcK (ORCPT ); Wed, 17 Feb 2016 01:32:10 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:49312 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932346AbcBQGcH (ORCPT ); Wed, 17 Feb 2016 01:32:07 -0500 X-AuditID: cbfee68f-f793a6d000001364-44-56c413e4b940 Date: Wed, 17 Feb 2016 06:32:04 +0000 (GMT) From: EunTaik Lee Subject: [RFC PATCH] staging/android/ion : fix a race condition in the ion driver To: gregkh@linuxfoundation.org, arve@android.com, riandrews@android.com, labbott@redhat.com, sumit.semwal@linaro.org, gioh.kim@lge.com, dan.carpenter@oracle.com, rohit.kr@samsung.com, sriram@marirs.net.in, shawn.lin@rock-chips.com, eun.taik.lee@samsung.com, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Reply-to: eun.taik.lee@samsung.com MIME-version: 1.0 X-MTR: 20160217055909569@eun.taik.lee Msgkey: 20160217055909569@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: 20160217055909569@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: <1538647436.1019551455690718996.JavaMail.weblogic@epmlwas08c> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDJsWRmVeSWpSXmKPExsVy+t8zPd0nwkfCDJZv4rW4vGsOmwOjx+dN cgGMUQ2MNolFyRmZZakKqXnJ+SmZeem2SqEhbroWSgoZ+cUltkrRRgbGekamJnpGJuZ6lgax VkamSgp5ibmptkoVulC9SgpFyQVAtbmVxUADclL1oOJ6xal5KQ5Z+aUgl+gVJ+YWl+al6yXn 5yoplCXmlAKNUNJPmMqY8ePIM8aCLYEV32acZ2pgXOLfxcjJISSgLnFi9xoWEFtCwETi9bXd rBC2mMSFe+vZuhi5gGqWMUrMuLWaqYuRA6yosS8OIj6HUWLS4WVMIA0sAqoSfU/2gNlsAroS /z92sYPYwgKBEv//dLKDNIgITGaWeP/2CBvEZiWJ+YcbwDbzCghKnJz5BOoKVYmOK0sZIeJq ErtPNbBBxCUkZk2/AHUdr8SM9qdQ9XIS076uYYawpSXOz9rACPPB4u+PoeL8Esdu72CCsAUk pp45CFWjJbG7dQE7hM0nsWbhW6iZghKnr3Uzw+xq2PibHeaGrS1PwG5gFlCUmNL9kB3C1pL4 8mMfG7pfeAU8Ja58WMcM8ryEQC+HxLtjh6GhJSDxbfIhlgmMirOQ9MxCMncWkrnIahYwsqxi FE0tSC4oTkovMkaO702MkGTYv4Px7gHrQ4wCHIxKPLwrsg6HCbEmlhVX5h5iTAZaPZFZSjQ5 H5hy80riDY3NjCxMTUyNjcwtzTCETUwtLEyMcAgrifMulPoZLCSQnliSmp2aWpBaFF9UmpNa fIiRiYNTqoHR9+P/5M7FPArBEd9TPiz+aXvMt++1W6/rhMiOmVYPOH45Hz795uClLtODBqkd sWu/XLFYpasf0zR1EVt/yOm023tNnPRdD5jFSDTqhvxNP7xtZ7t9i/bOBb+dfZTuWu98LFjq kX7g7tn3y9j/nk3idHf5XybdzKbl1BBTmMYUbmEgw33Sd70SS3FGoqEWc1FxIgByBhxQrwMA AA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrLKsWRmVeSWpSXmKPExsVy+t/tXt0nwkfCDLZ+47C4vGsOmwOjx+dN cgGMURk2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6hpYW5koKeYm5qbZKLj4Bum6ZOUBD lRTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StFGBsZ6RqYmekbGBnomBrFWhgYGRqZAVQkZ GT+OPGMs2BJY8W3GeaYGxiX+XYycHEIC6hIndq9h6WLk4JAQMJFo7IsDCUsIiElcuLeerYuR C6hkDqPEpMPLmEASLAKqEn1P9oDZbAK6Ev8/drGD2MICgRL//3SygzSICExmlnj/9ggbxAIl ifmHG1hAbF4BQYmTM5+wQGxQlei4spQRIq4msftUAxtEXEJi1vQLrBA2r8SM9qdQ9XIS076u YYawpSXOz9rACHPp4u+PoeL8Esdu72CCsAUkpp45CFWjJbG7dQE7hM0nsWbhW6iZghKnr3Uz w+xq2PibHeaGrS1PwG5gFlCUmNL9kB3C1pL48mMfG7pfeAU8Ja58WMc8gVFmFpLULCTts5C0 I6tZwMiyilE0tSC5oDgpvcJQrzgxt7g0L10vOT93EyM46TxbuIPxy3nrQ4wCHIxKPLwrsg6H CbEmlhVX5h5ilOBgVhLhZXgOFOJNSaysSi3Kjy8qzUktPsRoCoyqicxSosn5wISYVxJvaGxg bGhoaW5gamhkoSTOG/B3XZiQQHpiSWp2ampBahFMHxMHp1QDo5Np64H4f+vuzd7LefCyh/2X y3N5vp1Q5fF7/PJiqMjzLNF3Lcq9Uls0unwOGOgvYblW+Ly0QSEtfV5qV/k5v3lXTj/ZlpS9 fPWeeeVTbn/iVNu3pfe9sVHcNd4IlqCYhs7gvInKWfbPnta/ZI2f4+k5/2ULo2oF3xXL4E3R O7x2B0edTxRRUWIpzkg01GIuKk4EADBJ0wNQAwAA 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 u1H6WESf029990 There was a use-after-free problem in the ion driver. 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 unaligned access to the next free pointer.(thus the kmalloc function 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 --- 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..cb03b59 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_lock(&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