linux-kernel - Re: [PATCH v2] vfs: avoid recopying file names in getname

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: <20150412011318.GL889@ZenIV.linux.org.uk>
Date:	Sun, 12 Apr 2015 02:13:18 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Boqun Feng <boqun.feng@...il.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Paul Moore <pmoore@...hat.com>
Subject: Re: [PATCH v2] vfs: avoid recopying file names in getname_flags

On Sun, Apr 12, 2015 at 12:56:55AM +0100, Al Viro wrote:

> What for?  It's not as if userland memory had been communicated with by
> IP over carrier pigeons, after all, and the cost of 4Kb worth of
> (essentially) memcpy() is going to be
> 	a) incurred in extremely rare case
> and
> 	b) be dwarfed by the work we need to _do_ with what we'd copied.
> After all, that pathname is going to be parsed and traversed - all 4Kb
> worth of it.
> 
> So what's the point?

BTW, looking at the __getname() callers...  Lustre one sure as hell looks
bogus:
        char *tmp = __getname();

        if (!tmp)
                return ERR_PTR(-ENOMEM);

        len = strncpy_from_user(tmp, filename, PATH_MAX);
        if (len == 0)
                ret = -ENOENT;
        else if (len > PATH_MAX)
                ret = -ENAMETOOLONG;

        if (ret) {
                __putname(tmp);
                tmp =  ERR_PTR(ret);
        }
        return tmp;

Note that
	* strncpy_from_user(p, u, n) can return a negative (-EFAULT)
	* strncpy_from_user(p, u, n) cannot return a positive greater than
n.  In case of missing NUL among the n bytes starting at u (and no faults
accessing those) we get exactly n bytes copied and n returned.  In case
when NUL is there, we copy everything up to and including that NUL and
return number of non-NUL bytes copied.

IOW, these failure cases had never been tested.  Name being too long ends up
with non-NUL-terminated array of characters returned, and the very first
caller of ll_getname() feeds it to strlen().  Fault ends up with uninitialized
array...

AFAICS, the damn thing should just use getname() and quit reinventing the
wheel, badly.

As for your question in another thread - AFAICS, it's impossible with the
current code, but not too robust.  Fortunately, it's trivial to make
independent on allocator details - all it takes is
	result = kzalloc(offsetof(struct filename, iname[1]), GFP_KERNEL);
and we are done - result->iname+1 will be within the allocated object,
no matter what.
--
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/