| 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
| ||
|
Date: Tue, 4 Sep 2018 11:33:46 -0700
From: Laura Abbott <labbott@...hat.com>
To: Greg Hackmann <ghackmann@...roid.com>, linux-kernel@...r.kernel.org
Cc: Sumit Semwal <sumit.semwal@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
devel@...verdev.osuosl.org, kernel-team@...roid.com,
Greg Hackmann <ghackmann@...gle.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2] staging: android: ion: fix ION_IOC_{MAP,SHARE}
use-after-free
On 09/04/2018 09:33 AM, Greg Hackmann wrote:
> The ION_IOC_{MAP,SHARE} ioctls drop and reacquire client->lock several
> times while operating on one of the client's ion_handles. This creates
> windows where userspace can call ION_IOC_FREE on the same client with
> the same handle, and effectively make the kernel drop its own reference.
> For example:
>
> - thread A: ION_IOC_ALLOC creates an ion_handle with refcount 1
> - thread A: starts ION_IOC_MAP and increments the refcount to 2
> - thread B: ION_IOC_FREE decrements the refcount to 1
> - thread B: ION_IOC_FREE decrements the refcount to 0 and frees the
> handle
> - thread A: continues ION_IOC_MAP with a dangling ion_handle * to
> freed memory
>
> Fix this by holding client->lock for the duration of
> ION_IOC_{MAP,SHARE}, preventing the concurrent ION_IOC_FREE. Also
> remove ion_handle_get_by_id(), since there's literally no way to use it
> safely.
>
> This patch is applied on top of 4.4.y, and applies to older kernels
> too. 4.9.y was fixed separately. Kernels 4.12 and later are
> unaffected, since all the underlying ion_handle infrastructure has been
> ripped out.
>
Acked-by: Laura Abbott <labbott@...hat.com>
> Cc: stable@...r.kernel.org # v4.4-
> Signed-off-by: Greg Hackmann <ghackmann@...gle.com>
> ---
> v2: remove Change-Id line from commit message
>
> drivers/staging/android/ion/ion.c | 60 +++++++++++++++++++------------
> 1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 47cb163da9a0..4adb1138af09 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -449,18 +449,6 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> return 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)
> {
> @@ -1138,24 +1126,28 @@ static struct dma_buf_ops dma_buf_ops = {
> .kunmap = ion_dma_buf_kunmap,
> };
>
> -struct dma_buf *ion_share_dma_buf(struct ion_client *client,
> - struct ion_handle *handle)
> +static struct dma_buf *__ion_share_dma_buf(struct ion_client *client,
> + struct ion_handle *handle,
> + bool lock_client)
> {
> DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> struct ion_buffer *buffer;
> struct dma_buf *dmabuf;
> bool valid_handle;
>
> - mutex_lock(&client->lock);
> + if (lock_client)
> + mutex_lock(&client->lock);
> valid_handle = ion_handle_validate(client, handle);
> if (!valid_handle) {
> WARN(1, "%s: invalid handle passed to share.\n", __func__);
> - mutex_unlock(&client->lock);
> + if (lock_client)
> + mutex_unlock(&client->lock);
> return ERR_PTR(-EINVAL);
> }
> buffer = handle->buffer;
> ion_buffer_get(buffer);
> - mutex_unlock(&client->lock);
> + if (lock_client)
> + mutex_unlock(&client->lock);
>
> exp_info.ops = &dma_buf_ops;
> exp_info.size = buffer->size;
> @@ -1170,14 +1162,21 @@ 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)
> +{
> + return __ion_share_dma_buf(client, handle, true);
> +}
> 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(struct ion_client *client,
> + struct ion_handle *handle, bool lock_client)
> {
> struct dma_buf *dmabuf;
> int fd;
>
> - dmabuf = ion_share_dma_buf(client, handle);
> + dmabuf = __ion_share_dma_buf(client, handle, lock_client);
> if (IS_ERR(dmabuf))
> return PTR_ERR(dmabuf);
>
> @@ -1187,8 +1186,19 @@ 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)
> +{
> + return __ion_share_dma_buf_fd(client, handle, true);
> +}
> EXPORT_SYMBOL(ion_share_dma_buf_fd);
>
> +static int ion_share_dma_buf_fd_nolock(struct ion_client *client,
> + struct ion_handle *handle)
> +{
> + return __ion_share_dma_buf_fd(client, handle, false);
> +}
> +
> struct ion_handle *ion_import_dma_buf(struct ion_client *client, int fd)
> {
> struct dma_buf *dmabuf;
> @@ -1335,11 +1345,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);
> - 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;
>
Powered by blists - more mailing lists