Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 7 Oct 2019 23:05:35 +0200
From:   Christian Brauner <christian.brauner@ubuntu.com>
To:     Jann Horn <jannh@google.com>
Cc:     Todd Kjos <tkjos@google.com>, Martijn Coenen <maco@android.com>,
        Arve =?utf-8?B?SGrDuG5uZXbDpWc=?= <arve@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        "open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: UAF read in print_binder_transaction_log_entry() on
 ANDROID_BINDERFS kernels
Message-ID: <20191007210534.wqnizdp6pl7gn5qe@wittgenstein>
References: <CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Oct 07, 2019 at 10:49:57PM +0200, Jann Horn wrote:
> Hi!
> 
> There is a use-after-free read in print_binder_transaction_log_entry()
> on ANDROID_BINDERFS kernels because
> print_binder_transaction_log_entry() prints the char* e->context_name
> as string, and if the transaction occurred on a binder device from
> binderfs, e->context_name belongs to the binder device and is freed
> when the inode disappears.
> 
> Luckily this shouldn't have security implications, since:
> 
> a) reading the binder transaction log is already a pretty privileged operation
> b) I can't find any actual users of ANDROID_BINDERFS
> 
> I guess there are three ways to fix it:
> 1) Create a new shared global spinlock for binderfs_evict_inode() and
> binder_transaction_log_show(), and let binderfs_evict_inode() scan the
> transaction log for pointers to its name and replace them with
> pointers to a statically-allocated string "{DELETED}" or something
> like that.
> 2) Let the transaction log contain non-reusable device identifiers
> instead of name pointers, and let print_binder_transaction_log_entry()
> look them up in something like a hashtable.
> 3) Just copy the name into the transaction log every time.
> 
> I'm not sure which one is better, or whether there's a nicer fourth
> option, so I'm leaving writing a patch for this to y'all.

Moin,

Thanks for the report.
Android binderfs is enabled by default on Ubuntu and - iirc - Debian
kernels. It is actively used by Anbox to run Android in containers.

The codepath you're referring to is specific to the stats=global mount
option. This was contributed by the Android team for the 5.4 cycle (cf.
[1]). That means there is no released kernel that supports the
stats=global mount option. So all current users cannot be affected by
this bug.

I'll take a look at this tomorrow and see what makes the most sense. I
agree that this is not a security issue. Thanks for catching this early.

If you already have a script that trivially reproduces the bug it'd be
nice if you could paste it. Otherwise we can just add a reproducer based
on your snippet from below. I want to add a regression test for this.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f00834518ed3194b866f5f3d63b71e0ed7f6bc00
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e13e452dafc009049a9a5a4153e2f9e51b23915
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=03e2e07e38147917482d595ad3cf193212ded8ac
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4feb80faf428a02d407a9ea1952004af01308765

Thanks!
Christian