[PATCH] dmabuf: use spinlock to access dmabuf->name

Ruhl, Michael J michael.j.ruhl at intel.com
Wed Jun 17 17:43:38 UTC 2020


>-----Original Message-----
>From: charante=codeaurora.org at mg.codeaurora.org
><charante=codeaurora.org at mg.codeaurora.org> On Behalf Of Charan Teja
>Kalla
>Sent: Wednesday, June 17, 2020 2:29 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; Sumit Semwal
><sumit.semwal at linaro.org>; open list:DMA BUFFER SHARING FRAMEWORK
><linux-media at vger.kernel.org>; DRI mailing list <dri-
>devel at lists.freedesktop.org>
>Cc: Linaro MM SIG <linaro-mm-sig at lists.linaro.org>;
>vinmenon at codeaurora.org; LKML <linux-kernel at vger.kernel.org>;
>stable at vger.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 at lists.freedesktop.org> On Behalf Of
>>> Ruhl, Michael J
>>> Sent: Tuesday, June 16, 2020 9:51 AM
>>> To: Charan Teja Kalla <charante at codeaurora.org>; Sumit Semwal
>>> <sumit.semwal at linaro.org>; open list:DMA BUFFER SHARING
>FRAMEWORK
>>> <linux-media at vger.kernel.org>; DRI mailing list <dri-
>>> devel at lists.freedesktop.org>
>>> Cc: Linaro MM SIG <linaro-mm-sig at lists.linaro.org>;
>>> vinmenon at codeaurora.org; LKML <linux-kernel at vger.kernel.org>;
>>> stable at vger.kernel.org
>>> Subject: RE: [PATCH] dmabuf: use spinlock to access dmabuf->name
>>>
>>>> -----Original Message-----
>>>> From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of
>>>> Charan Teja Kalla
>>>> Sent: Thursday, June 11, 2020 9:40 AM
>>>> To: Sumit Semwal <sumit.semwal at linaro.org>; open list:DMA BUFFER
>>>> SHARING FRAMEWORK <linux-media at vger.kernel.org>; DRI mailing list
><dri-
>>>> devel at lists.freedesktop.org>
>>>> Cc: Linaro MM SIG <linaro-mm-sig at lists.linaro.org>;
>>>> vinmenon at codeaurora.org; LKML <linux-kernel at vger.kernel.org>;
>>>> stable at vger.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 at vger.kernel.org> [5.3+]
>>>> Signed-off-by: Charan Teja Reddy <charante at codeaurora.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

	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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.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


More information about the dri-devel mailing list