[3.11.y.z extended stable] Patch "vfs: fix subtle use-after-free of pipe_inode_info" has been added to staging queue

[Keng-Yu Lin]

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