Re: [PATCH v2] vfs: avoid recopying file names in getname_flags

[Boqun Feng]

On Sun, Apr 12, 2015 at 02:13:18AM +0100, Al Viro wrote:
> 
> 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...

Yeah.. and it's suprising to see it doesn't make any trouble yet.

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

I cscoped the kernel code and find 15 __getname() callers, they use the
memory that __getname()s return in quite different ways.

But at least we can divide them into two groups, 1) fill the memory with
names from user space 2) fill the memory with names from kernel space.

For 1) we can use getname() to do the job and for 2) I think first we
need to figure how they are using the memory, because they may generate
names in different ways, and clean them one by one if they need to be.

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

Thank you for your response. As long as the actual size of result is not
a power of 2, the problem will not happen.(Maybe add a comment before
struct filename)


Regards,
Boqun Feng

Attachment: pgp5UC1D5eg3C.pgp
Description: PGP signature