| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <56CF7C13.6000202@redhat.com>
Date: Thu, 25 Feb 2016 14:11:31 -0800
From: Laura Abbott <labbott@...hat.com>
To: eun.taik.lee@...sung.com,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"arve@...roid.com" <arve@...roid.com>,
"riandrews@...roid.com" <riandrews@...roid.com>,
"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
"dan.carpenter@...cle.com" <dan.carpenter@...cle.com>,
Rohit Kumar <rohit.kr@...sung.com>,
"sriram@...irs.net.in" <sriram@...irs.net.in>,
"shawn.lin@...k-chips.com" <shawn.lin@...k-chips.com>,
"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"euntaik@...il.com" <euntaik@...il.com>
Subject: Re: [PATCH v3] staging/android/ion : fix a race condition in the ion
driver
On 02/23/2016 08:38 PM, EunTaik Lee wrote:
> 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 <eun.taik.lee@...sung.com>
Reviewed-by: Laura Abbott <labbott@...hat.com>
> ---
> 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:
>
Powered by blists - more mailing lists