Re: [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname
Thank you for the reply. On 5/13/2020 9:33 PM, Sumit Semwal wrote: > On Wed, 13 May 2020 at 21:16, Daniel Vetter wrote: >> >> On Wed, May 13, 2020 at 02:51:12PM +0200, Greg KH wrote: >>> On Wed, May 13, 2020 at 05:40:26PM +0530, Charan Teja Kalla wrote: Thank you Greg for the comments. On 5/12/2020 2:22 PM, Greg KH wrote: > On Fri, May 08, 2020 at 12:11:03PM +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//fd/] >> >> read dmabuf stored in dentry->d_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. >> >> We are reading the dmabuf object stored in the dentry->d_fsdata but >> there is no binding between the dentry and the dmabuf which means that >> the dmabuf can be freed while it is being read from ->d_fsdata and >> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse >> with an extra refcount is not a viable solution as the exported dmabuf >> is already under file's refcount and keeping the multiple refcounts on >> the same object coordinated is not possible. >> >> As we are reading the dmabuf in ->d_fsdata just to get the user passed >> name, we can directly store the name in d_fsdata thus can avoid the >> reading of dmabuf altogether. >> >> 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") >> Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com >> Cc: [5.3+] >> Signed-off-by: Charan Teja Reddy >> --- >> >> Changes in v2: >> >> - Pass the user passed name in ->d_fsdata instead of dmabuf >> - Improve the commit message >> >> Changes in v1: (https://patchwork.kernel.org/patch/11514063/) >> >> drivers/dma-buf/dma-buf.c | 17 ++--- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 01ce125..0071f7d 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -40,15 +41,13 @@ struct dma_buf_list { >> >> 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; >> >> -dmabuf = dentry->d_fsdata; >> -dma_resv_lock(dmabuf->resv, NULL); >> -if (dmabuf->name) >> -ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); >> -dma_resv_unlock(dmabuf->resv); >> +spin_lock(&dentry->d_lock); > > Are you sure this lock always protects d_fsdata? I think yes. In the dma-buf.c, I have to make sure that d_fsdata should always be under d_lock thus it will be protected. (In this posted patch there is one place(in dma_buf_set_name) that is missed, will update this in V3). > >> +if (dentry->d_fsdata) >> +ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); >> +spin_unlock(&dentry->d_lock); >> >> return dynamic_dname(dentry, buffer, buflen, "/%s:%s", >> dentry->d_name.name, ret > 0 ? name : ""); > > If the above check fails the name will be what? How could d_name.name > be valid but d_fsdata not be valid? In case of check fails, empty string "" is appended to the name by the code, ret > 0 ? name : "", ret is initialized to zero. Thus the name string will be like "/dmabuf:". >>> >>> So multiple objects can have the same "name" if this happens to multiple >>> ones at once? Yes that it can happen. >>> Regarding the validity of d_fsdata, we are setting the dmabuf's dentry->d_fsdata to NULL in the dma_buf_release() thus can go invalid if tha
Re: [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname
On Wed, 13 May 2020 at 21:16, Daniel Vetter wrote: > > On Wed, May 13, 2020 at 02:51:12PM +0200, Greg KH wrote: > > On Wed, May 13, 2020 at 05:40:26PM +0530, Charan Teja Kalla wrote: > > > > > > Thank you Greg for the comments. > > > On 5/12/2020 2:22 PM, Greg KH wrote: > > > > On Fri, May 08, 2020 at 12:11:03PM +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//fd/] > > > >> > > > >> read dmabuf stored in dentry->d_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. > > > >> > > > >> We are reading the dmabuf object stored in the dentry->d_fsdata but > > > >> there is no binding between the dentry and the dmabuf which means that > > > >> the dmabuf can be freed while it is being read from ->d_fsdata and > > > >> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse > > > >> with an extra refcount is not a viable solution as the exported dmabuf > > > >> is already under file's refcount and keeping the multiple refcounts on > > > >> the same object coordinated is not possible. > > > >> > > > >> As we are reading the dmabuf in ->d_fsdata just to get the user passed > > > >> name, we can directly store the name in d_fsdata thus can avoid the > > > >> reading of dmabuf altogether. > > > >> > > > >> 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") > > > >> Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com > > > >> Cc: [5.3+] > > > >> Signed-off-by: Charan Teja Reddy > > > >> --- > > > >> > > > >> Changes in v2: > > > >> > > > >> - Pass the user passed name in ->d_fsdata instead of dmabuf > > > >> - Improve the commit message > > > >> > > > >> Changes in v1: (https://patchwork.kernel.org/patch/11514063/) > > > >> > > > >> drivers/dma-buf/dma-buf.c | 17 ++--- > > > >> 1 file changed, 10 insertions(+), 7 deletions(-) > > > >> > > > >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > > >> index 01ce125..0071f7d 100644 > > > >> --- a/drivers/dma-buf/dma-buf.c > > > >> +++ b/drivers/dma-buf/dma-buf.c > > > >> @@ -25,6 +25,7 @@ > > > >> #include > > > >> #include > > > >> #include > > > >> +#include > > > >> > > > >> #include > > > >> #include > > > >> @@ -40,15 +41,13 @@ struct dma_buf_list { > > > >> > > > >> 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; > > > >> > > > >> -dmabuf = dentry->d_fsdata; > > > >> -dma_resv_lock(dmabuf->resv, NULL); > > > >> -if (dmabuf->name) > > > >> -ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); > > > >> -dma_resv_unlock(dmabuf->resv); > > > >> +spin_lock(&dentry->d_lock); > > > > > > > > Are you sure this lock always protects d_fsdata? > > > > > > I think yes. In the dma-buf.c, I have to make sure that d_fsdata should > > > always be under d_lock thus it will be protected. (In this posted patch > > > there is one place(in dma_buf_set_name) that is missed, will update this > > > in V3). > > > > > > > > > > >> +if (dentry->d_fsdata) > > > >> +ret = strlcpy(name, dentry->d_fsdata, > > > >> DMA_BUF_NAME_LEN); > > > >> +spin_unlock(&dentry->d_lock); > > > >> > > > >> return dynamic_dname(dentry, buffer, buflen, "/%s:%s", > > > >> dentry->d_name.name, ret > 0 ? name : > > > >> ""); > > > > > > > > If the above check fails the name will be what? How could d_name.name > > > > be valid but d_fsdata not be valid? > > > > > > In case of check fails, empty string "" is appended to the name by the > > > code, ret > 0 ? name : "", ret is initialized to zero. Thus the name > > > string will be like "/dmabuf:". > > > > So multiple objects can have the same "name" if this happens to multiple > > ones at once? > > > > > Regarding the va
Re: [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname
On Wed, May 13, 2020 at 02:51:12PM +0200, Greg KH wrote: > On Wed, May 13, 2020 at 05:40:26PM +0530, Charan Teja Kalla wrote: > > > > Thank you Greg for the comments. > > On 5/12/2020 2:22 PM, Greg KH wrote: > > > On Fri, May 08, 2020 at 12:11:03PM +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//fd/] > > >> > > >> read dmabuf stored in dentry->d_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. > > >> > > >> We are reading the dmabuf object stored in the dentry->d_fsdata but > > >> there is no binding between the dentry and the dmabuf which means that > > >> the dmabuf can be freed while it is being read from ->d_fsdata and > > >> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse > > >> with an extra refcount is not a viable solution as the exported dmabuf > > >> is already under file's refcount and keeping the multiple refcounts on > > >> the same object coordinated is not possible. > > >> > > >> As we are reading the dmabuf in ->d_fsdata just to get the user passed > > >> name, we can directly store the name in d_fsdata thus can avoid the > > >> reading of dmabuf altogether. > > >> > > >> 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") > > >> Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com > > >> Cc: [5.3+] > > >> Signed-off-by: Charan Teja Reddy > > >> --- > > >> > > >> Changes in v2: > > >> > > >> - Pass the user passed name in ->d_fsdata instead of dmabuf > > >> - Improve the commit message > > >> > > >> Changes in v1: (https://patchwork.kernel.org/patch/11514063/) > > >> > > >> drivers/dma-buf/dma-buf.c | 17 ++--- > > >> 1 file changed, 10 insertions(+), 7 deletions(-) > > >> > > >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > > >> index 01ce125..0071f7d 100644 > > >> --- a/drivers/dma-buf/dma-buf.c > > >> +++ b/drivers/dma-buf/dma-buf.c > > >> @@ -25,6 +25,7 @@ > > >> #include > > >> #include > > >> #include > > >> +#include > > >> > > >> #include > > >> #include > > >> @@ -40,15 +41,13 @@ struct dma_buf_list { > > >> > > >> 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; > > >> > > >> -dmabuf = dentry->d_fsdata; > > >> -dma_resv_lock(dmabuf->resv, NULL); > > >> -if (dmabuf->name) > > >> -ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); > > >> -dma_resv_unlock(dmabuf->resv); > > >> +spin_lock(&dentry->d_lock); > > > > > > Are you sure this lock always protects d_fsdata? > > > > I think yes. In the dma-buf.c, I have to make sure that d_fsdata should > > always be under d_lock thus it will be protected. (In this posted patch > > there is one place(in dma_buf_set_name) that is missed, will update this > > in V3). > > > > > > > >> +if (dentry->d_fsdata) > > >> +ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); > > >> +spin_unlock(&dentry->d_lock); > > >> > > >> return dynamic_dname(dentry, buffer, buflen, "/%s:%s", > > >> dentry->d_name.name, ret > 0 ? name : ""); > > > > > > If the above check fails the name will be what? How could d_name.name > > > be valid but d_fsdata not be valid? > > > > In case of check fails, empty string "" is appended to the name by the > > code, ret > 0 ? name : "", ret is initialized to zero. Thus the name > > string will be like "/dmabuf:". > > So multiple objects can have the same "name" if this happens to multiple > ones at once? > > > Regarding the validity of d_fsdata, we are setting the dmabuf's > > dentry->d_fsdata to NULL in the dma_buf_release() thus can go invalid if > > that dmabuf is in the free path. > > Why are we allowing the name to be set if the dmabuf is on the free path > at all? Shouldn't that be the real fix here? > > >
Re: [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname
On Wed, May 13, 2020 at 05:40:26PM +0530, Charan Teja Kalla wrote: > > Thank you Greg for the comments. > On 5/12/2020 2:22 PM, Greg KH wrote: > > On Fri, May 08, 2020 at 12:11:03PM +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//fd/] > >> > >> read dmabuf stored in dentry->d_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. > >> > >> We are reading the dmabuf object stored in the dentry->d_fsdata but > >> there is no binding between the dentry and the dmabuf which means that > >> the dmabuf can be freed while it is being read from ->d_fsdata and > >> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse > >> with an extra refcount is not a viable solution as the exported dmabuf > >> is already under file's refcount and keeping the multiple refcounts on > >> the same object coordinated is not possible. > >> > >> As we are reading the dmabuf in ->d_fsdata just to get the user passed > >> name, we can directly store the name in d_fsdata thus can avoid the > >> reading of dmabuf altogether. > >> > >> 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") > >> Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com > >> Cc: [5.3+] > >> Signed-off-by: Charan Teja Reddy > >> --- > >> > >> Changes in v2: > >> > >> - Pass the user passed name in ->d_fsdata instead of dmabuf > >> - Improve the commit message > >> > >> Changes in v1: (https://patchwork.kernel.org/patch/11514063/) > >> > >> drivers/dma-buf/dma-buf.c | 17 ++--- > >> 1 file changed, 10 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > >> index 01ce125..0071f7d 100644 > >> --- a/drivers/dma-buf/dma-buf.c > >> +++ b/drivers/dma-buf/dma-buf.c > >> @@ -25,6 +25,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> #include > >> #include > >> @@ -40,15 +41,13 @@ struct dma_buf_list { > >> > >> 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; > >> > >> - dmabuf = dentry->d_fsdata; > >> - dma_resv_lock(dmabuf->resv, NULL); > >> - if (dmabuf->name) > >> - ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); > >> - dma_resv_unlock(dmabuf->resv); > >> + spin_lock(&dentry->d_lock); > > > > Are you sure this lock always protects d_fsdata? > > I think yes. In the dma-buf.c, I have to make sure that d_fsdata should > always be under d_lock thus it will be protected. (In this posted patch > there is one place(in dma_buf_set_name) that is missed, will update this > in V3). > > > > >> + if (dentry->d_fsdata) > >> + ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); > >> + spin_unlock(&dentry->d_lock); > >> > >>return dynamic_dname(dentry, buffer, buflen, "/%s:%s", > >> dentry->d_name.name, ret > 0 ? name : ""); > > > > If the above check fails the name will be what? How could d_name.name > > be valid but d_fsdata not be valid? > > In case of check fails, empty string "" is appended to the name by the > code, ret > 0 ? name : "", ret is initialized to zero. Thus the name > string will be like "/dmabuf:". So multiple objects can have the same "name" if this happens to multiple ones at once? > Regarding the validity of d_fsdata, we are setting the dmabuf's > dentry->d_fsdata to NULL in the dma_buf_release() thus can go invalid if > that dmabuf is in the free path. Why are we allowing the name to be set if the dmabuf is on the free path at all? Shouldn't that be the real fix here? > >> @@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context > >> *fc) > >> static int dma_buf_release(struct inode *inode, struct file *file) > >> { > >>struct dma_buf *dmabuf; > >> + struct dentry *dentry = file->f_path.dentry; > >> > >>if (!is_dma_buf_file(file)) > >>return -EINVAL; > >> > >>dmabuf = file->private_data; > >> > >> + spi
Re: [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname
Thank you Greg for the comments. On 5/12/2020 2:22 PM, Greg KH wrote: > On Fri, May 08, 2020 at 12:11:03PM +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//fd/] >> >> read dmabuf stored in dentry->d_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. >> >> We are reading the dmabuf object stored in the dentry->d_fsdata but >> there is no binding between the dentry and the dmabuf which means that >> the dmabuf can be freed while it is being read from ->d_fsdata and >> inuse. Reviews on the patch V1 says that protecting the dmabuf inuse >> with an extra refcount is not a viable solution as the exported dmabuf >> is already under file's refcount and keeping the multiple refcounts on >> the same object coordinated is not possible. >> >> As we are reading the dmabuf in ->d_fsdata just to get the user passed >> name, we can directly store the name in d_fsdata thus can avoid the >> reading of dmabuf altogether. >> >> 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") >> Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com >> Cc: [5.3+] >> Signed-off-by: Charan Teja Reddy >> --- >> >> Changes in v2: >> >> - Pass the user passed name in ->d_fsdata instead of dmabuf >> - Improve the commit message >> >> Changes in v1: (https://patchwork.kernel.org/patch/11514063/) >> >> drivers/dma-buf/dma-buf.c | 17 ++--- >> 1 file changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 01ce125..0071f7d 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -25,6 +25,7 @@ >> #include >> #include >> #include >> +#include >> >> #include >> #include >> @@ -40,15 +41,13 @@ struct dma_buf_list { >> >> 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; >> >> -dmabuf = dentry->d_fsdata; >> -dma_resv_lock(dmabuf->resv, NULL); >> -if (dmabuf->name) >> -ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); >> -dma_resv_unlock(dmabuf->resv); >> +spin_lock(&dentry->d_lock); > > Are you sure this lock always protects d_fsdata? I think yes. In the dma-buf.c, I have to make sure that d_fsdata should always be under d_lock thus it will be protected. (In this posted patch there is one place(in dma_buf_set_name) that is missed, will update this in V3). > >> +if (dentry->d_fsdata) >> +ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); >> +spin_unlock(&dentry->d_lock); >> >> return dynamic_dname(dentry, buffer, buflen, "/%s:%s", >> dentry->d_name.name, ret > 0 ? name : ""); > > If the above check fails the name will be what? How could d_name.name > be valid but d_fsdata not be valid? In case of check fails, empty string "" is appended to the name by the code, ret > 0 ? name : "", ret is initialized to zero. Thus the name string will be like "/dmabuf:". Regarding the validity of d_fsdata, we are setting the dmabuf's dentry->d_fsdata to NULL in the dma_buf_release() thus can go invalid if that dmabuf is in the free path. > > >> @@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc) >> static int dma_buf_release(struct inode *inode, struct file *file) >> { >> struct dma_buf *dmabuf; >> +struct dentry *dentry = file->f_path.dentry; >> >> if (!is_dma_buf_file(file)) >> return -EINVAL; >> >> dmabuf = file->private_data; >> >> +spin_lock(&dentry->d_lock); >> +dentry->d_fsdata = NULL; >> +spin_unlock(&dentry->d_lock); >> BUG_ON(dmabuf->vmapping_counter); >> >> /* >> @@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, >> const char __user *buf) >> } >> kfree(dmabuf->name); >> dmabuf->name = name; >> +dmabuf->file->f_path.dentry->d_fsdata = name; > > You are just changing the use of d_fsdata from being a pointer to the > dmabuf to being a pointer
Re: [PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname
On Fri, May 08, 2020 at 12:11:03PM +0530, Charan Teja Reddy wrote: > The following race occurs while accessing the dmabuf object exported as > file: > P1P2 > dma_buf_release() dmabuffs_dname() > [say lsof reading /proc//fd/] > > read dmabuf stored in dentry->d_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. > > We are reading the dmabuf object stored in the dentry->d_fsdata but > there is no binding between the dentry and the dmabuf which means that > the dmabuf can be freed while it is being read from ->d_fsdata and > inuse. Reviews on the patch V1 says that protecting the dmabuf inuse > with an extra refcount is not a viable solution as the exported dmabuf > is already under file's refcount and keeping the multiple refcounts on > the same object coordinated is not possible. > > As we are reading the dmabuf in ->d_fsdata just to get the user passed > name, we can directly store the name in d_fsdata thus can avoid the > reading of dmabuf altogether. > > 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") > Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com > Cc: [5.3+] > Signed-off-by: Charan Teja Reddy > --- > > Changes in v2: > > - Pass the user passed name in ->d_fsdata instead of dmabuf > - Improve the commit message > > Changes in v1: (https://patchwork.kernel.org/patch/11514063/) > > drivers/dma-buf/dma-buf.c | 17 ++--- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 01ce125..0071f7d 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -40,15 +41,13 @@ struct dma_buf_list { > > 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; > > - dmabuf = dentry->d_fsdata; > - dma_resv_lock(dmabuf->resv, NULL); > - if (dmabuf->name) > - ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); > - dma_resv_unlock(dmabuf->resv); > + spin_lock(&dentry->d_lock); Are you sure this lock always protects d_fsdata? > + if (dentry->d_fsdata) > + ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); > + spin_unlock(&dentry->d_lock); > > return dynamic_dname(dentry, buffer, buflen, "/%s:%s", >dentry->d_name.name, ret > 0 ? name : ""); If the above check fails the name will be what? How could d_name.name be valid but d_fsdata not be valid? > @@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc) > static int dma_buf_release(struct inode *inode, struct file *file) > { > struct dma_buf *dmabuf; > + struct dentry *dentry = file->f_path.dentry; > > if (!is_dma_buf_file(file)) > return -EINVAL; > > dmabuf = file->private_data; > > + spin_lock(&dentry->d_lock); > + dentry->d_fsdata = NULL; > + spin_unlock(&dentry->d_lock); > BUG_ON(dmabuf->vmapping_counter); > > /* > @@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, > const char __user *buf) > } > kfree(dmabuf->name); > dmabuf->name = name; > + dmabuf->file->f_path.dentry->d_fsdata = name; You are just changing the use of d_fsdata from being a pointer to the dmabuf to being a pointer to the name string? What's to keep that name string around and not have the same reference counting issues that the dmabuf structure itself has? Who frees that string memory? thanks, greg k-h
[PATCH v2] dma-buf: fix use-after-free in dmabuffs_dname
The following race occurs while accessing the dmabuf object exported as file: P1 P2 dma_buf_release() dmabuffs_dname() [say lsof reading /proc//fd/] read dmabuf stored in dentry->d_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. We are reading the dmabuf object stored in the dentry->d_fsdata but there is no binding between the dentry and the dmabuf which means that the dmabuf can be freed while it is being read from ->d_fsdata and inuse. Reviews on the patch V1 says that protecting the dmabuf inuse with an extra refcount is not a viable solution as the exported dmabuf is already under file's refcount and keeping the multiple refcounts on the same object coordinated is not possible. As we are reading the dmabuf in ->d_fsdata just to get the user passed name, we can directly store the name in d_fsdata thus can avoid the reading of dmabuf altogether. 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: bb2bb9030425 ("dma-buf: add DMA_BUF_SET_NAME ioctls") Reported-by: syzbot+3643a18836bce555b...@syzkaller.appspotmail.com Cc: [5.3+] Signed-off-by: Charan Teja Reddy --- Changes in v2: - Pass the user passed name in ->d_fsdata instead of dmabuf - Improve the commit message Changes in v1: (https://patchwork.kernel.org/patch/11514063/) drivers/dma-buf/dma-buf.c | 17 ++--- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 01ce125..0071f7d 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -40,15 +41,13 @@ struct dma_buf_list { 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; - dmabuf = dentry->d_fsdata; - dma_resv_lock(dmabuf->resv, NULL); - if (dmabuf->name) - ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); - dma_resv_unlock(dmabuf->resv); + spin_lock(&dentry->d_lock); + if (dentry->d_fsdata) + ret = strlcpy(name, dentry->d_fsdata, DMA_BUF_NAME_LEN); + spin_unlock(&dentry->d_lock); return dynamic_dname(dentry, buffer, buflen, "/%s:%s", dentry->d_name.name, ret > 0 ? name : ""); @@ -80,12 +79,16 @@ static int dma_buf_fs_init_context(struct fs_context *fc) static int dma_buf_release(struct inode *inode, struct file *file) { struct dma_buf *dmabuf; + struct dentry *dentry = file->f_path.dentry; if (!is_dma_buf_file(file)) return -EINVAL; dmabuf = file->private_data; + spin_lock(&dentry->d_lock); + dentry->d_fsdata = NULL; + spin_unlock(&dentry->d_lock); BUG_ON(dmabuf->vmapping_counter); /* @@ -343,6 +346,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) } kfree(dmabuf->name); dmabuf->name = name; + dmabuf->file->f_path.dentry->d_fsdata = name; out_unlock: dma_resv_unlock(dmabuf->resv); @@ -446,7 +450,6 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) goto err_alloc_file; file->f_flags = flags & (O_ACCMODE | O_NONBLOCK); file->private_data = dmabuf; - file->f_path.dentry->d_fsdata = dmabuf; return file; -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation