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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 24.11.22 um 13:49 schrieb 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);

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.

Thanks for pointing this out,
Christian.

>> 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.
>>>> .
>>
>> .
> _______________________________________________
> Linaro-mm-sig mailing list -- linaro-mm-sig@lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave@lists.linaro.org