From: Jeff Moyer <jmoyer@redhat.com> Date: Mon, 26 Oct 2009 17:00:49 -0400 Subject: [fs] pipe.c null pointer dereference Message-id: x4963a1vpj2.fsf@segfault.boston.devel.redhat.com O-Subject: [kernel team] [rhel 5 patch, CVE-2009-3547] fs: pipe.c null pointer dereference Bugzilla: 530939 RH-Acked-by: Jeff Layton <jlayton@redhat.com> RH-Acked-by: Danny Feng <dfeng@redhat.com> RH-Acked-by: David Howells <dhowells@redhat.com> RH-Acked-by: Cong Wang <amwang@redhat.com> CVE: CVE-2009-3547 Hi, This bug was reported upstream and fixed by the following commit: ad3960243e55320d74195fb85c975e0a8cc4466c Americo Wang summed up the problem thusly: "So, if I understand you correctly, you mean we have a small window between calling sys_open() and fifo_open(), during this little period, we don't have i_mutex held, thun another process have a chance to release that pipe and make i_pipe NULL. Right?" [1] There is a reproducer in the upstream posting which the reporter used to verify that the patch solved the issue. The upstream patch applied directly to RHEL 5. I have built the kernel, but have not yet even booted it. I was asked to post this for review to get into a kernel respin tomorrow. I plan to test this tomorrow. For now, comments would be greatly appreciated. Cheers, Jeff [1] http://lkml.org/lkml/2009/10/21/42 diff --git a/fs/pipe.c b/fs/pipe.c index 717a9bb..5116022 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -697,36 +697,55 @@ pipe_rdwr_release(struct inode *inode, struct file *filp) static int pipe_read_open(struct inode *inode, struct file *filp) { - /* We could have perhaps used atomic_t, but this and friends - below are the only places. So it doesn't seem worthwhile. */ + int ret = -ENOENT; + mutex_lock(&inode->i_mutex); - inode->i_pipe->readers++; + + if (inode->i_pipe) { + ret = 0; + inode->i_pipe->readers++; + } + mutex_unlock(&inode->i_mutex); - return 0; + return ret; } static int pipe_write_open(struct inode *inode, struct file *filp) { + int ret = -ENOENT; + mutex_lock(&inode->i_mutex); - inode->i_pipe->writers++; + + if (inode->i_pipe) { + ret = 0; + inode->i_pipe->writers++; + } + mutex_unlock(&inode->i_mutex); - return 0; + return ret; } static int pipe_rdwr_open(struct inode *inode, struct file *filp) { + int ret = -ENOENT; + mutex_lock(&inode->i_mutex); - if (filp->f_mode & FMODE_READ) - inode->i_pipe->readers++; - if (filp->f_mode & FMODE_WRITE) - inode->i_pipe->writers++; + + if (inode->i_pipe) { + ret = 0; + if (filp->f_mode & FMODE_READ) + inode->i_pipe->readers++; + if (filp->f_mode & FMODE_WRITE) + inode->i_pipe->writers++; + } + mutex_unlock(&inode->i_mutex); - return 0; + return ret; } /*