| Message ID | 20180717160035.9422-1-aryabinin@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/17/18 6:00 PM, Andrey Ryabinin wrote: > fuse_dev_splice_write() reads pipe->buffers to determine the size of > 'bufs' array before taking the pipe_lock(). This is not safe as > another thread might change the 'pipe->buffers' between the allocation > and taking the pipe_lock(). So we end up with too small 'bufs' array. > > Move the bufs allocations inside pipe_lock()/pipe_unlock() to fix this. > > Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device") > Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> > Cc: <stable@vger.kernel.org> BTW, why don't we need to do the same in fuse_dev_splice_read()? Thanks, Vlastimil > --- > fs/fuse/dev.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index c6b88fa85e2e..702592cce546 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -1944,12 +1944,15 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe, > if (!fud) > return -EPERM; > > + pipe_lock(pipe); > + > bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer), > GFP_KERNEL); > - if (!bufs) > + if (!bufs) { > + pipe_unlock(pipe); > return -ENOMEM; > + } > > - pipe_lock(pipe); > nbuf = 0; > rem = 0; > for (idx = 0; idx < pipe->nrbufs && rem < len; idx++) >
On 6/12/19 11:57 AM, Vlastimil Babka wrote: > On 7/17/18 6:00 PM, Andrey Ryabinin wrote: >> fuse_dev_splice_write() reads pipe->buffers to determine the size of >> 'bufs' array before taking the pipe_lock(). This is not safe as >> another thread might change the 'pipe->buffers' between the allocation >> and taking the pipe_lock(). So we end up with too small 'bufs' array. >> >> Move the bufs allocations inside pipe_lock()/pipe_unlock() to fix this. >> >> Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device") >> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> >> Cc: <stable@vger.kernel.org> > > BTW, why don't we need to do the same in fuse_dev_splice_read()? > do_splice() already takes the pipe_lock() before calling ->splice_read() > Thanks, > Vlastimil > >> --- >> fs/fuse/dev.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c >> index c6b88fa85e2e..702592cce546 100644 >> --- a/fs/fuse/dev.c >> +++ b/fs/fuse/dev.c >> @@ -1944,12 +1944,15 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe, >> if (!fud) >> return -EPERM; >> >> + pipe_lock(pipe); >> + >> bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer), >> GFP_KERNEL); >> - if (!bufs) >> + if (!bufs) { >> + pipe_unlock(pipe); >> return -ENOMEM; >> + } >> >> - pipe_lock(pipe); >> nbuf = 0; >> rem = 0; >> for (idx = 0; idx < pipe->nrbufs && rem < len; idx++) >> >
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index c6b88fa85e2e..702592cce546 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -1944,12 +1944,15 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe, if (!fud) return -EPERM; + pipe_lock(pipe); + bufs = kmalloc_array(pipe->buffers, sizeof(struct pipe_buffer), GFP_KERNEL); - if (!bufs) + if (!bufs) { + pipe_unlock(pipe); return -ENOMEM; + } - pipe_lock(pipe); nbuf = 0; rem = 0; for (idx = 0; idx < pipe->nrbufs && rem < len; idx++)
fuse_dev_splice_write() reads pipe->buffers to determine the size of 'bufs' array before taking the pipe_lock(). This is not safe as another thread might change the 'pipe->buffers' between the allocation and taking the pipe_lock(). So we end up with too small 'bufs' array. Move the bufs allocations inside pipe_lock()/pipe_unlock() to fix this. Fixes: dd3bb14f44a6 ("fuse: support splice() writing to fuse device") Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> Cc: <stable@vger.kernel.org> --- fs/fuse/dev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)