Message-ID: <14063C7AD467DE4B82DEDB5C278E866301154C342E@FMSMSX108.amr.corp.intel.com>
Date:   Tue, 23 Jun 2020 14:23:18 +0000
From:   "Ruhl, Michael J" <michael.j.ruhl@...el.com>
To:     Charan Teja Kalla <charante@...eaurora.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        "David.Laight@...LAB.COM" <David.Laight@...LAB.COM>,
        "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>,
        LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] dmabuf: use spinlock to access dmabuf->name

>-----Original Message-----
>From: charante=codeaurora.org@...codeaurora.org
><charante=codeaurora.org@...codeaurora.org> On Behalf Of Charan Teja
>Kalla
>Sent: Tuesday, June 23, 2020 7:34 AM
>To: Ruhl, Michael J <michael.j.ruhl@...el.com>; Sumit Semwal
><sumit.semwal@...aro.org>; David.Laight@...LAB.COM; 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>; LKML <linux-
>kernel@...r.kernel.org>
>Subject: Re: [PATCH v2] dmabuf: use spinlock to access dmabuf->name
>
>Thanks Mike for the inputs.
>
>On 6/22/2020 5:10 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: Monday, June 22, 2020 5:26 AM
>>> To: Ruhl, Michael J <michael.j.ruhl@...el.com>; Sumit Semwal
>>> <sumit.semwal@...aro.org>; David.Laight@...LAB.COM; 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>; LKML <linux-
>>> kernel@...r.kernel.org>
>>> Subject: Re: [PATCH v2] dmabuf: use spinlock to access dmabuf->name
>>>
>>> Hello Mike,
>>>
>>> On 6/19/2020 7:11 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: Friday, June 19, 2020 7:57 AM
>>>>> To: Sumit Semwal <sumit.semwal@...aro.org>; Ruhl, Michael J
>>>>> <michael.j.ruhl@...el.com>; David.Laight@...LAB.COM; 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>; LKML <linux-
>>>>> kernel@...r.kernel.org>
>>>>> Subject: [PATCH v2] 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>
>>>>> ---
>>>>>
>>>>> Changes in V2: Addressed review comments from Ruhl, Michael J
>>>>>
>>>>> Changes in V1: https://lore.kernel.org/patchwork/patch/1255055/
>>>>>
>>>>> drivers/dma-buf/dma-buf.c | 11 +++++++----
>>>>> include/linux/dma-buf.h   |  1 +
>>>>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>>>>> index 01ce125..d81d298 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);
>>>>>
>>>>> 	return dynamic_dname(dentry, buffer, buflen, "/%s:%s",
>>>>> 			     dentry->d_name.name, ret > 0 ? name : "");
>>>>> @@ -341,8 +341,10 @@ static long dma_buf_set_name(struct dma_buf
>>>>> *dmabuf, const char __user *buf)
>>>>> 		kfree(name);
>>>>> 		goto out_unlock;
>>>>> 	}
>>>>> +	spin_lock(&dmabuf->name_lock);
>>>>> 	kfree(dmabuf->name);
>>>>> 	dmabuf->name = name;
>>>>> +	spin_unlock(&dmabuf->name_lock);
>>>>
>>>> While this code path is ok, I would have separated the protection of the
>>>> attachment list and the name manipulation.
>>>>
>>>> dma_resv_lock(resv)
>>>> if (!list_empty(attachment)
>>>> 	ret = -EBUSY
>>>> dma_resv_unlock(resv)
>>>>
>>>> if (ret) {
>>>> 	kfree(name)
>>>> 	return ret;
>>>> }
>>>
>>> Is it that the name should be visible before importer attaches to the
>>> dmabuf,(using dma_buf_attach()), thus _buf_set_name() is under the
>>> _resv_lock() as well?
>>
>> That is the name that was being freed in the error path of the lock block.
>> Alternatively:
>>
>> dma_resv_lock(resv)
>> if (!list_empty(attachment) {
>>  	ret = -EBUSY
>> 	kfree(name)
>
>We can free this buffer even outside of the lock with out any issues.
>This is just a user passed name copied into local buffer yet to assign
>to dmabuf->name.
>
>> }
>> dma_resv_unlock(resv)
>>
>> if (ret)
>>  	return ret;
>>
>> I was limiting what was happening in the lock block.
>>
>> You have two distinct locks, that protect two distinct items:
>>
>> dmabuf->attachment
>> dmabuf->name
>>
>> Nesting the locking is ok, but if the code ever changes
>> you can get that nesting wrong, so:
>
>Your suggestion below looks clean, but what I am still not sure is that
>is there any condition like "there should be no attachments to the
>exported dmabuf before assigning the name" -- If yes, then _resv_lock
>and name_lock should be nested while assigning the name which otherwise
>breaks under below scenario:

I missed that piece of info, and I now understand the constraint.

Sorry for the extended conversation. 😊

I good with version 2.

Mike

>P1					P2
>
>buf_set_name() called and
>no attachments to this dmabuf
>yet.
>				attaches to the exported dmabuf by P1.
>
>				Say it tries to get the name with the
>				assumption that name is already set.
>Now it tries to
>change the name under
>just name_lock
>
>In the above case P2 didn't get any name of the exported dmabuf despite
>name is set.
>
>
>If not, then I can give V3 with the suggested changes..
>
>>
>> 	long ret = 0;
>>
>> 	if (IS_ERR(name))
>> 		return PTR_ERR(name);
>>
>> 	dma_resv_lock(dmabuf->resv, NULL);
>> 	if (!list_empty(&dmabuf->attachments)) {
>> 		ret = -EBUSY;
>> 		kfree(name);
>> 	}
>> 	dma_resv_unlock(dmabuf->resv);
>> 	if (ret)
>> 		return ret;
>>
>> 	spinlock(dmabuf->name_lock)
>> 	kfree(dmabuf->name);
>> 	dmabuf->name = name;
>> 	spinunlock(dmabuf->name_lock)
>>
>> 	return 0;
>> }
>>
>> M
>>
>>>
>>>>
>>>> spinlock(nam_lock)
>>>> ...
>>>>
>>>> Nesting locks  that don't need to be nested always makes me nervous
>>>> for future use that misses the lock/unlock pattern.
>>>>
>>>> However, this looks reasonable.
>>>>
>>>> With this current code, or if you update to the above pattern:
>>>>
>>>> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@...el.com>
>>>
>>> Thanks for the ACK.
>>>>
>>>> Mike
>>>>
>>>>
>>>>> out_unlock:
>>>>> 	dma_resv_unlock(dmabuf->resv);
>>>>> @@ -405,10 +407,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 +548,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
>>>
>>> --
>>> 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