[Linaro-mm-sig] Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export

[Charan Teja Kalla]

Thanks Christian/TJ for all your inputs!!

On 11/24/2022 6:25 PM, Christian König wrote:
>>> I was already wondering why the order is this way.
>>>
>>> Why is dma_buf_stats_setup() needing the file in the first place? 
>>
>> dmabuf->file will be used in dma_buf_stats_setup(), the
>> dma_buf_stats_setup() as follows:
>>
>>> 171 int dma_buf_stats_setup(struct dma_buf *dmabuf)
>>> 172 {
>>> 173         struct dma_buf_sysfs_entry *sysfs_entry;
>>> 174         int ret;
>>> 175
>>> 176         if (!dmabuf || !dmabuf->file)
>>> 177                 return -EINVAL;
>>> 178
>>> 179         if (!dmabuf->exp_name) {
>>> 180                 pr_err("exporter name must not be empty if stats
>>> needed\n");
>>> 181                 return -EINVAL;
>>> 182         }
>>> 183
>>> 184         sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry),
>>> GFP_KERNEL);
>>> 185         if (!sysfs_entry)
>>> 186                 return -ENOMEM;
>>> 187
>>> 188         sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset;
>>> 189         sysfs_entry->dmabuf = dmabuf;
>>> 190
>>> 191         dmabuf->sysfs_entry = sysfs_entry;
>>> 192
>>> 193         /* create the directory for buffer stats */
>>> 194         ret = kobject_init_and_add(&sysfs_entry->kobj,
>>> &dma_buf_ktype, NULL,
>>> 195                                    "%lu",
>>> file_inode(dmabuf->file)->i_ino);
> 
> Ah, so it uses the i_ino of the file for the sysfs unique name.
> 
> I'm going to take another look how to properly clean this up.
> 

How about deleting the dmabuf from the db_list directly in the error
path (which is usually done by the fput()) and then continue with the
normal fput() here.

Just compile tested the below code and If the logic make sense for you,
will send the final tested patch.
----------------------><---------------------------------------------

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index e6f36c0..10a1727 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -87,19 +87,28 @@ static void dma_buf_release(struct dentry *dentry)
        kfree(dmabuf);
 }

-static int dma_buf_file_release(struct inode *inode, struct file *file)
+static void dma_buf_db_list_remove(struct file *file)
 {
        struct dma_buf *dmabuf;

-       if (!is_dma_buf_file(file))
-               return -EINVAL;
-
        dmabuf = file->private_data;
+       if (!dmabuf)
+               return;

        mutex_lock(&db_list.lock);
        list_del(&dmabuf->list_node);
        mutex_unlock(&db_list.lock);

+       file->private_data = NULL;
+}
+
+static int dma_buf_file_release(struct inode *inode, struct file *file)
+{
+       if (!is_dma_buf_file(file))
+               return -EINVAL;
+
+       dma_buf_db_list_remove(file);
+
        return 0;
 }

@@ -688,6 +697,8 @@ struct dma_buf *dma_buf_export(const struct
dma_buf_export_info *exp_info)
         * early before calling the release() dma_buf op.
         */
        file->f_path.dentry->d_fsdata = NULL;
+
+       dma_buf_db_list_remove(file);
        fput(file);
 err_dmabuf:
        kfree(dmabuf);

--------------------><-----------------------------


> Thanks for pointing this out,
> Christian.