linux-kernel - Re: [PATCH] binder: use standard functions to allocate fds

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] [day] [month] [year] [list]

Message-ID: <CAHRSSExgXbKOYR9HU=+WDaQjHBYB3=YCYXgnAOGwJ0Y3Q3qshw@mail.gmail.com>
Date:   Thu, 30 Aug 2018 08:49:40 -0700
From:   Todd Kjos <tkjos@...gle.com>
To:     hch@...radead.org
Cc:     Todd Kjos <tkjos@...roid.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Martijn Coenen <maco@...gle.com>,
        Christian Brauner <christian.brauner@...onical.com>,
        ben@...adent.org.uk, torvalds@...ux-foundation.org,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] binder: use standard functions to allocate fds

On Wed, Aug 29, 2018 at 12:00 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> >  config ANDROID_BINDER_IPC
> >       bool "Android Binder IPC Driver"
> > -     depends on MMU
> > +     depends on MMU && !CPU_CACHE_VIVT
>
> Thats is a purely arm specific symbol which should not be
> used in common code.  Nevermind that there generally should
> be no good reason for it.

It is true that the current design (10+ years old) has this flaw. VIVT
cache support was the rationale for using the non-standard functions
to allocate file descriptors from the sender context. There are no
known cases of recent android releases running on a device with
a VIVT cache architecture, but I did want to call this out.

We're working on refactoring to eliminate this issue.

>
> > +     fixup->offset = (uintptr_t)fdp - (uintptr_t)t->buffer->data;
>
> This looks completely broken.  Why would you care at what exact
> place the fd is placed?  Oh, because you share an array with fds
> with userspace, which is a hell of a bad idea, and then maninpulate
> that buffer mapped to userspace from kernel threads.

Well, android apps rely on this behavior (and have for 10+ years) so
there is no way to avoid the need to manipulate the buffer from
the kernel. We are working to refactor the driver to do this
using standard mechanisms instead of relying on low-level internal
functions.

>
> I think we just need to rm -rf drivers/android/binder*.c and be done
> with it, as this piece of crap should never have been merged to start
> with.