d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix mutual exclusion in pipe_direct_write().

Browse Source 
We use PIPE_DIRECTW as a semaphore for direct writes to a pipe, where
the reader copies data directly from pages mapped into the writer.
However, when a reader finishes such a copy, it previously cleared
PIPE_DIRECTW, allowing multiple writers to race and corrupt the state
used to track wired pages belonging to the writer.

Fix this by having the writer clear PIPE_DIRECTW and instead use the
count of unread bytes to determine whether a write is finished.

Reported by:	syzbot+21811cc0a89b2a87a9e7@syzkaller.appspotmail.com
Reviewed by:	kib, mjg
Tested by:	pho
MFC after:	1 week
Sponsored by:	The FreeBSD Foundation
Differential Revision:	https://reviews.freebsd.org/D20784
This commit is contained in:
Mark Johnston 2019-06-29 16:05:52 +00:00
parent 9725e74c7b
commit 02476c44c5
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/head/; revision=349546
1 changed files with 17 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
sys/kern/sys_pipe.c
View File

@ -724,7 +724,7 @@ pipe_read(struct file *fp, struct uio *uio, struct ucred *active_cred,
rpipe->pipe_map.pos += size; rpipe->pipe_map.pos += size;
rpipe->pipe_map.cnt -= size; rpipe->pipe_map.cnt -= size;
if (rpipe->pipe_map.cnt == 0) { if (rpipe->pipe_map.cnt == 0) {
rpipe->pipe_state &= ~(PIPE_DIRECTW|PIPE_WANTW); rpipe->pipe_state &= ~PIPE_WANTW;
wakeup(rpipe); wakeup(rpipe);
} }
#endif #endif
@ -855,13 +855,17 @@ pipe_build_write_buffer(struct pipe *wpipe, struct uio *uio)
} }
/* /*
* unmap and unwire the process buffer * Unwire the process buffer.
*/ */
static void static void
pipe_destroy_write_buffer(struct pipe *wpipe) pipe_destroy_write_buffer(struct pipe *wpipe)
{ {
PIPE_LOCK_ASSERT(wpipe, MA_OWNED); PIPE_LOCK_ASSERT(wpipe, MA_OWNED);
KASSERT((wpipe->pipe_state & PIPE_DIRECTW) != 0,
("%s: PIPE_DIRECTW not set on %p", __func__, wpipe));
wpipe->pipe_state &= ~PIPE_DIRECTW;
vm_page_unhold_pages(wpipe->pipe_map.ms, wpipe->pipe_map.npages); vm_page_unhold_pages(wpipe->pipe_map.ms, wpipe->pipe_map.npages);
wpipe->pipe_map.npages = 0; wpipe->pipe_map.npages = 0;
} }
@ -880,13 +884,15 @@ pipe_clone_write_buffer(struct pipe *wpipe)
int pos; int pos;
PIPE_LOCK_ASSERT(wpipe, MA_OWNED); PIPE_LOCK_ASSERT(wpipe, MA_OWNED);
KASSERT((wpipe->pipe_state & PIPE_DIRECTW) != 0,
("%s: PIPE_DIRECTW not set on %p", __func__, wpipe));
size = wpipe->pipe_map.cnt; size = wpipe->pipe_map.cnt;
pos = wpipe->pipe_map.pos; pos = wpipe->pipe_map.pos;
wpipe->pipe_buffer.in = size; wpipe->pipe_buffer.in = size;
wpipe->pipe_buffer.out = 0; wpipe->pipe_buffer.out = 0;
wpipe->pipe_buffer.cnt = size; wpipe->pipe_buffer.cnt = size;
wpipe->pipe_state &= ~PIPE_DIRECTW;
PIPE_UNLOCK(wpipe); PIPE_UNLOCK(wpipe);
iov.iov_base = wpipe->pipe_buffer.buffer; iov.iov_base = wpipe->pipe_buffer.buffer;
@ -925,7 +931,7 @@ pipe_direct_write(struct pipe *wpipe, struct uio *uio)
pipeunlock(wpipe); pipeunlock(wpipe);
goto error1; goto error1;
} }
while (wpipe->pipe_state & PIPE_DIRECTW) { if (wpipe->pipe_state & PIPE_DIRECTW) {
if (wpipe->pipe_state & PIPE_WANTR) { if (wpipe->pipe_state & PIPE_WANTR) {
wpipe->pipe_state &= ~PIPE_WANTR; wpipe->pipe_state &= ~PIPE_WANTR;
wakeup(wpipe); wakeup(wpipe);
@ -968,8 +974,7 @@ pipe_direct_write(struct pipe *wpipe, struct uio *uio)
goto error1; goto error1;
} }
error = 0; while (wpipe->pipe_map.cnt != 0) {
while (!error && (wpipe->pipe_state & PIPE_DIRECTW)) {
if (wpipe->pipe_state & PIPE_EOF) { if (wpipe->pipe_state & PIPE_EOF) {
pipe_destroy_write_buffer(wpipe); pipe_destroy_write_buffer(wpipe);
pipeselwakeup(wpipe); pipeselwakeup(wpipe);
@ -987,20 +992,19 @@ pipe_direct_write(struct pipe *wpipe, struct uio *uio)
error = msleep(wpipe, PIPE_MTX(wpipe), PRIBIO | PCATCH, error = msleep(wpipe, PIPE_MTX(wpipe), PRIBIO | PCATCH,
"pipdwt", 0); "pipdwt", 0);
pipelock(wpipe, 0); pipelock(wpipe, 0);
if (error != 0)
break;
} }
if (wpipe->pipe_state & PIPE_EOF) if (wpipe->pipe_state & PIPE_EOF)
error = EPIPE; error = EPIPE;
if (wpipe->pipe_state & PIPE_DIRECTW) { if (error == EINTR || error == ERESTART)
/*
* this bit of trickery substitutes a kernel buffer for
* the process that might be going away.
*/
pipe_clone_write_buffer(wpipe); pipe_clone_write_buffer(wpipe);
} else { else
pipe_destroy_write_buffer(wpipe); pipe_destroy_write_buffer(wpipe);
}
pipeunlock(wpipe); pipeunlock(wpipe);
KASSERT((wpipe->pipe_state & PIPE_DIRECTW) == 0,
("pipe %p leaked PIPE_DIRECTW", wpipe));
return (error); return (error);
error1: error1: