linux-kernel - Re: [RFC v3 7/8] binder: use __kernel_pid_t and __kernel_uid

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]

Date:   Mon, 14 Mar 2022 16:45:28 -0700
From:   "T.J. Mercier" <tjmercier@...gle.com>
To:     Todd Kjos <tkjos@...gle.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Jonathan Corbet <corbet@....net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        Hridya Valsaraju <hridya@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian König <christian.koenig@....com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Laura Abbott <labbott@...hat.com>,
        Brian Starkey <Brian.Starkey@....com>,
        John Stultz <john.stultz@...aro.org>,
        Tejun Heo <tj@...nel.org>, Zefan Li <lizefan.x@...edance.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Shuah Khan <shuah@...nel.org>,
        Kalesh Singh <kaleshsingh@...gle.com>, Kenny.Ho@....com,
        dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linaro-mm-sig@...ts.linaro.org, cgroups@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [RFC v3 7/8] binder: use __kernel_pid_t and __kernel_uid_t for userspace

On Thu, Mar 10, 2022 at 11:33 AM Todd Kjos <tkjos@...gle.com> wrote:
>
> On Wed, Mar 9, 2022 at 8:52 AM T.J. Mercier <tjmercier@...gle.com> wrote:
> >
> > The kernel interface should use types that the kernel defines instead of
> > pid_t and uid_t, whose definiton is owned by libc. This fixes the header
> > so that it can be included without first including sys/types.h.
> >
> > Signed-off-by: T.J. Mercier <tjmercier@...gle.com>
> > ---
> >  include/uapi/linux/android/binder.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/android/binder.h
> > index 169fd5069a1a..aa28454dbca3 100644
> > --- a/include/uapi/linux/android/binder.h
> > +++ b/include/uapi/linux/android/binder.h
> > @@ -289,8 +289,8 @@ struct binder_transaction_data {
> >
> >         /* General information about the transaction. */
> >         __u32           flags;
> > -       pid_t           sender_pid;
> > -       uid_t           sender_euid;
> > +       __kernel_pid_t  sender_pid;
> > +       __kernel_uid_t  sender_euid;
>
> Are we guaranteed that this does not affect the UAPI at all? Userspace
> code using this definition will have to run with kernels using the old
> definition and visa-versa.

A standards compliant userspace should be expecting a signed integer
type here. So the only way I can think userspace would be affected is
if:
1) pid_t is a long AND
2) sizeof(long) > sizeof(int) AND
3) Consumers of the pid_t definition actually attempt to mutate the
result to make use of extra bits in the variable (which are not there)

This seems extremely unlikely. For instance just on the topic of the
first item, all of the C library implementations with pid_t
definitions linked here use an int, except for Bionic which typdefs
pid_t to __kernel_pid_t and Sortix which uses long.
https://wiki.osdev.org/C_Library

However I would argue this is already broken and should count as a bug
fix since I can't do this:

$ cat binder_include.c ; gcc binder_include.c
#include <linux/android/binder.h>
int main() {}
In file included from binder_include.c:1:
/usr/include/linux/android/binder.h:291:9: error: unknown type name ‘pid_t’
  291 |         pid_t           sender_pid;
      |         ^~~~~
/usr/include/linux/android/binder.h:292:9: error: unknown type name ‘uid_t’
  292 |         uid_t           sender_euid;
      |         ^~~~~

This is also the only occurrence of pid_t in all of
include/uapi/linux. All 40+ other uses are __kernel_pid_t, and I don't
see why the binder header should be different.


>
> >         binder_size_t   data_size;      /* number of bytes of data */
> >         binder_size_t   offsets_size;   /* number of bytes of offsets */
> >
> > --
> > 2.35.1.616.g0bdcbb4464-goog
> >