[Linaro-mm-sig] Re: [PATCH] dma-buf: Fix possible UAF in dma_buf_export
Christian König
ckoenig.leichtzumerken at gmail.com
Thu Nov 24 12:55:39 UTC 2022
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 at lists.linaro.org
> To unsubscribe send an email to linaro-mm-sig-leave at lists.linaro.org
More information about the dri-devel
mailing list