Re: updated orangefs tree at kernel.org

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

On Fri, Oct 09, 2015 at 12:03:33AM +0100, Al Viro wrote:

> And I haven't looked at the rest yet - some bugs I've mentioned are definitely
> still there (e.g. bogus iput() on failure of d_make_root() - it's already
> done by d_make_root() itself in that case, so what you are doing is double
> iput(); just remove it and be done with that), but I hadn't checked the
> whole list.  Will post more later tonight...

Directories:

1) I don't understand what happens to position in directory (ctx->pos).
AFAICS, you are reading directories in moderate chunks (up to 96
entries?) and use a token stored in *(file->private_data) to tell which
chunk it is.  So far, so good, but...  AFAICS ctx->pos is reset to 0
on the beginning of each such chunk.  Am I right assuming that looping
through a directory with readdir(3) and watching for return values of
telldir(3) will yield repeating offsets every time you go into the next
chunk?

2) More seriously, rewinddir(3) *must* rewind the directory.  With all
Linux libc variants it means lseek() to 0; AFAICS, orangefs directories
won't do it properly - all lseek attempts flat-out fail with ESPIPE.
Userland won't be happy...

3) ->pvfs_dirent_outcount is u32 and it comes from server with no validation
attempts ever made.  Now, look at
        readdir->dirent_array = kmalloc(readdir->pvfs_dirent_outcount *
                                        sizeof(*readdir->dirent_array),
in decode_dirents().  ->dirent_array is a structure consisting of pointer,
int and a 16-byte 64bit-aligned array.  IOW, it's size is greater than 1
and this product might wrap around.  You really should use kcalloc() there.

What's more, the loop parsing the response body is 
        for (i = 0; i < readdir->pvfs_dirent_outcount; i++) {
                dec_string(pptr, &readdir->dirent_array[i].d_name,
                           &readdir->dirent_array[i].d_length);
                readdir->dirent_array[i].khandle =
                        *(struct pvfs2_khandle *) *pptr;
                *pptr += 16;
        }
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.

4)
	if (pos == 0) {
                ino = get_ino_from_khandle(dentry->d_inode);
                gossip_debug(GOSSIP_DIR_DEBUG,
                             "%s: calling dir_emit of \".\" with pos = %llu\n",
                             __func__,
                             llu(pos));
                ret = dir_emit(ctx, ".", 1, ino, DT_DIR);
                pos += 1;
        }
in pvfs2_readdir() is bloody odd.  What's wrong with emit_dot(), or,
for that matter, dir_emit_dots() to cover both that and .. right after
it.  You *are* setting ->i_ino to the same hash of khandle you are
using here anyway, so why not use the standard helpers?