linux-kernel - Re: [PATCH 5.13 089/151] ovl: fix deadlock in splice write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOssrKe5S4AGUD+TUT2AkeWo-7Cjfbw4vX-d3bPi6xfNOVY-CA@mail.gmail.com>
Date:   Mon, 15 Nov 2021 14:54:35 +0100
From:   Miklos Szeredi <mszeredi@...hat.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>, Sasha Levin <sashal@...nel.org>,
        syzbot <syzbot+579885d1a9a833336209@...kaller.appspotmail.com>
Subject: Re: [PATCH 5.13 089/151] ovl: fix deadlock in splice write

Hi Greg,

Looks like upstream commit b91b6b019fd ("ovl: fix deadlock in splice
write") needs to be added to linux-5.4.y.

The reason is that commit 82a763e61e2b ("ovl: simplify file splice")
was backported to v5.4.155, and the above commit fixes this.

Applies cleanly and I reviewed that the backport is correct.

Thanks,
Miklos

On Mon, Aug 16, 2021 at 3:13 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> From: Miklos Szeredi <mszeredi@...hat.com>
>
> [ Upstream commit 9b91b6b019fda817eb52f728eb9c79b3579760bc ]
>
> There's possibility of an ABBA deadlock in case of a splice write to an
> overlayfs file and a concurrent splice write to a corresponding real file.
>
> The call chain for splice to an overlay file:
>
>  -> do_splice                     [takes sb_writers on overlay file]
>    -> do_splice_from
>      -> iter_file_splice_write    [takes pipe->mutex]
>        -> vfs_iter_write
>          ...
>          -> ovl_write_iter        [takes sb_writers on real file]
>
> And the call chain for splice to a real file:
>
>  -> do_splice                     [takes sb_writers on real file]
>    -> do_splice_from
>      -> iter_file_splice_write    [takes pipe->mutex]
>
> Syzbot successfully bisected this to commit 82a763e61e2b ("ovl: simplify
> file splice").
>
> Fix by reverting the write part of the above commit and by adding missing
> bits from ovl_write_iter() into ovl_splice_write().
>
> Fixes: 82a763e61e2b ("ovl: simplify file splice")
> Reported-and-tested-by: syzbot+579885d1a9a833336209@...kaller.appspotmail.com
> Signed-off-by: Miklos Szeredi <mszeredi@...hat.com>
> Signed-off-by: Sasha Levin <sashal@...nel.org>
> ---
>  fs/overlayfs/file.c | 47 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 4d53d3b7e5fe..d081faa55e83 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -392,6 +392,51 @@ out_unlock:
>         return ret;
>  }
>
> +/*
> + * Calling iter_file_splice_write() directly from overlay's f_op may deadlock
> + * due to lock order inversion between pipe->mutex in iter_file_splice_write()
> + * and file_start_write(real.file) in ovl_write_iter().
> + *
> + * So do everything ovl_write_iter() does and call iter_file_splice_write() on
> + * the real file.
> + */
> +static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
> +                               loff_t *ppos, size_t len, unsigned int flags)
> +{
> +       struct fd real;
> +       const struct cred *old_cred;
> +       struct inode *inode = file_inode(out);
> +       struct inode *realinode = ovl_inode_real(inode);
> +       ssize_t ret;
> +
> +       inode_lock(inode);
> +       /* Update mode */
> +       ovl_copyattr(realinode, inode);
> +       ret = file_remove_privs(out);
> +       if (ret)
> +               goto out_unlock;
> +
> +       ret = ovl_real_fdget(out, &real);
> +       if (ret)
> +               goto out_unlock;
> +
> +       old_cred = ovl_override_creds(inode->i_sb);
> +       file_start_write(real.file);
> +
> +       ret = iter_file_splice_write(pipe, real.file, ppos, len, flags);
> +
> +       file_end_write(real.file);
> +       /* Update size */
> +       ovl_copyattr(realinode, inode);
> +       revert_creds(old_cred);
> +       fdput(real);
> +
> +out_unlock:
> +       inode_unlock(inode);
> +
> +       return ret;
> +}
> +
>  static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  {
>         struct fd real;
> @@ -603,7 +648,7 @@ const struct file_operations ovl_file_operations = {
>         .fadvise        = ovl_fadvise,
>         .flush          = ovl_flush,
>         .splice_read    = generic_file_splice_read,
> -       .splice_write   = iter_file_splice_write,
> +       .splice_write   = ovl_splice_write,
>
>         .copy_file_range        = ovl_copy_file_range,
>         .remap_file_range       = ovl_remap_file_range,
> --
> 2.30.2
>
>
>