[PATCH] dma-buf: fix use-after-free in dmabuffs_dname
Charan Teja Kalla
charante at codeaurora.org
Tue May 12 05:13:18 UTC 2020
Thank you Greg for the comments.
On 5/6/2020 2:30 PM, Greg KH wrote:
> On Wed, May 06, 2020 at 02:00:10PM +0530, Charan Teja Kalla wrote:
>> Thank you Greg for the reply.
>>
>> On 5/5/2020 3:38 PM, Greg KH wrote:
>>> On Tue, Apr 28, 2020 at 01:24:02PM +0530, Charan Teja Reddy wrote:
>>>> The following race occurs while accessing the dmabuf object exported as
>>>> file:
>>>> P1 P2
>>>> dma_buf_release() dmabuffs_dname()
>>>> [say lsof reading /proc/<P1 pid>/fd/<num>]
>>>>
>>>> read dmabuf stored in dentry->fsdata
>>>> Free the dmabuf object
>>>> Start accessing the dmabuf structure
>>>>
>>>> In the above description, the dmabuf object freed in P1 is being
>>>> accessed from P2 which is resulting into the use-after-free. Below is
>>>> the dump stack reported.
>>>>
>>>> Call Trace:
>>>> kasan_report+0x12/0x20
>>>> __asan_report_load8_noabort+0x14/0x20
>>>> dmabuffs_dname+0x4f4/0x560
>>>> tomoyo_realpath_from_path+0x165/0x660
>>>> tomoyo_get_realpath
>>>> tomoyo_check_open_permission+0x2a3/0x3e0
>>>> tomoyo_file_open
>>>> tomoyo_file_open+0xa9/0xd0
>>>> security_file_open+0x71/0x300
>>>> do_dentry_open+0x37a/0x1380
>>>> vfs_open+0xa0/0xd0
>>>> path_openat+0x12ee/0x3490
>>>> do_filp_open+0x192/0x260
>>>> do_sys_openat2+0x5eb/0x7e0
>>>> do_sys_open+0xf2/0x180
>>>>
>>>> Fixes: bb2bb90 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
>>> Nit, please read the documentation for how to do a Fixes: line properly,
>>> you need more digits:
>>> Fixes: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls")
>>
>> Will update the patch
>>
>>
>>>> Reported-by:syzbot+3643a18836bce555bff6 at syzkaller.appspotmail.com
>>>> Signed-off-by: Charan Teja Reddy<charante at codeaurora.org>
>>> Also, any reason you didn't include the other mailing lists that
>>> get_maintainer.pl said to?
>>
>> Really sorry for not sending to complete list. Added now.
>>
>>
>>> And finally, no cc: stable in the s-o-b area for an issue that needs to
>>> be backported to older kernels?
>>
>> Will update the patch.
>>
>>
>>>> ---
>>>> drivers/dma-buf/dma-buf.c | 25 +++++++++++++++++++++++--
>>>> include/linux/dma-buf.h | 1 +
>>>> 2 files changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>> index 570c923..069d8f78 100644
>>>> --- a/drivers/dma-buf/dma-buf.c
>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>> @@ -25,6 +25,7 @@
>>>> #include <linux/mm.h>
>>>> #include <linux/mount.h>
>>>> #include <linux/pseudo_fs.h>
>>>> +#include <linux/dcache.h>
>>>> #include <uapi/linux/dma-buf.h>
>>>> #include <uapi/linux/magic.h>
>>>> @@ -38,18 +39,34 @@ struct dma_buf_list {
>>>> static struct dma_buf_list db_list;
>>>> +static void dmabuf_dent_put(struct dma_buf *dmabuf)
>>>> +{
>>>> + if (atomic_dec_and_test(&dmabuf->dent_count)) {
>>>> + kfree(dmabuf->name);
>>>> + kfree(dmabuf);
>>>> + }
>>> Why not just use a kref instead of an open-coded atomic value?
>>
>> Kref approach looks cleaner. will update the patch accordingly.
>>
>>
>>>> +}
>>>> +
>>>> static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen)
>>>> {
>>>> struct dma_buf *dmabuf;
>>>> char name[DMA_BUF_NAME_LEN];
>>>> size_t ret = 0;
>>>> + spin_lock(&dentry->d_lock);
>>>> dmabuf = dentry->d_fsdata;
>>>> + if (!dmabuf || !atomic_add_unless(&dmabuf->dent_count, 1, 0)) {
>>>> + spin_unlock(&dentry->d_lock);
>>>> + goto out;
>>> How can dmabuf not be valid here?
>>>
>>> And isn't there already a usecount for the buffer?
>>
>> dmabuf exported as file simply relies on that file's refcount, thus fput()
>> releases the dmabuf.
>>
>> We are storing the dmabuf in the dentry->d_fsdata but there is no binding
>> between the dentry and the dmabuf. So, flow will be like
>>
>> 1) P1 calls fput(dmabuf_fd)
>>
>> 2) P2 trying to access the file information of P1.
>> Eg: lsof command trying to list out the dmabuf_fd information using
>> /proc/<P1 pid>/fd/dmabuf_fd
>>
>> 3) P1 calls the file->f_op->release(dmabuf_fd_file)(ends up in calling
>> dma_buf_release()), thus frees up the dmabuf buffer.
>>
>> 4) P2 access the dmabuf stored in the dentry->d_fsdata which was freed in
>> step 3.
>>
>> So we need to have some refcount mechanism to avoid the use-after-free in
>> step 4.
> Ok, but watch out, now you have 2 different reference counts for the
> same structure. Keeping them coordinated is almost always an impossible
> task so you need to only rely on one. If you can't use the file api,
> just drop all of the reference counting logic in there and only use the
> kref one.
I feel that changing the refcount logic now to dma-buf objects involve
changes in
the core dma-buf framework. NO? Instead, how about passing the user
passed name directly
in the ->d_fsdata inplace of dmabuf object? Because we just need user
passed name in the
dmabuffs_dname(). With this we can avoid the need for extra refcount on
dmabuf.
Posted patch-V2: https://lkml.org/lkml/2020/5/8/158
>
> good luck!
>
> greg k-h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
More information about the dri-devel
mailing list