linux-kernel - Re: [PATCH v5 1/8] debugfs: prevent access to possibly dead 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: <87ziu4ak5g.fsf@gmail.com>
Date:	Fri, 11 Mar 2016 20:28:59 +0100
From:	Nicolai Stange <nicstange@...il.com>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Nicolai Stange <nicstange@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/8] debugfs: prevent access to possibly dead file_operations at file open

Rasmus Villemoes <linux@...musvillemoes.dk> writes:

> On Sun, Mar 06 2016, Nicolai Stange <nicstange@...il.com> wrote:
>
>> + *
>> + * Allow any ongoing concurrent call into debugfs_remove() or
>> + * debugfs_remove_recursive() blocked by a former call to
>> + * debugfs_use_file_start() to proceed and return to its caller.
>> + */
>> +static void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
>> +{
>> +	srcu_read_unlock(&debugfs_srcu, srcu_idx);
>> +}
>> +
>> +#define F_DENTRY(filp) ((filp)->f_path.dentry)
>> +
>> +#define REAL_FOPS_DEREF(dentry)					\
>> +	((const struct file_operations *)(dentry)->d_fsdata)
>> +
>> +static int open_proxy_open(struct inode *inode, struct file *filp)
>> +{
>> +	const struct dentry *dentry = F_DENTRY(filp);
>> +	const struct file_operations *real_fops = NULL;
>> +	int srcu_idx, r;
>> +
>> +	r = debugfs_use_file_start(dentry, &srcu_idx);
>> +	if (r) {
>> +		debugfs_use_file_finish(srcu_idx);
>> +		r = -ENOENT;
>> +		goto out;
>
> this...
>
>> +out:
>> +	fops_put(real_fops);
>> +	debugfs_use_file_finish(srcu_idx);
>
> ... and that seems like an obvious double unlock?

Yes, it is. Thank you so much for reviewing and finding this!

Since v4 of this series has been applied to the driver-core tree's
driver-core-testing branch already, I'll send a patch addressing this
issue tomorrow.

Best,

Nicolai