Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jens Wiklander <jens.wiklander@linaro.org>
To:     linux-kernel@vger.kernel.org,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     Al Viro <viro@zeniv.linux.org.uk>,
        Gustavo Padovan <gustavo@padovan.org>,
        Zhenyu Wang <zhenyuw@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Christian Koenig <christian.koenig@amd.com>,
        Laura Abbott <labbott@redhat.com>,
        Jens Wiklander <jens.wiklander@linaro.org>
Subject: [PATCH] dma-buf: simplified calling conventions for dma_buf_fd()
Date:   Tue, 22 May 2018 10:09:25 +0200
Message-Id: <20180522080925.18158-1-jens.wiklander@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

From: Al Viro <viro@zeniv.linux.org.uk>

Make dma_buf_fd() drop the file reference on failure, folding
dma_buf_put() into it.

Users of dma_buf_fd() (drm, videobuf2, ion and tee) are updated to take
the new calling convention into account.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
[jw: rebased and updated commit message]
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---

I received this patch as a response to what has become
bb765d1c331f ("tee: shm: fix use-after-free via temporarily dropped reference")

 drivers/dma-buf/dma-buf.c                     | 16 +++++----
 drivers/gpu/drm/drm_prime.c                   | 15 +++-----
 drivers/gpu/drm/i915/gvt/dmabuf.c             |  4 +--
 drivers/gpu/drm/ttm/ttm_object.c              |  3 +-
 .../media/common/videobuf2/videobuf2-core.c   | 14 ++++----
 drivers/staging/android/ion/ion.c             |  7 +---
 drivers/tee/tee_core.c                        | 35 ++++++-------------
 drivers/tee/tee_shm.c                         |  8 +----
 8 files changed, 36 insertions(+), 66 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d78d5fc173dc..78981928758e 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -471,17 +471,21 @@ EXPORT_SYMBOL_GPL(dma_buf_export);
  */
 int dma_buf_fd(struct dma_buf *dmabuf, int flags)
 {
+	struct file *file;
 	int fd;
 
-	if (!dmabuf || !dmabuf->file)
+	if (!dmabuf)
 		return -EINVAL;
 
-	fd = get_unused_fd_flags(flags);
-	if (fd < 0)
-		return fd;
-
-	fd_install(fd, dmabuf->file);
+	file = dmabuf->file;
+	if (!file)
+		return -EINVAL;
 
+	fd = get_unused_fd_flags(flags);
+	if (fd >= 0)
+		fd_install(fd, file);
+	else
+		fput(file);
 	return fd;
 }
 EXPORT_SYMBOL_GPL(dma_buf_fd);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7856a9b3f8a8..06d1b7d3751a 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -662,8 +662,10 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	ret = drm_prime_add_buf_handle(&file_priv->prime,
 				       dmabuf, handle);
 	mutex_unlock(&dev->object_name_lock);
-	if (ret)
-		goto fail_put_dmabuf;
+	if (unlikely(ret)) {
+		dma_buf_put(dmabuf);
+		goto out;
+	}
 
 out_have_handle:
 	ret = dma_buf_fd(dmabuf, flags);
@@ -673,17 +675,10 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	 * and that is invariant as long as a userspace gem handle exists.
 	 * Closing the handle will clean out the cache anyway, so we don't leak.
 	 */
-	if (ret < 0) {
-		goto fail_put_dmabuf;
-	} else {
+	if (ret >= 0) {
 		*prime_fd = ret;
 		ret = 0;
 	}
-
-	goto out;
-
-fail_put_dmabuf:
-	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_put_unlocked(obj);
 out_unlock:
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
index 6f4f8e941fc2..1783c724d96c 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -479,7 +479,7 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu, unsigned int dmabuf_id)
 	ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR);
 	if (ret < 0) {
 		gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret);
-		goto out_free_dmabuf;
+		goto out_free_gem;
 	}
 	dmabuf_fd = ret;
 
@@ -502,8 +502,6 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu, unsigned int dmabuf_id)
 
 	return dmabuf_fd;
 
-out_free_dmabuf:
-	dma_buf_put(dmabuf);
 out_free_gem:
 	i915_gem_object_put(obj);
 out:
