linux-kernel - Re: build_path_from_dentry_optional_prefix() may schedule from invalid context

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]

Message-ID: <20190921223847.GB29065@ZenIV.linux.org.uk>
Date:   Sat, 21 Sep 2019 23:38:47 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Pavel Shilovsky <piastryyy@...il.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Steve French <stfrench@...rosoft.com>,
        Ronnie Sahlberg <lsahlber@...hat.com>,
        linux-cifs <linux-cifs@...r.kernel.org>,
        Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: build_path_from_dentry_optional_prefix() may schedule from
 invalid context

On Thu, Sep 19, 2019 at 05:11:54PM -0700, Pavel Shilovsky wrote:

> Good catch. I think we should have another version of
> build_path_from_dentry() which takes pre-allocated (probably on stack)
> full_path as an argument. This would allow us to avoid allocations
> under the spin lock.

On _stack_?  For relative pathname?  Er...  You do realize that
kernel stack is small, right?  And said relative pathname can
bloody well be up to 4Kb (i.e. the half of said stack already,
on top of whatever the call chain has already eaten up)...

BTW, looking at build_path_from_dentry()...  WTF is this?
                temp = temp->d_parent;
                if (temp == NULL) {
                        cifs_dbg(VFS, "corrupt dentry\n");
                        rcu_read_unlock();
                        return NULL;
                }
Why not check for any number of other forms of memory corruption?
Like, say it, if (temp == (void *)0xf0adf0adf0adf0ad)?

IOW, kindly lose that nonsense.  More importantly, why bother
with that kmalloc()?  Just __getname() in the very beginning
and __putname() on failure (and for freeing the result afterwards).

What's more, you are open-coding dentry_path_raw(), badly.
The only differences are
	* use of dirsep instead of '/' and
	* a prefix slapped in the beginning.

I'm fairly sure that
	char *buf = __getname();
	char *s;

	*to_free = NULL;
	if (unlikely(!buf))
		return NULL;

	s = dentry_path_raw(dentry, buf, PATH_MAX);
	if (IS_ERR(s) || s < buf + prefix_len)
		__putname(buf);
		return NULL; // assuming that you don't care about details
	}

	if (dirsep != '/') {
		char *p = s;
		while ((p = strchr(p, '/')) != NULL)
			*p++ = dirsep;
	}

	s -= prefix_len;
	memcpy(s, prefix, prefix_len);
	
	*to_free = buf;
	return s;

would end up being faster, not to mention much easier to understand.
With the caller expected to pass &to_free among the arguments and
__putname() it once it's done.

Or just do __getname() in the caller and pass it to the function -
in that case freeing (in all cases) would be up to the caller.