linux-kernel - Re: Kernel crash in free_pipe

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: <CA+55aFxwY6aLaKEifHwh6UZJ+6VR_XBA8B1w4SbnA+F8Cu2v6g@mail.gmail.com>
Date:   Mon, 30 Oct 2017 20:06:23 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: Kernel crash in free_pipe_info()

On Mon, Oct 30, 2017 at 7:08 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> I'm not seeing anything that makes sense. I'll have to think about this.

Al, would you mind taking a look at the error handling in create_pipe_files().

In particular, look here:

 - we start out allocating the inode with "get_pipe_inode().

   That sets up a inode->i_pipe, with pipe->files initialized to 2.
Fine. We're going to have two file descriptors.

 - we then create the dummy path:

        path.dentry = d_alloc_pseudo(pipe_mnt->mnt_sb, &empty_name);

   fine fine. Again, this looks all good for the success cases.

But the *error* cases are a bit dodgy, aren't they?

We have three different error cases:

 - we couldn't even allocate a dentry. We do

        free_pipe_info(inode->i_pipe);
        iput(inode);

 - we couldn't allocate any file at all:

        free_pipe_info(inode->i_pipe);
        path_put(&path);

 - we allocated the first file, but not the second:

        put_filp(f);
        free_pipe_info(inode->i_pipe);
        path_put(&path);

and it worries me a bit that in all those error cases, we end up doing
that "free_pipe_info()", but we basically do this half-arsed job of
freeing things.

For example, we use "put_filp()" to free the file pointer, not "fput()".

We do that "free_pipe_info(inode->i_pipe);", but we never actually
clear inode->i_pipe, so now we have an inode that looks like a pipe
inode, and has a stale pointer to a pipe_inode_info.

It all looks technically correct. It's fine to use put_filp(), because
the file pointer has never really been used. And the inode should
never get re-used anyway without going through the whole reinit in
inode_init_always().

So I don't see anything *wrong*, but I see a lot that is just unusual,
and seems to depend on half-initialized state being fine. Can you look
at this?

                Linus