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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Thu, 5 Sep 2013 21:04:01 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Waiman Long <Waiman.Long@...com>
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 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.

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.
--
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/