[3.11.y.z extended stable] Patch "vfs: fix subtle use-after-free of pipe_inode_info" has been added to staging queue
Tim Gardner
tim.gardner at canonical.com
Thu Dec 12 15:04:12 UTC 2013
Kengyu - I agree that this is an important patch for 3.11 stable.
However, SRU testing for 12.04.4 is sufficiently far along that I would
not want to interrupt (and restart) testing for a symptom that requires
many hours to manifest. In other words, its not a boot essential fix. We
could certainly have a kernel waiting in -proposed by the time 12.04.4
is released.
Brad - thoughts ?
rtg
On 12/12/2013 07:29 AM, Keng-Yu Lin wrote:
> Hi there:
> By any chance can this patch make itself included in the 12.04.4
> point release?
>
> It is found that this patch fixes kernel oops occurring with the
> current Ubuntu 3.11-based kernel. (the oops happens in roughly 30 hrs
> in a stress test environment on a server machine).
>
> The patch is already in the upstream 3.10.y and 3.12.y stable trees,
> but has no change to be in the last 3.11.y release (this is 3.11.10) .
>
> cheers,
> -kengyu
>
> On Wed, Dec 11, 2013 at 10:41 PM, Luis Henriques
> <luis.henriques at canonical.com> wrote:
>> This is a note to let you know that I have just added a patch titled
>>
>> vfs: fix subtle use-after-free of pipe_inode_info
>>
>> to the linux-3.11.y-queue branch of the 3.11.y.z extended stable tree
>> which can be found at:
>>
>> http://kernel.ubuntu.com/git?p=ubuntu/linux.git;a=shortlog;h=refs/heads/linux-3.11.y-queue
>>
>> If you, or anyone else, feels it should not be added to this tree, please
>> reply to this email.
>>
>> For more information about the 3.11.y.z tree, see
>> https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable
>>
>> Thanks.
>> -Luis
>>
>> ------
>>
>> From ddd487a2f0b5b804751a2972e99071d657d8a53d Mon Sep 17 00:00:00 2001
>> From: Linus Torvalds <torvalds at linux-foundation.org>
>> Date: Mon, 2 Dec 2013 09:44:51 -0800
>> Subject: vfs: fix subtle use-after-free of pipe_inode_info
>>
>> commit b0d8d2292160bb63de1972361ebed100c64b5b37 upstream.
>>
>> The pipe code was trying (and failing) to be very careful about freeing
>> the pipe info only after the last access, with a pattern like:
>>
>> spin_lock(&inode->i_lock);
>> if (!--pipe->files) {
>> inode->i_pipe = NULL;
>> kill = 1;
>> }
>> spin_unlock(&inode->i_lock);
>> __pipe_unlock(pipe);
>> if (kill)
>> free_pipe_info(pipe);
>>
>> where the final freeing is done last.
>>
>> HOWEVER. The above is actually broken, because while the freeing is
>> done at the end, if we have two racing processes releasing the pipe
>> inode info, the one that *doesn't* free it will decrement the ->files
>> count, and unlock the inode i_lock, but then still use the
>> "pipe_inode_info" afterwards when it does the "__pipe_unlock(pipe)".
>>
>> This is *very* hard to trigger in practice, since the race window is
>> very small, and adding debug options seems to just hide it by slowing
>> things down.
>>
>> Simon originally reported this way back in July as an Oops in
>> kmem_cache_allocate due to a single bit corruption (due to the final
>> "spin_unlock(pipe->mutex.wait_lock)" incrementing a field in a different
>> allocation that had re-used the free'd pipe-info), it's taken this long
>> to figure out.
>>
>> Since the 'pipe->files' accesses aren't even protected by the pipe lock
>> (we very much use the inode lock for that), the simple solution is to
>> just drop the pipe lock early. And since there were two users of this
>> pattern, create a helper function for it.
>>
>> Introduced commit ba5bb147330a ("pipe: take allocation and freeing of
>> pipe_inode_info out of ->i_mutex").
>>
>> Reported-by: Simon Kirby <sim at hostway.ca>
>> Reported-by: Ian Applegate <ia at cloudflare.com>
>> Acked-by: Al Viro <viro at zeniv.linux.org.uk>
>> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
>> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
>> ---
>> fs/pipe.c | 39 +++++++++++++++++++--------------------
>> 1 file changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/pipe.c b/fs/pipe.c
>> index d2c45e1..0e0752e 100644
>> --- a/fs/pipe.c
>> +++ b/fs/pipe.c
>> @@ -726,11 +726,25 @@ pipe_poll(struct file *filp, poll_table *wait)
>> return mask;
>> }
>>
>> +static void put_pipe_info(struct inode *inode, struct pipe_inode_info *pipe)
>> +{
>> + int kill = 0;
>> +
>> + spin_lock(&inode->i_lock);
>> + if (!--pipe->files) {
>> + inode->i_pipe = NULL;
>> + kill = 1;
>> + }
>> + spin_unlock(&inode->i_lock);
>> +
>> + if (kill)
>> + free_pipe_info(pipe);
>> +}
>> +
>> static int
>> pipe_release(struct inode *inode, struct file *file)
>> {
>> - struct pipe_inode_info *pipe = inode->i_pipe;
>> - int kill = 0;
>> + struct pipe_inode_info *pipe = file->private_data;
>>
>> __pipe_lock(pipe);
>> if (file->f_mode & FMODE_READ)
>> @@ -743,17 +757,9 @@ pipe_release(struct inode *inode, struct file *file)
>> kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
>> kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
>> }
>> - spin_lock(&inode->i_lock);
>> - if (!--pipe->files) {
>> - inode->i_pipe = NULL;
>> - kill = 1;
>> - }
>> - spin_unlock(&inode->i_lock);
>> __pipe_unlock(pipe);
>>
>> - if (kill)
>> - free_pipe_info(pipe);
>> -
>> + put_pipe_info(inode, pipe);
>> return 0;
>> }
>>
>> @@ -1014,7 +1020,6 @@ static int fifo_open(struct inode *inode, struct file *filp)
>> {
>> struct pipe_inode_info *pipe;
>> bool is_pipe = inode->i_sb->s_magic == PIPEFS_MAGIC;
>> - int kill = 0;
>> int ret;
>>
>> filp->f_version = 0;
>> @@ -1130,15 +1135,9 @@ err_wr:
>> goto err;
>>
>> err:
>> - spin_lock(&inode->i_lock);
>> - if (!--pipe->files) {
>> - inode->i_pipe = NULL;
>> - kill = 1;
>> - }
>> - spin_unlock(&inode->i_lock);
>> __pipe_unlock(pipe);
>> - if (kill)
>> - free_pipe_info(pipe);
>> +
>> + put_pipe_info(inode, pipe);
>> return ret;
>> }
>>
>> --
>> 1.8.3.2
>>
>>
>> --
>> kernel-team mailing list
>> kernel-team at lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
--
Tim Gardner tim.gardner at canonical.com
More information about the kernel-team
mailing list