diff --git a/drivers/gpu/drm/ttm/ttm_object.c b/drivers/gpu/drm/ttm/ttm_object.c
index 1aa2baa83959..30d29d17653f 100644
--- a/drivers/gpu/drm/ttm/ttm_object.c
+++ b/drivers/gpu/drm/ttm/ttm_object.c
@@ -730,8 +730,7 @@ int ttm_prime_handle_to_fd(struct ttm_object_file *tfile,
 	if (ret >= 0) {
 		*prime_fd = ret;
 		ret = 0;
-	} else
-		dma_buf_put(dma_buf);
+	}
 
 out_unref:
 	if (base)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index d3f7bb33a54d..3baf689241cc 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1881,15 +1881,13 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type,
 	if (ret < 0) {
 		dprintk(3, "buffer %d, plane %d failed to export (%d)\n",
 			index, plane, ret);
-		dma_buf_put(dbuf);
-		return ret;
+	} else {
+		dprintk(3, "buffer %d, plane %d exported as %d descriptor\n",
+			index, plane, ret);
+		*fd = ret;
+		ret = 0;
 	}
-
-	dprintk(3, "buffer %d, plane %d exported as %d descriptor\n",
-		index, plane, ret);
-	*fd = ret;
-
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vb2_core_expbuf);
 
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index e74db7902549..c9fcb2a2f518 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -381,7 +381,6 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
 	struct ion_buffer *buffer = NULL;
 	struct ion_heap *heap;
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
-	int fd;
 	struct dma_buf *dmabuf;
 
 	pr_debug("%s: len %zu heap_id_mask %u flags %x\n", __func__,
@@ -425,11 +424,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
 		return PTR_ERR(dmabuf);
 	}
 
-	fd = dma_buf_fd(dmabuf, O_CLOEXEC);
-	if (fd < 0)
-		dma_buf_put(dmabuf);
-
-	return fd;
+	return dma_buf_fd(dmabuf, O_CLOEXEC);
 }
 
 int ion_query_heaps(struct ion_heap_query *query)
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index dd46b758852a..1cb54f6f3c6a 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -125,7 +125,6 @@ static int tee_ioctl_version(struct tee_context *ctx,
 static int tee_ioctl_shm_alloc(struct tee_context *ctx,
 			       struct tee_ioctl_shm_alloc_data __user *udata)
 {
-	long ret;
 	struct tee_ioctl_shm_alloc_data data;
 	struct tee_shm *shm;
 
@@ -144,25 +143,18 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
 	data.flags = shm->flags;
 	data.size = shm->size;
 
-	if (copy_to_user(udata, &data, sizeof(data)))
-		ret = -EFAULT;
-	else
-		ret = tee_shm_get_fd(shm);
+	if (unlikely(copy_to_user(udata, &data, sizeof(data)))) {
+		tee_shm_put(shm);
+		return -EFAULT;
+	}
 
-	/*
-	 * When user space closes the file descriptor the shared memory
-	 * should be freed or if tee_shm_get_fd() failed then it will
-	 * be freed immediately.
-	 */
-	tee_shm_put(shm);
-	return ret;
+	return tee_shm_get_fd(shm);
 }
 
 static int
 tee_ioctl_shm_register(struct tee_context *ctx,
 		       struct tee_ioctl_shm_register_data __user *udata)
 {
-	long ret;
 	struct tee_ioctl_shm_register_data data;
 	struct tee_shm *shm;
 
@@ -182,17 +174,12 @@ tee_ioctl_shm_register(struct tee_context *ctx,
 	data.flags = shm->flags;
 	data.length = shm->size;
 
-	if (copy_to_user(udata, &data, sizeof(data)))
-		ret = -EFAULT;
-	else
-		ret = tee_shm_get_fd(shm);
-	/*
-	 * When user space closes the file descriptor the shared memory
-	 * should be freed or if tee_shm_get_fd() failed then it will
-	 * be freed immediately.
-	 */
-	tee_shm_put(shm);
-	return ret;
+	if (unlikely(copy_to_user(udata, &data, sizeof(data)))) {
+		tee_shm_put(shm);
+		return -EFAULT;
+	}
+
+	return tee_shm_get_fd(shm);
 }
 
 static int params_from_user(struct tee_context *ctx, struct tee_param *params,
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 07d3be6f0780..53b35b5e5776 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -355,16 +355,10 @@ EXPORT_SYMBOL_GPL(tee_shm_register);
  */
 int tee_shm_get_fd(struct tee_shm *shm)
 {
-	int fd;
-
 	if (!(shm->flags & TEE_SHM_DMA_BUF))
 		return -EINVAL;
 
-	get_dma_buf(shm->dmabuf);
-	fd = dma_buf_fd(shm->dmabuf, O_CLOEXEC);
-	if (fd < 0)
-		dma_buf_put(shm->dmabuf);
-	return fd;
+	return dma_buf_fd(shm->dmabuf, O_CLOEXEC);
 }
 
 /**
-- 
2.17.0