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)... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
