| Message ID | 20221117115323.1718-1-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pipe: fix potential use-after-free in pipe_read() | expand |
On Thu, Nov 17, 2022 at 07:53:23PM +0800, Zhen Lei wrote: > Accessing buf->flags after pipe_buf_release(pipe, buf) is unsafe, because > the 'buf' memory maybe freed. Huh? What are you talking about? struct pipe_buffer *buf = &pipe->bufs[tail & mask]; To free *buf you would need to free the entire damn array, which is obviously not going to be possible here; if you are talking about reuse of *buf - that's controlled by pipe->tail, and we do not assign it until later. Fetching any fields of *buf is safe; what can get freed is buf->page, not buf itself. So that buf->flags access is fine.
On 2022/11/25 14:33, Al Viro wrote: > On Thu, Nov 17, 2022 at 07:53:23PM +0800, Zhen Lei wrote: >> Accessing buf->flags after pipe_buf_release(pipe, buf) is unsafe, because >> the 'buf' memory maybe freed. > > Huh? What are you talking about? > struct pipe_buffer *buf = &pipe->bufs[tail & mask]; > To free *buf you would need to free the entire damn array, which is > obviously not going to be possible here; if you are talking about reuse > of *buf - that's controlled by pipe->tail, and we do not assign it until > later. > > Fetching any fields of *buf is safe; what can get freed is buf->page, not > buf itself. So that buf->flags access is fine. Right. Thank you for explaining clearly. Sorry, I misunderstood in the course of learning. > > . >
diff --git a/fs/pipe.c b/fs/pipe.c index 42c7ff41c2dba29..0f873949337ed28 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -321,12 +321,12 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) } if (!buf->len) { - pipe_buf_release(pipe, buf); - spin_lock_irq(&pipe->rd_wait.lock); #ifdef CONFIG_WATCH_QUEUE if (buf->flags & PIPE_BUF_FLAG_LOSS) pipe->note_loss = true; #endif + pipe_buf_release(pipe, buf); + spin_lock_irq(&pipe->rd_wait.lock); tail++; pipe->tail = tail; spin_unlock_irq(&pipe->rd_wait.lock);
Accessing buf->flags after pipe_buf_release(pipe, buf) is unsafe, because the 'buf' memory maybe freed. In fact, pipe->note_loss does not need the protection of spinlock pipe->rd_wait.lock, it only needs the protection of __pipe_lock(pipe). So make the assignment of pipe->note_loss complete before releasing 'buf' to eliminate the risk. Fixes: e7d553d69cf6 ("pipe: Add notification lossage handling") Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- fs/pipe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)