Am 14.07.21 um 09:11 schrieb guangming.cao@xxxxxxxxxxxx:
From: Guangming Cao <Guangming.Cao@xxxxxxxxxxxx>
Add a refcount for kernel to prevent UAF(Use After Free) issue.
Well NAK on so many levels.
We can assume a case like below:
1. kernel space alloc dma_buf(file count = 1)
2. kernel use dma_buf to get fd(file count = 1)
3. userspace use fd to do mapping (file count = 2)
Creating an userspace mapping increases the reference count for the
underlying file object.
See the implementation of mmap_region():
...
vma->vm_file = get_file(file);
error = call_mmap(file, vma);
...
What can happen is the the underlying exporter redirects the mmap to a
different file, e.g. TTM or GEM drivers do that all the time.
But this is fine since then the VA mapping is independent of the DMA-buf.
4. kernel call dma_buf_put (file count = 1)
5. userpsace close buffer fd(file count = 0)
6. at this time, buffer is released, but va is valid!!
So we still can read/write buffer via mmap va,
it maybe cause memory leak, or kernel exception.
And also, if we use "ls -ll" to watch corresponding process
fd link info, it also will cause kernel exception.
Another case:
Using dma_buf_fd to generate more than 1 fd, because
dma_buf_fd will not increase file count, thus, when close
the second fd, it maybe occurs error.
Each opened fd will increase the reference count so this is certainly
not correct what you describe here.
Regards,
Christian.
Solution:
Add a kernel count for dma_buf, and make sure the file count
of dma_buf.file hold by kernel is 1.
Notes: For this solution, kref couldn't work because kernel ref
maybe added from 0, but kref don't allow it.
Signed-off-by: Guangming Cao <Guangming.Cao@xxxxxxxxxxxx>
---
drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++----
include/linux/dma-buf.h | 6 ++++--
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 511fe0d217a0..04ee92aac8b9 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -62,6 +62,7 @@ static void dma_buf_release(struct dentry *dentry)
if (unlikely(!dmabuf))
return;
+ WARN_ON(atomic64_read(&dmabuf->kernel_ref));
BUG_ON(dmabuf->vmapping_counter);
/*
@@ -555,6 +556,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
goto err_module;
}
+ atomic64_set(&dmabuf->kernel_ref, 1);
dmabuf->priv = exp_info->priv;
dmabuf->ops = exp_info->ops;
dmabuf->size = exp_info->size;
@@ -617,6 +619,9 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
fd_install(fd, dmabuf->file);
+ /* Add file cnt for each new fd */
+ get_file(dmabuf->file);
+
return fd;
}
EXPORT_SYMBOL_GPL(dma_buf_fd);
@@ -626,12 +631,13 @@ EXPORT_SYMBOL_GPL(dma_buf_fd);
* @fd: [in] fd associated with the struct dma_buf to be returned
*
* On success, returns the struct dma_buf associated with an fd; uses
- * file's refcounting done by fget to increase refcount. returns ERR_PTR
- * otherwise.
+ * dmabuf's ref refcounting done by kref_get to increase refcount.
+ * Returns ERR_PTR otherwise.
*/
struct dma_buf *dma_buf_get(int fd)
{
struct file *file;
+ struct dma_buf *dmabuf;
file = fget(fd);
@@ -643,7 +649,12 @@ struct dma_buf *dma_buf_get(int fd)
return ERR_PTR(-EINVAL);
}
- return file->private_data;
+ dmabuf = file->private_data;
+ /* replace file count increase as ref increase for kernel user */
+ get_dma_buf(dmabuf);
+ fput(file);
+
+ return dmabuf;
}
EXPORT_SYMBOL_GPL(dma_buf_get);
@@ -662,7 +673,11 @@ void dma_buf_put(struct dma_buf *dmabuf)
if (WARN_ON(!dmabuf || !dmabuf->file))
return;
- fput(dmabuf->file);
+ if (WARN_ON(!atomic64_read(&dmabuf->kernel_ref)))
+ return;
+
+ if (!atomic64_dec_return(&dmabuf->kernel_ref))
+ fput(dmabuf->file);
}
EXPORT_SYMBOL_GPL(dma_buf_put);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index efdc56b9d95f..bc790cb028eb 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -308,6 +308,7 @@ struct dma_buf_ops {
struct dma_buf {
size_t size;
struct file *file;
+ atomic64_t kernel_ref;
struct list_head attachments;
const struct dma_buf_ops *ops;
struct mutex lock;
@@ -436,7 +437,7 @@ struct dma_buf_export_info {
.owner = THIS_MODULE }
/**
- * get_dma_buf - convenience wrapper for get_file.
+ * get_dma_buf - increase a kernel ref of dma-buf
* @dmabuf: [in] pointer to dma_buf
*
* Increments the reference count on the dma-buf, needed in case of drivers
@@ -446,7 +447,8 @@ struct dma_buf_export_info {
*/
static inline void get_dma_buf(struct dma_buf *dmabuf)
{
- get_file(dmabuf->file);
+ if (atomic64_inc_return(&dmabuf->kernel_ref) == 1)
+ get_file(dmabuf->file);
}
/**
