Skip to content

Commit

Permalink
Userspace can pass zero length segments via writev/readv
Browse files Browse the repository at this point in the history 
Userspace can trigger an assertion by passing a zero-length segment when
assertions are enabled:

[27961.614792] VERIFY3(skip < iov->iov_len) failed (0 < 0)
[27961.614795] PANIC at zfs_uio.c:187:uio_prefaultpages()
[27961.614796] Showing stack for process 99921
[27961.614798] CPU: 7 PID: 99921 Comm: cmake Tainted: P           OE   4.1.3 #2
[27961.614799] Hardware name: Supermicro X10SAE/X10SAE, BIOS 3.0 05/20/2015
[27961.614800]  00000000000000bb ffff8804346838c8 ffffffff815e7d05 0000000000000007
[27961.614802]  ffffffffa0a01451 ffff8804346838d8 ffffffffa07f96b4 ffff880434683a68
[27961.614803]  ffffffffa07f977b ffff880426e2c600 ffff880426e2c6d0 ffff880400000030
[27961.614805] Call Trace:
[27961.614811]  [<ffffffff815e7d05>] dump_stack+0x45/0x57
[27961.614830]  [<ffffffffa07f96b4>] spl_dumpstack+0x44/0x50 [spl]
[27961.614834]  [<ffffffffa07f977b>] spl_panic+0xbb/0x100 [spl]
[27961.614865]  [<ffffffffa0b578cc>] ? zap_add+0xec/0x1a0 [zfs]
[27961.614886]  [<ffffffffa0b628ca>] ? zfs_link_create+0x38a/0x600 [zfs]
[27961.614888]  [<ffffffff815eb516>] ? mutex_lock+0x16/0x40
[27961.614904]  [<ffffffffa0b15f61>] ? refcount_add_many+0xa1/0x140 [zfs]
[27961.614908]  [<ffffffffa09fdb14>] uio_prefaultpages+0x134/0x140 [zcommon]
[27961.614930]  [<ffffffffa0b807bd>] zfs_write+0x1fd/0xe80 [zfs]
[27961.614934]  [<ffffffff8124db2d>] ? security_transition_sid+0x2d/0x40
[27961.614950]  [<ffffffffa0b17120>] ? rrm_exit+0x50/0xa0 [zfs]
[27961.614951]  [<ffffffff815eb516>] ? mutex_lock+0x16/0x40
[27961.614967]  [<ffffffffa0b17120>] ? rrm_exit+0x50/0xa0 [zfs]
[27961.614988]  [<ffffffffa0b79276>] ? zfs_open+0x86/0x120 [zfs]
[27961.614990]  [<ffffffff811c5d87>] ? inode_to_bdi+0x27/0x60
[27961.614992]  [<ffffffff811552e9>] ? file_ra_state_init+0x19/0x30
[27961.615014]  [<ffffffffa0ba004f>] zpl_write_common_iovec+0x7f/0x110 [zfs]
[27961.615035]  [<ffffffffa0ba0f00>] zpl_iter_write+0xa0/0xd0 [zfs]
[27961.615037]  [<ffffffff8119b8b9>] do_iter_readv_writev+0x59/0x80
[27961.615061]  [<ffffffffa0ba02f0>] ? zpl_read+0xa0/0xa0 [zfs]
[27961.615063]  [<ffffffff8119bf8b>] do_readv_writev+0x11b/0x260
[27961.615093]  [<ffffffffa0ba0e60>] ? zpl_compat_ioctl+0x10/0x10 [zfs]
[27961.615096]  [<ffffffff811aba1f>] ? putname+0x6f/0x80
[27961.615098]  [<ffffffff8119c159>] vfs_writev+0x39/0x50
[27961.615100]  [<ffffffff8119ce9a>] SyS_writev+0x4a/0xe0
[27961.615103]  [<ffffffff815ed75b>] system_call_fastpath+0x16/0x6e

The solution is to delete the assertion. This could potentially occur in
uiomove as well, which contains analogous assertions that appear
similarly unnecessary, so we remove those as well.

Closes openzfs#3792

Reported-by: Jonathan Vasquez <jvasquez1011@gmail.com>
Signed-off-by: Richard Yao <ryao@gentoo.org>
  • Loading branch information
@ryao
ryao committed Sep 21, 2015
1 parent e7a0518 commit 6edacba
Showing 1 changed file with 0 additions and 5 deletions.
5 changes: 0 additions & 5 deletions module/zcommon/zfs_uio.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ uiomove_iov(void *p, size_t n, enum uio_rw rw, struct uio *uio)
size_t skip = uio->uio_skip;
ulong_t cnt;

ASSERT3U(skip, <, iov->iov_len);

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Sep 22, 2015

Alternately, these could be changed to ASSERT3U(skip, <=, iov->iov_len); but I'm OK with dropping them too.

This comment has been minimized.

Sign in to view

Copy link
@ryao

ryao Sep 23, 2015

Author Owner

@behlendorf Good point, but after thinking about it, I think neither is actually correct. Since we are processing iovecs from userland, it is possible for userland to pass segments such that ASSERT3U(skip, <=, iov->iov_len) is triggered. Our setting skip = 0; would then result in us under skipping. Consequently, the solution is not only to drop the assertion, but to also modify the skip update to handle this case. This is a problem for both uiomove and uio_prefaultpages, but it is only serious in the former.

Let me know if you concur. This is an area where we should have at least two people scrutinize the code for correctness. It is easy to make mistakes when reasoning about control flow of copy operations on segments.

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Sep 25, 2015

@ryao the patch as is stands looks reasonable and safe to me, and I'm OK with merging it. But I also think that adding an early return to the function to handle this exact case would improve readability but isn't critical. Please let me know if you have any additional concerns, if not I'll get this merged as is.

while (n && uio->uio_resid) {
cnt = MIN(iov->iov_len - skip, n);
switch (uio->uio_segflg) {
Expand Down Expand Up @@ -114,8 +112,6 @@ uiomove_bvec(void *p, size_t n, enum uio_rw rw, struct uio *uio)
size_t skip = uio->uio_skip;
ulong_t cnt;

ASSERT3U(skip, <, bv->bv_len);

while (n && uio->uio_resid) {
void *paddr;
cnt = MIN(bv->bv_len - skip, n);
Expand Down Expand Up @@ -184,7 +180,6 @@ uio_prefaultpages(ssize_t n, struct uio *uio)

iov = uio->uio_iov;
iovcnt = uio->uio_iovcnt;
ASSERT3U(skip, <, iov->iov_len);

while ((n > 0) && (iovcnt > 0)) {
cnt = MIN(iov->iov_len - skip, n);
Expand Down

0 comments on commit 6edacba

Please sign in to comment.