Re: [PATCH] dma-buf: fix use-after-free in dmabuffs_dname

[Charan Teja Kalla <charante@codeaurora.org>]

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@syzkaller.appspotmail.com
>>>> Signed-off-by: Charan Teja Reddy<charante@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