Messages in this thread | | | Subject | Re: [PATCH] dma-buf: fix use-after-free in dmabuffs_dname | From | Charan Teja Kalla <> | Date | Tue, 12 May 2020 10:43:18 +0530 |
| |
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
| |