Date:   Thu, 18 Jun 2020 15:28:49 +0530
From:   Charan Teja Kalla <charante@...eaurora.org>
To:     "Ruhl, Michael J" <michael.j.ruhl@...el.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        "open list:DMA BUFFER SHARING FRAMEWORK" 
        <linux-media@...r.kernel.org>,
        DRI mailing list <dri-devel@...ts.freedesktop.org>
Cc:     Linaro MM SIG <linaro-mm-sig@...ts.linaro.org>,
        "vinmenon@...eaurora.org" <vinmenon@...eaurora.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] dmabuf: use spinlock to access dmabuf->name



On 6/17/2020 11:13 PM, Ruhl, Michael J wrote:
>> -----Original Message-----
>> From: charante=codeaurora.org@...codeaurora.org
>> <charante=codeaurora.org@...codeaurora.org> On Behalf Of Charan Teja
>> Kalla
>> Sent: Wednesday, June 17, 2020 2:29 AM
>> To: Ruhl, Michael J <michael.j.ruhl@...el.com>; Sumit Semwal
>> <sumit.semwal@...aro.org>; open list:DMA BUFFER SHARING FRAMEWORK
>> <linux-media@...r.kernel.org>; DRI mailing list <dri-
>> devel@...ts.freedesktop.org>
>> Cc: Linaro MM SIG <linaro-mm-sig@...ts.linaro.org>;
>> vinmenon@...eaurora.org; LKML <linux-kernel@...r.kernel.org>;
>> stable@...r.kernel.org
>> Subject: Re: [PATCH] dmabuf: use spinlock to access dmabuf->name
>>
>> Thanks Michael for the comments..
>>
>> On 6/16/2020 7:29 PM, Ruhl, Michael J wrote:
>>>> -----Original Message-----
>>>> From: dri-devel <dri-devel-bounces@...ts.freedesktop.org> On Behalf Of
>>>> Ruhl, Michael J
>>>> Sent: Tuesday, June 16, 2020 9:51 AM
>>>> To: Charan Teja Kalla <charante@...eaurora.org>; Sumit Semwal
>>>> <sumit.semwal@...aro.org>; open list:DMA BUFFER SHARING
>> FRAMEWORK
>>>> <linux-media@...r.kernel.org>; DRI mailing list <dri-
>>>> devel@...ts.freedesktop.org>
>>>> Cc: Linaro MM SIG <linaro-mm-sig@...ts.linaro.org>;
>>>> vinmenon@...eaurora.org; LKML <linux-kernel@...r.kernel.org>;
>>>> stable@...r.kernel.org
>>>> Subject: RE: [PATCH] dmabuf: use spinlock to access dmabuf->name
>>>>
>>>>> -----Original Message-----
>>>>> From: dri-devel <dri-devel-bounces@...ts.freedesktop.org> On Behalf Of
>>>>> Charan Teja Kalla
>>>>> Sent: Thursday, June 11, 2020 9:40 AM
>>>>> To: Sumit Semwal <sumit.semwal@...aro.org>; open list:DMA BUFFER
>>>>> SHARING FRAMEWORK <linux-media@...r.kernel.org>; DRI mailing list
>> <dri-
>>>>> devel@...ts.freedesktop.org>
>>>>> Cc: Linaro MM SIG <linaro-mm-sig@...ts.linaro.org>;
>>>>> vinmenon@...eaurora.org; LKML <linux-kernel@...r.kernel.org>;
>>>>> stable@...r.kernel.org
>>>>> Subject: [PATCH] dmabuf: use spinlock to access dmabuf->name
>>>>>
>>>>> There exists a sleep-while-atomic bug while accessing the dmabuf->name
>>>>> under mutex in the dmabuffs_dname(). This is caused from the SELinux
>>>>> permissions checks on a process where it tries to validate the inherited
>>>>> files from fork() by traversing them through iterate_fd() (which
>>>>> traverse files under spin_lock) and call
>>>>> match_file(security/selinux/hooks.c) where the permission checks
>> happen.
>>>>> This audit information is logged using dump_common_audit_data()
>> where it
>>>>> calls d_path() to get the file path name. If the file check happen on
>>>>> the dmabuf's fd, then it ends up in ->dmabuffs_dname() and use mutex
>> to
>>>>> access dmabuf->name. The flow will be like below:
>>>>> flush_unauthorized_files()
>>>>>  iterate_fd()
>>>>>    spin_lock() --> Start of the atomic section.
>>>>>      match_file()
>>>>>        file_has_perm()
>>>>>          avc_has_perm()
>>>>>            avc_audit()
>>>>>              slow_avc_audit()
>>>>> 	        common_lsm_audit()
>>>>> 		  dump_common_audit_data()
>>>>> 		    audit_log_d_path()
>>>>> 		      d_path()
>>>>>                        dmabuffs_dname()
>>>>>                          mutex_lock()--> Sleep while atomic.
>>>>>
>>>>> Call trace captured (on 4.19 kernels) is below:
>>>>> ___might_sleep+0x204/0x208
>>>>> __might_sleep+0x50/0x88
>>>>> __mutex_lock_common+0x5c/0x1068
>>>>> __mutex_lock_common+0x5c/0x1068
>>>>> mutex_lock_nested+0x40/0x50
>>>>> dmabuffs_dname+0xa0/0x170
>>>>> d_path+0x84/0x290
>>>>> audit_log_d_path+0x74/0x130
>>>>> common_lsm_audit+0x334/0x6e8
>>>>> slow_avc_audit+0xb8/0xf8
>>>>> avc_has_perm+0x154/0x218
>>>>> file_has_perm+0x70/0x180
>>>>> match_file+0x60/0x78
>>>>> iterate_fd+0x128/0x168
>>>>> selinux_bprm_committing_creds+0x178/0x248
>>>>> security_bprm_committing_creds+0x30/0x48
>>>>> install_exec_creds+0x1c/0x68
>>>>> load_elf_binary+0x3a4/0x14e0
>>>>> search_binary_handler+0xb0/0x1e0
>>>>>
>>>>> So, use spinlock to access dmabuf->name to avoid sleep-while-atomic.
>>>>>
>>>>> Cc: <stable@...r.kernel.org> [5.3+]
>>>>> Signed-off-by: Charan Teja Reddy <charante@...eaurora.org>
>>>>> ---
>>>>> drivers/dma-buf/dma-buf.c | 13 +++++++------
>>>>> include/linux/dma-buf.h   |  1 +
>>>>> 2 files changed, 8 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>>> index 01ce125..2e0456c 100644
>>>>> --- a/drivers/dma-buf/dma-buf.c
>>>>> +++ b/drivers/dma-buf/dma-buf.c
>>>>> @@ -45,10 +45,10 @@ static char *dmabuffs_dname(struct dentry
>> *dentry,
>>>>> char *buffer, int buflen)
>>>>> 	size_t ret = 0;
>>>>>
>>>>> 	dmabuf = dentry->d_fsdata;
>>>>> -	dma_resv_lock(dmabuf->resv, NULL);
>>>>> +	spin_lock(&dmabuf->name_lock);
>>>>> 	if (dmabuf->name)
>>>>> 		ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN);
>>>>> -	dma_resv_unlock(dmabuf->resv);
>>>>> +	spin_unlock(&dmabuf->name_lock);
>>>>
>>>> I am not really clear on why you need this lock.
>>>>
>>>> If name == NULL you have no issues.
>>>> If name is real, you have no issues.
>>
>> Yeah, ideal cases...
>>
>>>>
>>>> If name is freed you will copy garbage, but the only way
>>>> for that to happen is that _set_name or _release have to be called
>>>> at just the right time.
>>>>
>>>> And the above would probably only be an issue if the set_name
>>>> was called, so you will get NULL or a real name.
>>
>> And there exists a use-after-free to avoid which requires the lock. Say
>> that memcpy() in dmabuffs_dname is in progress and in parallel _set_name
>> will free the same buffer that memcpy is operating on.
> 
> Hmm...  I can see that.
> 
> However, note that in dma_buf_set_name, you cannot use the spinlock
> to protect the dma_buf->attachements list.
> 
> I think you need to do this:
> 
> 	dma_resv_lock(dmabuf->resv, NULL);
>  	if (!list_empty(&dmabuf->attachments)) {
>  		ret = -EBUSY;
>  		kfree(name);
>               }
> 	dma_resv_unlock(dmabuf->resv, NULL);
> 	if (ret)
> 		return ret;
> 
> 	spinlock(nam_lock)
> 	namestuff;
> 	spinunlock

