From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1427307AbcBSIbe (ORCPT ); Fri, 19 Feb 2016 03:31:34 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:50177 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1426737AbcBSIbc (ORCPT ); Fri, 19 Feb 2016 03:31:32 -0500 X-AuditID: cbfee68f-f793a6d000001364-dc-56c6d2e25b6e Date: Fri, 19 Feb 2016 08:31:30 +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" , "gioh.kim@lge.com" , "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 Reply-to: eun.taik.lee@samsung.com MIME-version: 1.0 X-MTR: 20160219062743225@eun.taik.lee Msgkey: 20160219062743225@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: 20160218080500317@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: <2066832629.1145341455870685689.JavaMail.weblogic@epmlwas08c> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDJsWRmVeSWpSXmKPExsVy+t8zXd1Hl46FGZz4YmZxedccNgdGj8+b 5AIYoxoYbRKLkjMyy1IVUvOS81My89JtlUJD3HQtlBQy8otLbJWijQyM9YxMTfSMTMz1LA1i rYxMlRTyEnNTbZUqdKF6lRSKkguAanMri4EG5KTqQcX1ilPzUhyy8ktBLtErTswtLs1L10vO z1VSKEvMKQUaoaSfMJUx49PEl2wFdyIrPrW9YG1g3BDexcjJISSgLnFi9xoWEFtCwERi2Y9f zBC2mMSFe+vZIGqWMUoc7zSGqdl2dzJTFyMXUHwOo8SRGR8YQRIsAqoSk69uA2tgE9CV+P+x ix3EFhYIkLh14Rk7SIOIwFRWidtzn7BATFWSmH+4AczmFRCUODnzCdQVqhJ35t2DiqtJ9Dw6 A3WRhMSs6RdYIWxeiRntT6Hq5SSmfV0DVSMtcX7WBkaYDxZ/fwwV55c4dnsHE4QtIDH1zEGo Gi2Jr3+PQM3UkZjYtIENwhaUOH2tmxlmV8PG3+wwN2xteQJWzyygKDGl+yE7hK0l8eXHPjZ0 v/AKeEq0bL3NCPK8hEArh8T5g0uYIaElIPFt8iGWCYyKs5D0zEIydxaSuchqFjCyrGIUTS1I LihOSi8yRo7vTYyQZNi/g/HuAetDjAIcjEo8vBV6x8KEWBPLiitzDzEmA62eyCwlmpwPTLl5 JfGGxmZGFqYmpsZG5pZmGMImphYWJkY4hJXEeRdK/QwWEkhPLEnNTk0tSC2KLyrNSS0+xMjE wSnVwMguuv7zw3ORi08JT58V99b5zXNlSTnOjebLZsVa3Jy45jqHwjWx00ZNlq/uvpia+OZS u8v58v06740XNTyv9JjdzG/1dCpH6Y7LGt2anlWbTmk8Pbnzo57drv0K81fnLf1zkfNLb/z5 GSs2JD3Y4nbp+V+GOTne+ya0L5JbI6HHPv+cTeaXuoj/SizFGYmGWsxFxYkA225lqK8DAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrHKsWRmVeSWpSXmKPExsVy+t/tXt1Hl46FGTTMMbK4vGsOmwOjx+dN cgGMURk2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6hpYW5koKeYm5qbZKLj4Bum6ZOUBD lRTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StFGBsZ6RqYmekbGBnomBrFWhgYGRqZAVQkZ GZ8mvmQruBNZ8antBWsD44bwLkZODiEBdYkTu9ewgNgSAiYS2+5OZoKwxSQu3FvP1sXIBVQz h1HiyIwPjCAJFgFViclXt7GB2GwCuhL/P3axg9jCAgESty48YwdpEBGYyipxe+4TFogNShLz DzeA2bwCghInZz6B2qYqcWfePai4mkTPozPMEHEJiVnTL7BC2LwSM9qfQtXLSUz7ugaqRlri /KwNjDCXLv7+GCrOL3Hs9g6oDwQkpp45CFWjJfH17xGomToSE5s2sEHYghKnr3Uzw+xq2Pib HeaGrS1PwOqZBRQlpnQ/ZIewtSS+/NjHhu4XXgFPiZattxknMMrMQpKahaR9FpJ2ZDULGFlW MYqmFiQXFCelVxjrFSfmFpfmpesl5+duYgSnnWeLdzD+P299iFGAg1GJh/eCwbEwIdbEsuLK 3EOMEhzMSiK8uv5AId6UxMqq1KL8+KLSnNTiQ4ymwKiayCwlmpwPTIl5JfGGxgbGhoaW5gam hkYWSuK8AX/XhQkJpCeWpGanphakFsH0MXFwSjUwVgZqhLZpBGyI2rEyz2mG17bK0iX1lz+F MrP9KJt4VnPnJRFb7uOPWD9esr84oajl9qnVOZx8/7xyZ7/xm5vqwvLgdnVu6s38ivrvG+vF Qg7I5peWr67oW83k3us7Mf7/vzuX+SOCNX2fJl9b5Ml+b9HhTNXlP2KKlskU2nydKO0sLaLd dSVNiaU4I9FQi7moOBEAUr/N8VEDAAA= 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 u1J8VdQD020013 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 --- 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