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/<P1 pid>/fd/<num>]
>>
>> 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+3643a18836bce555bff6@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Cc: <stable@xxxxxxxxxxxxxxx> [5.3+]
>> Signed-off-by: Charan Teja Reddy <charante@xxxxxxxxxxxxxx>
>> ---
>>
>> 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 <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>
>> @@ -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 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?
>
Yes, I am just storing the name string in the d_fsdata in place of
dmabuf and this helps to get rid of any extra refcount requirement.
Because the user passed name carried in the d_fsdata is copied to the
local buffer in dmabuffs_dname under spin_lock(d_lock) and the same
d_fsdata is set to NULL(under the d_lock only) when that dmabuf is in
the release path. So, when d_fsdata is NULL, name string is not accessed
from the dmabuffs_dname thus extra count is not required.
String memory, stored in the dmabuf->name, is released from the
dma_buf_release(). Flow will be like, It fist sets d_fsdata=NULL and
then free the dmabuf->name.
However from your comments I have realized that there is a race in this
patch when using the name string between dma_buf_set_name() and
dmabuffs_dname(). But, If the idea of passing the name string inplace of
dmabuf in d_fsdata looks fine, I can update this next patch.
> thanks,
>
> greg k-h
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