Hmm..Yes, I should use the dma_resv_lock() to access the ->attachments
list. Will correct this in V2.

> 
> 	return 0;
> 
> Mike
> 
>>>> Is there a reason for the lock here?
>>>>
>>>> Mike
>>>
>>> Maybe dmabuf->name = NULL after the kfree(dmabuf->name) in:
>>>
>>> dma_buf_release()
>>>
>>> Would be sufficient?
>>
>> I don't think that we will access the 'dmabuf'(thus dmabuf->name) once
>> it is in the dma_buf_release(). So, setting the NULL in the _release()
>> is not required at all.
>>
>>>
>>> M
>>>>> 	return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
>>>>> 			     dentry->d_name.name, ret > 0 ? name : "");
>>>>> @@ -335,7 +335,7 @@ static long dma_buf_set_name(struct dma_buf
>>>>> *dmabuf, const char __user *buf)
>>>>> 	if (IS_ERR(name))
>>>>> 		return PTR_ERR(name);
>>>>>
>>>>> -	dma_resv_lock(dmabuf->resv, NULL);
>>>>> +	spin_lock(&dmabuf->name_lock);
>>>>> 	if (!list_empty(&dmabuf->attachments)) {
>>>>> 		ret = -EBUSY;
>>>>> 		kfree(name);
>>>>> @@ -345,7 +345,7 @@ static long dma_buf_set_name(struct dma_buf
>>>>> *dmabuf, const char __user *buf)
>>>>> 	dmabuf->name = name;
>>>>>
>>>>> out_unlock:
>>>>> -	dma_resv_unlock(dmabuf->resv);
>>>>> +	spin_unlock(&dmabuf->name_lock);
>>>>> 	return ret;
>>>>> }
>>>>>
>>>>> @@ -405,10 +405,10 @@ static void dma_buf_show_fdinfo(struct
>> seq_file
>>>>> *m, struct file *file)
>>>>> 	/* Don't count the temporary reference taken inside procfs seq_show
>>>>> */
>>>>> 	seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1);
>>>>> 	seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name);
>>>>> -	dma_resv_lock(dmabuf->resv, NULL);
>>>>> +	spin_lock(&dmabuf->name_lock);
>>>>> 	if (dmabuf->name)
>>>>> 		seq_printf(m, "name:\t%s\n", dmabuf->name);
>>>>> -	dma_resv_unlock(dmabuf->resv);
>>>>> +	spin_unlock(&dmabuf->name_lock);
>>>>> }
>>>>>
>>>>> static const struct file_operations dma_buf_fops = {
>>>>> @@ -546,6 +546,7 @@ struct dma_buf *dma_buf_export(const struct
>>>>> dma_buf_export_info *exp_info)
>>>>> 	dmabuf->size = exp_info->size;
>>>>> 	dmabuf->exp_name = exp_info->exp_name;
>>>>> 	dmabuf->owner = exp_info->owner;
>>>>> +	spin_lock_init(&dmabuf->name_lock);
>>>>> 	init_waitqueue_head(&dmabuf->poll);
>>>>> 	dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
>>>>> 	dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
>>>>> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
>>>>> index ab0c156..93108fd 100644
>>>>> --- a/include/linux/dma-buf.h
>>>>> +++ b/include/linux/dma-buf.h
>>>>> @@ -311,6 +311,7 @@ struct dma_buf {
>>>>> 	void *vmap_ptr;
>>>>> 	const char *exp_name;
>>>>> 	const char *name;
>>>>> +	spinlock_t name_lock;
>>>>> 	struct module *owner;
>>>>> 	struct list_head list_node;
>>>>> 	void *priv;
>>>>> --
>>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>>> Forum, a Linux Foundation Collaborative Project
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@...ts.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@...ts.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum, a Linux Foundation Collaborative Project

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Powered by blists - more mailing lists