Re: [PATCH] dcache: fix dpath buffer corruption for too small buffers

[Imre Deak]

On Sun, 2014-03-23 at 04:36 +0000, Al Viro wrote:
> On Thu, Mar 13, 2014 at 01:29:46AM +0200, Imre Deak wrote:
> > During dentry path lookups we can end up corrupting memory if the
> > destination path buffer is too small. This is because prepend_path()
> > and prepend() adjust the passed buffer length unconditionally, allowing
> > for the buffer length to go negative. Then a later prepend_name() call
> > will receive a negative length and convert this to unsigned before
> > comparing it against the source string length leading to a possible
> > memory corruption preceeding the destination buffer.
> > 
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 265e0ce..4015fd9 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2833,7 +2833,8 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
> >  	u32 dlen = ACCESS_ONCE(name->len);
> >  	char *p;
> >  
> > -	if (*buflen < dlen + 1)
> > +	/* make sure we don't convert a negative value to unsigned int */
> > +	if (*buflen < 0 || *buflen < dlen + 1)
> >  		return -ENAMETOOLONG;
> >  	*buflen -= dlen + 1;
> 
> It's much easier to fix, actually.  Look at the callers of prepend_name();
> when it returns a negative value, they either discard the value left in
> *buflen (__dentry_path()) or pass it back to their caller and return a
> negative value themselves (prepend_path()).  The same is true for
> prepend_path() (discared in __d_path(), d_absolute_path(), sys_getcwd();
> passed to caller with negative return value in path_with_deleted()) and
> path_with_deleted() (the only caller is d_path() and it always discards).
> 
> In other words, when prepend_name() returns -ENAMETOOLONG, it is free to
> leave whatever it wants in *buflen.  So let's do what prepend() does -
> subtract from *buflen, then check if the result has become negative.
> Generates a better code than the original, actually...

Thanks for the review.

I was hesitating between the two solutions and picked the above one out
of paranoia, in case dlen > INT_MAX. prepend() accepts an int namelen,
so it doesn't have this issue. But I realize such big qstrs should be
guarded against at some higher level already and your solution makes
things more unified with the rest of the helpers.

I'll follow up with a v2 per your suggestion.

--Imre

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