Re: updated orangefs tree at kernel.org

[Al Viro <viro@ZenIV.linux.org.uk>]

On Fri, Oct 09, 2015 at 04:41:26AM +0100, Al Viro wrote:

> where dec_string(pptr, pbuf, plen) is defined as
>         __u32 len = (*(__u32 *) *(pptr)); \
>         *pbuf = *(pptr) + 4; \
>         *(pptr) += roundup8(4 + len + 1); \
>         if (plen) \
>                 *plen = len;\
> 
> Note that we do *not* validate len, so this code might end up dereferencing
> any address within +-2Gb from the buffer.  You really can't assume that
> server is neither insane nor compromised; this is a blatant security hole.
> 
> And please, drop these macros - you are not using enc_string() at all while
> dec_string() is used only in decode_dirents() and would be easier to
> understand if it had been spelled out right there.  Especially since you
> need to add sanity checks (and buggering off should they fail) to it.

Actually, validation is bloody weak in pvfs2_devreq_writev() as well.
And frankly, the layout it's expecting is downright insane.

What you have is some initial segment of
	* 32bit protocol version.  Never checked, which renders it useless.
	* 32bit magic.  That one _is_ checked, so basically they work
together as 64bit protocol version, with bloody odd validation.
	* 64bit tag, used to match with requests
	* 32bit type, apparently never checked - that of the matching request is
used
	* 32bit status
	* 64bit trailer_size, apparently used only with readdir, so
probably non-zero only if type is PVFS2_VFS_OP_READDIR or
PVFS2_VFS_OP_READDIRPLUS (the latter doesn't seem to be ever issued, though).
	* pointer-sized junk.  Ignored.
	* big fat union *NOT* including anything for readdir
possibly followed by readdir response.

To make things even funnier, you require 4- or 5-element iovec array,
the first 4 covering the aforementioned "some initial segment".  The 5th
is quietly ignored unless trailer_size is positive and status is zero.
If trailer_size > 0 && status == 0, you verify that the length of the 5th
segment is no more than trailer_size and copy it to vmalloc'ed buffer.
Without bothering to zero the rest of that buffer out.

The member of that big fat union for getattr has a bit of additional insanity -
several pointer-sized gaps.  Entirely unused.

Far be it from me to support The War On Some Drugs, but really, staying away
from the stuff that induces trips _that_ bad is just plain common sense...

First of all, this "exactly 4 or 5 iovecs in array" thing is beyond ugly - 
it's a way to separate large variable-length segment, all right, but why
the hell not just declare e.g. that if request is at least 32 bytes long
and has trailer_size > 0 && status == 0, trailer_size must be no more than
request size - 32 and that much bytes in the end will go into vmalloc'ed
buffer?  Everything else must fit into MAX_ALIGNED_DEV_REQ_DOWNSIZE.
AFAICS, that would be compatible with what your server is doing now.

Next, and that can't be fixed without a protocol change, I'd suggest losing
those pointer-sized gaps (and probably reordering struct PVFS_sys_attr_s
to make sure that all u64 are naturally aligned there).  Why make your
protocol wordsize-sensitive, when it's trivial to avoid?

In any case, the current code is asking for serious trouble if the 5th segment
is there and _shorter_ than trailer_size.  As the absolute minimum we need to
zero the rest of vmalloc'ed buffer, and I seriously suspect that we need
to outright reject such requests.  And I would really prefer to get rid of
this "exactly 4 or 5" thing as well (see above)...