linux-kernel - Re: [PATCH v2 1/1] dcache: Translating dentry into pathname without taking rename

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 05 Sep 2013 16:43:14 -0400
From:	Waiman Long <waiman.long@...com>
To:	Al Viro <viro@...IV.linux.org.uk>
CC:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Chandramouleeswaran, Aswin" <aswin@...com>,
	"Norton, Scott J" <scott.norton@...com>,
	George Spelvin <linux@...izon.com>,
	John Stoffel <john@...ffel.org>
Subject: Re: [PATCH v2 1/1] dcache: Translating dentry into pathname without
 taking rename_lock

On 09/05/2013 04:04 PM, Al Viro wrote:
> On Thu, Sep 05, 2013 at 02:55:16PM -0400, Waiman Long wrote:
>> +	const char *dname = ACCESS_ONCE(dentry->d_name.name);
>> +	u32 dlen = dentry->d_name.len;
>> +	int error;
>> +
>> +	if (likely(dname == (const char *)dentry->d_iname)) {
>> +		/*
>> +		 * Internal dname, the string memory is valid as long
>> +		 * as its length is not over the limit.
>> +		 */
>> +		if (unlikely(dlen>  sizeof(dentry->d_iname)))
>> +			return -EINVAL;
>> +	} else if (!dname)
>> +		return -EINVAL;
> Can't happen.
>> +	else {
>> +		const char *ptr;
>> +		u32 len;
>> +
>> +		/*
>> +		 * External dname, need to fetch name pointer and length
>> +		 * again under d_lock to get a consistent set and avoid
>> +		 * racing with d_move() which will take d_lock before
>> +		 * acting on the dentries.
>> +		 */
>> +		spin_lock(&dentry->d_lock);
>> +		dname = dentry->d_name.name;
>> +		dlen  = dentry->d_name.len;
>> +		spin_unlock(&dentry->d_lock);
>> +
>> +		if (unlikely(!dname || !dlen))
>> +			return -EINVAL;
> Can't happen.
>
>> +		/*
>> +		 * As the length and the content of the string may not be
>> +		 * valid, need to scan the string and return EINVAL if there
>> +		 * is embedded null byte within the length of the string.
>> +		 */
>> +		for (ptr = dname, len = dlen; len; len--, ptr++) {
>> +			if (*ptr == '\0')
>> +				return -EINVAL;
> Egads...  First of all, this is completely pointless - if you've grabbed
> ->d_name.name and ->d_name.len under ->d_lock, you don't *need* that crap.
> At all.  The whole point of that exercise is to avoid taking ->d_lock;
> _that_ is where the "read byte by byte until you hit NUL" comes from.
> And if you do that, you can bloody well just go ahead and store them in
> the target array *as* *you* *go*.  No reason to bother with memcpy()
> afterwards.

That is what I thought too. I am just not totally sure about it. So yes, 
I can scrap all these additional check.

As the internal dname buffer is at least 32 bytes, most dentries will 
use the internal buffer instead of allocating from kmem. IOW, the d_lock 
taking code path is unlikely to be used.

> Damnit, just grab len and name (no ->d_lock, etc.).  Check if you've got
> enough space in the buffer, treat "not enough" as an overflow.  Then
> proceed to copy the damn thing over there (starting at *buffer -= len)
> byte by byte, stopping when you've copied len bytes *or* when the byte you've
> got happens to be NUL.  Don't bother with EINVAL, etc. - just return to
> caller and let rename_lock logics take care of the races.  That's it - nothing
> more is needed.

OK, I will do that.

-Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/