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

["Christian König" <christian.koenig@amd.com>]

Am 06.12.22 um 13:55 schrieb 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.

No, that's not really clean either.

Give me 10 Minutes, going to come up with something.

Regards,
Christian.

>
> 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.