[PATCH] dma-buf: Fix possible UAF in dma_buf_export

[cuigaosheng]

> 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);
> 196         if (ret)
> 197                 goto err_sysfs_dmabuf;
> 198
> 199         return 0;
> 200
> 201 err_sysfs_dmabuf:
> 202         kobject_put(&sysfs_entry->kobj);
> 203         dmabuf->sysfs_entry = NULL;
> 204         return ret;
> 205 }
Did I miss something?

Thanks.

On 2022/11/24 20:37, Christian König wrote:
>
>
> Am 24.11.22 um 13:05 schrieb cuigaosheng:
>> Some tips:
>>     Before we call the dma_buf_stats_setup(), we have to finish 
>> creating the file,
>> otherwise dma_buf_stats_setup() will return -EINVAL, maybe we need to 
>> think about
>> this when making a new patch.
>
> I was already wondering why the order is this way.
>
> Why is dma_buf_stats_setup() needing the file in the first place?
>
> Thanks,
> Christian.
>
>>
>> Hope these tips are useful, thanks!
>>
>> On 2022/11/24 13:56, Charan Teja Kalla wrote:
>>> Thanks T.J and Christian for the inputs.
>>>
>>> On 11/19/2022 7:00 PM, Christian König wrote:
>>>>>      Yes, exactly that's the idea.
>>>>>
>>>>>      The only alternatives I can see would be to either move 
>>>>> allocating
>>>>>      the
>>>>>      file and so completing the dma_buf initialization last again 
>>>>> or just
>>>>>      ignore errors from sysfs.
>>>>>
>>>>>      > If we still want to avoid calling 
>>>>> dmabuf->ops->release(dmabuf) in
>>>>>      > dma_buf_release like the comment says I guess we could use
>>>>>      sysfs_entry
>>>>>      > and ERR_PTR to flag that, otherwise it looks like we'd need 
>>>>> a bit
>>>>>      > somewhere.
>>>>>
>>>>>      No, this should be dropped as far as I can see. The sysfs 
>>>>> cleanup
>>>>>      code
>>>>>      looks like it can handle not initialized kobj pointers.
>>>>>
>>>>>
>>>>> Yeah there is also the null check in dma_buf_stats_teardown() that
>>>>> would prevent it from running, but I understood the comment to be
>>>>> referring to the release() dma_buf_ops call into the exporter which
>>>>> comes right after the teardown call. That looks like it's preventing
>>>>> the fput task work calling back into the exporter after the exporter
>>>>> already got an error from dma_buf_export(). Otherwise the exporter
>>>>> sees a release() for a buffer that it doesn't know about / thinks
>>>>> shouldn't exist. So I could imagine an exporter trying to double 
>>>>> free:
>>>>> once for the failed dma_buf_export() call, and again when the
>>>>> release() op is called later.
>>>>
>>>> Oh, very good point as well. Yeah, then creating the file should
>>>> probably come last.
>>>>
>>> @Gaosheng: Could you please make these changes or you let me to do?
>>>
>>>> Regards,
>>>> Christian.
>>> .
>
> .