Messages in this thread | | | Subject | Re: [PATCH v2] staging: android: ion: fix ION_IOC_{MAP,SHARE} use-after-free | From | Laura Abbott <> | Date | Tue, 4 Sep 2018 11:33:46 -0700 |
| |
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@redhat.com>
> Cc: stable@vger.kernel.org # v4.4- > Signed-off-by: Greg Hackmann <ghackmann@google.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; >
| |