linux-kernel - Re: [PATCH 7/8] cachefiles: Fix NULL pointer dereference in object->file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <94004b36-01ae-4c62-ad74-0bad5992eb7c@huawei.com>
Date: Fri, 11 Oct 2024 09:31:53 +0800
From: Zizhi Wo <wozizhi@...wei.com>
To: David Howells <dhowells@...hat.com>
CC: <netfs@...ts.linux.dev>, <jlayton@...nel.org>,
	<hsiangkao@...ux.alibaba.com>, <jefflexu@...ux.alibaba.com>,
	<zhujia.zj@...edance.com>, <linux-erofs@...ts.ozlabs.org>,
	<linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<libaokun1@...wei.com>, <yangerkun@...wei.com>, <houtao1@...wei.com>,
	<yukuai3@...wei.com>
Subject: Re: [PATCH 7/8] cachefiles: Fix NULL pointer dereference in
 object->file



在 2024/10/10 22:52, David Howells 写道:
> Zizhi Wo <wozizhi@...wei.com> wrote:
> 
>> 在 2024/10/10 19:26, David Howells 写道:
>>> Zizhi Wo <wozizhi@...wei.com> wrote:
>>>
>>>> +	spin_lock(&object->lock);
>>>>    	if (object->file) {
>>>>    		fput(object->file);
>>>>    		object->file = NULL;
>>>>    	}
>>>> +	spin_unlock(&object->lock);
>>> I would suggest stashing the file pointer in a local var and then doing the
>>> fput() outside of the locks.
>>> David
>>>
>>
>> If fput() is executed outside the lock, I am currently unsure how to
>> guarantee that file in __cachefiles_write() does not trigger null
>> pointer dereference...
> 
> I'm not sure why there's a problem here.  I was thinking along the lines of:
> 
> 	struct file *tmp;
> 	spin_lock(&object->lock);
>   	tmp = object->file)
> 	object->file = NULL;
> 	spin_unlock(&object->lock);
> 	if (tmp)
> 		fput(tmp);
> 
> Note that fput() may defer the actual work if the counter hits zero, so the
> cleanup may not happen inside the lock; further, the cleanup done by __fput()
> may sleep.
> 
> David
> 
> 
Oh, I see what you mean. I will sort it out and issue the second patch
as soon as possible.

Thanks,
Zizhi Wo