driverdev-devel.linuxdriverproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] binder: prevent transactions to context manager from its own process.
@ 2019-07-15 19:18 Hridya Valsaraju
  2019-07-15 20:36 ` Todd Kjos
  2019-10-11 21:59 ` Jann Horn
  0 siblings, 2 replies; 6+ messages in thread
From: Hridya Valsaraju @ 2019-07-15 19:18 UTC (permalink / raw)


Currently, a transaction to context manager from its own process
is prevented by checking if its binder_proc struct is the same as
that of the sender. However, this would not catch cases where the
process opens the binder device again and uses the new fd to send
a transaction to the context manager.

Reported-by: syzbot+8b3c354d33c4ac78bfad at syzkaller.appspotmail.com
Signed-off-by: Hridya Valsaraju <hridya at google.com>
---
 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index e4d25ebec5be..89b9cedae088 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -3138,7 +3138,7 @@ static void binder_transaction(struct binder_proc *proc,
 			else
 				return_error = BR_DEAD_REPLY;
 			mutex_unlock(&context->context_mgr_node_lock);
-			if (target_node && target_proc == proc) {
+			if (target_node && target_proc->pid == proc->pid) {
 				binder_user_error("%d:%d got transaction to context manager from process owning it\n",
 						  proc->pid, thread->pid);
 				return_error = BR_FAILED_REPLY;
-- 
2.22.0.510.g264f2c817a-goog

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] binder: prevent transactions to context manager from its own process.
  2019-07-15 19:18 [PATCH] binder: prevent transactions to context manager from its own process Hridya Valsaraju
@ 2019-07-15 20:36 ` Todd Kjos
  2019-10-11 21:59 ` Jann Horn
  1 sibling, 0 replies; 6+ messages in thread
From: Todd Kjos @ 2019-07-15 20:36 UTC (permalink / raw)
  To: Hridya Valsaraju
  Cc: open list:ANDROID DRIVERS, Android Kernel Team, Todd Kjos,
	syzbot, Greg Kroah-Hartman, LKML, Arve Hjønnevåg,
	Joel Fernandes, Martijn Coenen, Christian Brauner

On Mon, Jul 15, 2019 at 12:18 PM Hridya Valsaraju <hridya@google.com> wrote:
>
> Currently, a transaction to context manager from its own process
> is prevented by checking if its binder_proc struct is the same as
> that of the sender. However, this would not catch cases where the
> process opens the binder device again and uses the new fd to send
> a transaction to the context manager.
>
> Reported-by: syzbot+8b3c354d33c4ac78bfad@syzkaller.appspotmail.com
> Signed-off-by: Hridya Valsaraju <hridya@google.com>

Acked-by: Todd Kjos <tkjos@google.com>

> ---
>  drivers/android/binder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index e4d25ebec5be..89b9cedae088 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3138,7 +3138,7 @@ static void binder_transaction(struct binder_proc *proc,
>                         else
>                                 return_error = BR_DEAD_REPLY;
>                         mutex_unlock(&context->context_mgr_node_lock);
> -                       if (target_node && target_proc == proc) {
> +                       if (target_node && target_proc->pid == proc->pid) {
>                                 binder_user_error("%d:%d got transaction to context manager from process owning it\n",
>                                                   proc->pid, thread->pid);
>                                 return_error = BR_FAILED_REPLY;
> --
> 2.22.0.510.g264f2c817a-goog
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] binder: prevent transactions to context manager from its own process.
  2019-07-15 19:18 [PATCH] binder: prevent transactions to context manager from its own process Hridya Valsaraju
  2019-07-15 20:36 ` Todd Kjos
@ 2019-10-11 21:59 ` Jann Horn
  2019-10-11 22:11   ` Jann Horn
  1 sibling, 1 reply; 6+ messages in thread
From: Jann Horn @ 2019-10-11 21:59 UTC (permalink / raw)
  To: Hridya Valsaraju, Todd Kjos
  Cc: open list:ANDROID DRIVERS, kernel-team,
	syzbot+8b3c354d33c4ac78bfad, Greg Kroah-Hartman, kernel list,
	Arve Hjønnevåg, Joel Fernandes, Martijn Coenen,
	Christian Brauner

[-- Attachment #1: Type: text/plain, Size: 7498 bytes --]

On Mon, Jul 15, 2019 at 9:18 PM Hridya Valsaraju <hridya@google.com> wrote:
> Currently, a transaction to context manager from its own process
> is prevented by checking if its binder_proc struct is the same as
> that of the sender. However, this would not catch cases where the
> process opens the binder device again and uses the new fd to send
> a transaction to the context manager.
>
> Reported-by: syzbot+8b3c354d33c4ac78bfad@syzkaller.appspotmail.com
> Signed-off-by: Hridya Valsaraju <hridya@google.com>
> ---
>  drivers/android/binder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index e4d25ebec5be..89b9cedae088 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -3138,7 +3138,7 @@ static void binder_transaction(struct binder_proc *proc,
>                         else
>                                 return_error = BR_DEAD_REPLY;
>                         mutex_unlock(&context->context_mgr_node_lock);
> -                       if (target_node && target_proc == proc) {
> +                       if (target_node && target_proc->pid == proc->pid) {
>                                 binder_user_error("%d:%d got transaction to context manager from process owning it\n",
>                                                   proc->pid, thread->pid);
>                                 return_error = BR_FAILED_REPLY;

This isn't a valid fix.

For context, the syzkaller report at
<https://lore.kernel.org/lkml/000000000000afe2c70589526668@google.com/>
triggered this WARN_ON() in binder_transaction_buffer_release() in the
BINDER_TYPE_FD case, which Todd added in 44d8047f1d87 ("binder: use
standard functions to allocate fds"):

    case BINDER_TYPE_FD: {
        /*
         * No need to close the file here since user-space
         * closes it for for successfully delivered
         * transactions. For transactions that weren't
         * delivered, the new fd was never allocated so
         * there is no need to close and the fput on the
         * file is done when the transaction is torn
         * down.
         */
        WARN_ON(failed_at &&
            proc->tsk == current->group_leader);
    } break;

That check seems to be attempting to detect cases where
binder_transaction() fails and rolls back a partial transaction sent
by a process to itself. I think the intent there is probably to catch
cases that would cause the check in the BINDER_TYPE_FDA case below to
trip up?

About this fix: This prevents a task from sending binder transactions
to the context manager if they're running in the same process. (By the
way, I don't understand why that's a problem, conceptually.) But you
can still open a binder device twice (binder_proc instances A and B)
from a process that does not own the context manager instance, pass a
binder object from A to the context manager, let the context manager
pass it to B, and then A can transact with the same-process B. So this
merely looks fixed because syzkaller isn't able to construct such a
complicated testcase. (I think you could also let A receive a handle
to itself and then transact with itself, but I haven't tested that.)


I think this fix should probably be reverted (unless you actually want
to prevent intra-process transactions, which would probably require a
bunch of ugly extra checks), the WARN_ON() should be removed, and the
BINDER_TYPE_FDA case should be adjusted to make its decision based on
a flag passed from its parent instead of guessing based on what
`current` is. Since it looks like because of this bug, an aborted
intra-process transaction containing BINDER_TYPE_FDA (e.g. via the
err_translate_failed or err_dead_proc_or_thread cases) will cause file
descriptors to unexpectedly be released in the caller, leading to a
file-descriptor use-after-free in userspace, the fix should probably
also be stable-backported. (It's probably not a huge problem in
practice though, given that only hwbinder uses BINDER_TYPE_FDA and you
need to have an intra-process transaction at the same time as
something like a thread going away, or something like that? I don't
fully understand the failure conditions for binder transactions.)


Here's a reproducer for triggering the WARN_ON() on git master. The
helper files binder.c and binder.h are attached.

=================
#define _GNU_SOURCE
#include <unistd.h>
#include <stdio.h>
#include <stdint.h>
#include <err.h>
#include <stdlib.h>
#include <sys/signal.h>
#include <sys/prctl.h>
#include "binder.h"

#define BINDER_PATH "/dev/binder/binder"

static void do_exit(int dummy) {
  _exit(1);
}

static uint32_t ref_a_from_manager;

int my_handler(struct binder_state *bs, struct binder_transaction_data *txn,
               struct binder_io *msg, struct binder_io *reply) {
  if (txn->code == 1) {
    ref_a_from_manager = bio_get_ref(msg);
    if (ref_a_from_manager == 0)
      errx(1, "manager received bogus message 1");
    binder_acquire(bs, ref_a_from_manager);
    printf("manager received handle 0x%x from A\n", ref_a_from_manager);
    return 0;
  } else if (txn->code == 2) {
    if (ref_a_from_manager == 0)
      errx(1, "B asked too early");
    bio_put_ref(reply, ref_a_from_manager);
    printf("manager is sending handle to B\n");
    return 0;
  } else {
    errx(1, "manager got unexpected message");
  }
}

int main(void) {
  if (signal(SIGCHLD, do_exit))
    err(1, "signal");

  struct binder_state *bs_mgr = binder_open(BINDER_PATH, 0x400000);
  if (bs_mgr == NULL)
    err(1, "binder_open()");
  if (binder_become_context_manager(bs_mgr))
    err(1, "become mgr");

  pid_t child = fork();
  if (child == -1)
    err(1, "fork");
  if (child == 0) {
    prctl(PR_SET_PDEATHSIG, SIGKILL);
    if (getppid() == 1) exit(0);

    /* create endpoint A and send message with handle to manager */
    {
      struct binder_state *bs_a = binder_open(BINDER_PATH, 0x400000);
      if (bs_a == NULL) err(1, "binder_open()");

        struct binder_io msg;
        struct binder_io reply;
        char data[0x1000];
        bio_init(&msg, data, sizeof(data), 4);
        bio_put_obj(&msg, (void*)1);
        if (binder_call(bs_a, &msg, &reply, 0, 1/*code*/))
          errx(1, "binder_call");
        binder_done(bs_a, &msg, &reply);
    }

    /* create endpoint B and retrieve handle from manager */
    struct binder_state *bs_b;
    uint32_t ref_a_from_b;
    {
      bs_b = binder_open(BINDER_PATH, 0x400000);
      if (bs_b == NULL) err(1, "binder_open()");

        struct binder_io msg;
        struct binder_io reply;
        char data[0x1000];
        bio_init(&msg, data, sizeof(data), 4);
        if (binder_call(bs_b, &msg, &reply, 0, 2/*code*/))
          errx(1, "binder_call");
        ref_a_from_b = bio_get_ref(&reply);
        if (ref_a_from_b == 0)
          errx(1, "B received bogus reply");
        binder_acquire(bs_b, ref_a_from_b);
        printf("B received handle 0x%x from manager\n", ref_a_from_b);
        binder_done(bs_b, &msg, &reply);
    }

    /* let B send a message with a valid FD and an invalid FD to A */
    {
      struct binder_io msg;
      struct binder_io reply;
      char data[0x1000];
      bio_init(&msg, data, sizeof(data), 4);
      bio_put_fd(&msg, 0); /*valid*/
      bio_put_fd(&msg, -1); /*invalid*/
      if (binder_call(bs_b, &msg, &reply, ref_a_from_b, 3/*code*/))
        errx(1, "binder_call");
    }

    exit(0);
  }

  binder_loop(bs_mgr, my_handler);
}
=================

[-- Attachment #2: binder.h --]
[-- Type: text/x-chdr, Size: 4181 bytes --]

/* Copyright 2008 The Android Open Source Project
 */
#ifndef _BINDER_H_
#define _BINDER_H_

#include <sys/ioctl.h>
#include </h/aosp-walleye/bionic/libc/kernel/uapi/linux/android/binder.h>

struct binder_state
{
    int fd;
    void *mapped;
    size_t mapsize;
};
struct binder_io
{
    char *data;            /* pointer to read/write from */
    binder_size_t *offs;   /* array of offsets */
    size_t data_avail;     /* bytes available in data buffer */
    size_t offs_avail;     /* entries available in offsets array */
    char *data0;           /* start of data buffer */
    binder_size_t *offs0;  /* start of offsets buffer */
    uint32_t flags;
    uint32_t unused;
    uint64_t buffers_size;
};
struct binder_death {
    void (*func)(struct binder_state *bs, void *ptr);
    void *ptr;
};
/* the one magic handle */
#define BINDER_SERVICE_MANAGER  0U
#define SVC_MGR_NAME "android.os.IServiceManager"
enum {
    /* Must match definitions in IBinder.h and IServiceManager.h */
    PING_TRANSACTION  = B_PACK_CHARS('_','P','N','G'),
    SVC_MGR_GET_SERVICE = 1,
    SVC_MGR_CHECK_SERVICE,
    SVC_MGR_ADD_SERVICE,
    SVC_MGR_LIST_SERVICES,
};
typedef int (*binder_handler)(struct binder_state *bs,
                              struct binder_transaction_data *txn,
                              struct binder_io *msg,
                              struct binder_io *reply);
struct binder_state *binder_open(char *device, size_t mapsize);
void binder_close(struct binder_state *bs);
/* initiate a blocking binder call
 * - returns zero on success
 */
int binder_call(struct binder_state *bs,
                struct binder_io *msg, struct binder_io *reply,
                uint32_t target, uint32_t code);
int binder_call_async(struct binder_state *bs,
                struct binder_io *msg,
                uint32_t target, uint32_t code);
int binder_read_reply(struct binder_state* bs,
                struct binder_io* reply);
int binder_read_reply_handler(struct binder_state* bs,
                struct binder_io* reply, binder_handler func);
/* release any state associate with the binder_io
 * - call once any necessary data has been extracted from the
 *   binder_io after binder_call() returns
 * - can safely be called even if binder_call() fails
 */
void binder_done(struct binder_state *bs,
                 struct binder_io *msg, struct binder_io *reply);

/* manipulate strong references */
void binder_acquire(struct binder_state *bs, uint32_t target);
void binder_release(struct binder_state *bs, uint32_t target);
void binder_increfs(struct binder_state *bs, uint32_t target);
void binder_decrefs(struct binder_state *bs, uint32_t target);
void binder_link_to_death(struct binder_state *bs, uint32_t target, struct binder_death *death);
void binder_loop(struct binder_state *bs, binder_handler func);
int binder_become_context_manager(struct binder_state *bs);
/* allocate a binder_io, providing a stack-allocated working
 * buffer, size of the working buffer, and how many object
 * offset entries to reserve from the buffer
 */
void bio_init(struct binder_io *bio, void *data,
           size_t maxdata, size_t maxobjects);
void bio_put_obj(struct binder_io *bio, void *ptr);
void bio_put_fd(struct binder_io *bio, int fd);
void bio_put_ref(struct binder_io *bio, uint32_t handle);
void bio_put_uint32(struct binder_io *bio, uint32_t n);
void bio_put_string16(struct binder_io *bio, const uint16_t *str);
void bio_put_string16_x(struct binder_io *bio, const char *_str);
void bio_put_string8_x(struct binder_io *bio, const char *_str);
void bio_put_cstring(struct binder_io *bio, const char *_str);
uint32_t bio_get_uint32(struct binder_io *bio);
uint16_t *bio_get_string16(struct binder_io *bio, size_t *sz);
char *bio_get_string8(struct binder_io *bio, size_t *sz);
uint32_t bio_get_ref(struct binder_io *bio);
uint32_t bio_get_ref_cookie(struct binder_io *bio, uint64_t* cookie);
void bio_put_buf(struct binder_io *bio, void *data, size_t len, int *buf_id);
void bio_put_sub_buf(struct binder_io *bio, int parent_id, int parent_offset, void *data, size_t len, int *buf_id);

int binder_write(struct binder_state *bs, void *data, size_t len);
#endif

[-- Attachment #3: binder.c --]
[-- Type: text/x-csrc, Size: 25964 bytes --]

/* Copyright 2008 The Android Open Source Project
 */
#include <inttypes.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>
#include <err.h>
#include <sys/mman.h>
#include <sys/wait.h>
#include "binder.h"
#define MAX_BIO_SIZE (1 << 30)
#define TRACE 0
#define LOG_TAG "Binder"
#define ALOGE(...)
#define ALOGW(...)
#define ALOGD(...)
void bio_init_from_txn(struct binder_io *io, struct binder_transaction_data *txn);
#if TRACE
void hexdump(void *_data, size_t len)
{
    unsigned char *data = _data;
    size_t count;
    for (count = 0; count < len; count++) {
        if ((count & 15) == 0)
            fprintf(stderr,"%04zu:", count);
        fprintf(stderr," %02x %c", *data,
                (*data < 32) || (*data > 126) ? '.' : *data);
        data++;
        if ((count & 15) == 15)
            fprintf(stderr,"\n");
    }
    if ((count & 15) != 0)
        fprintf(stderr,"\n");
}
void binder_dump_txn(struct binder_transaction_data *txn)
{
    struct flat_binder_object *obj;
    binder_size_t *offs = (binder_size_t *)(uintptr_t)txn->data.ptr.offsets;
    size_t count = txn->offsets_size / sizeof(binder_size_t);
    fprintf(stderr,"  target %016"PRIx64"  cookie %016"PRIx64"  code %08x  flags %08x\n",
            (uint64_t)txn->target.ptr, (uint64_t)txn->cookie, txn->code, txn->flags);
    fprintf(stderr,"  pid %8d  uid %8d  data %"PRIu64"  offs %"PRIu64"\n",
            txn->sender_pid, txn->sender_euid, (uint64_t)txn->data_size, (uint64_t)txn->offsets_size);
    hexdump((void *)(uintptr_t)txn->data.ptr.buffer, txn->data_size);
    while (count--) {
        obj = (struct flat_binder_object *) (((char*)(uintptr_t)txn->data.ptr.buffer) + *offs++);
        fprintf(stderr,"  - type %08x  flags %08x  ptr %016"PRIx64"  cookie %016"PRIx64"\n",
                obj->hdr.type, obj->flags, (uint64_t)obj->binder, (uint64_t)obj->cookie);
    }
}
#define NAME(n) case n: return #n
const char *cmd_name(uint32_t cmd)
{
    switch(cmd) {
        NAME(BR_NOOP);
        NAME(BR_TRANSACTION_COMPLETE);
        NAME(BR_INCREFS);
        NAME(BR_ACQUIRE);
        NAME(BR_RELEASE);
        NAME(BR_DECREFS);
        NAME(BR_TRANSACTION);
        NAME(BR_REPLY);
        NAME(BR_FAILED_REPLY);
        NAME(BR_DEAD_REPLY);
        NAME(BR_DEAD_BINDER);
    default: return "???";
    }
}
#else
#define hexdump(a,b) do{} while (0)
#define binder_dump_txn(txn)  do{} while (0)
#endif
#define BIO_F_SHARED    0x01  /* needs to be buffer freed */
#define BIO_F_OVERFLOW  0x02  /* ran out of space */
#define BIO_F_IOERROR   0x04
#define BIO_F_MALLOCED  0x08  /* needs to be free()'d */
struct binder_state *binder_open(char *device, size_t mapsize)
{
    struct binder_state *bs;
    struct binder_version vers;
    bs = malloc(sizeof(*bs));
    if (!bs) {
        errno = ENOMEM;
        return NULL;
    }
    bs->fd = open(device, O_RDWR | O_CLOEXEC);
    if (bs->fd < 0) {
        fprintf(stderr,"binder: cannot open device (%s)\n",
                strerror(errno));
        goto fail_open;
    }
    if ((ioctl(bs->fd, BINDER_VERSION, &vers) == -1) ||
        (vers.protocol_version != BINDER_CURRENT_PROTOCOL_VERSION)) {
        fprintf(stderr,
                "binder: kernel driver version (%d) differs from user space version (%d)\n",
                vers.protocol_version, BINDER_CURRENT_PROTOCOL_VERSION);
        goto fail_open;
    }
    bs->mapsize = mapsize;
    bs->mapped = mmap(NULL, mapsize, PROT_READ, MAP_PRIVATE, bs->fd, 0);
    if (bs->mapped == MAP_FAILED) {
        fprintf(stderr,"binder: cannot map device (%s)\n",
                strerror(errno));
        goto fail_map;
    }
    if (madvise(bs->mapped, mapsize, MADV_DOFORK)) err(1, "MADV_DOFORK");
    return bs;
fail_map:
    close(bs->fd);
fail_open:
    free(bs);
    return NULL;
}
void binder_close(struct binder_state *bs)
{
    munmap(bs->mapped, bs->mapsize);
    close(bs->fd);
    free(bs);
}
int binder_become_context_manager(struct binder_state *bs)
{
    return ioctl(bs->fd, BINDER_SET_CONTEXT_MGR, 0);
}
int binder_write(struct binder_state *bs, void *data, size_t len)
{
    struct binder_write_read bwr;
    int res;
    bwr.write_size = len;
    bwr.write_consumed = 0;
    bwr.write_buffer = (uintptr_t) data;
    bwr.read_size = 0;
    bwr.read_consumed = 0;
    bwr.read_buffer = 0;
    res = ioctl(bs->fd, BINDER_WRITE_READ, &bwr);
    if (res < 0) {
        fprintf(stderr,"binder_write: ioctl failed (%s)\n",
                strerror(errno));
    }
    return res;
}
void binder_send_reply(struct binder_state *bs,
                       struct binder_io *reply,
                       binder_uintptr_t buffer_to_free,
                       int status)
{
    printf("binder_send_reply(status=%d)\n", status);
    struct {
        uint32_t cmd_free;
        binder_uintptr_t buffer;
        uint32_t cmd_reply;
        struct binder_transaction_data_sg txn_sg;
    } __attribute__((packed)) data;
    data.cmd_free = BC_FREE_BUFFER;
    data.buffer = buffer_to_free;
    data.cmd_reply = BC_REPLY_SG;
    data.txn_sg.buffers_size = reply->buffers_size;
    data.txn_sg.transaction_data.target.ptr = 0;
    data.txn_sg.transaction_data.cookie = 0;
    data.txn_sg.transaction_data.code = 0;
    if (status) {
        data.txn_sg.transaction_data.flags = TF_STATUS_CODE;
        data.txn_sg.transaction_data.data_size = sizeof(int);
        data.txn_sg.transaction_data.offsets_size = 0;
        data.txn_sg.transaction_data.data.ptr.buffer = (uintptr_t)&status;
        data.txn_sg.transaction_data.data.ptr.offsets = 0;
    } else {
        data.txn_sg.transaction_data.flags = 0;
        data.txn_sg.transaction_data.data_size = reply->data - reply->data0;
        data.txn_sg.transaction_data.offsets_size = ((char*) reply->offs) - ((char*) reply->offs0);
        data.txn_sg.transaction_data.data.ptr.buffer = (uintptr_t)reply->data0;
        data.txn_sg.transaction_data.data.ptr.offsets = (uintptr_t)reply->offs0;
        if (data.txn_sg.transaction_data.offsets_size) {
            *(volatile unsigned long *)data.txn_sg.transaction_data.data.ptr.offsets;
        }
        printf("offsets=%p, offsets_size=%lu\n",
            reply->offs0,
            (unsigned long)data.txn_sg.transaction_data.offsets_size);
    }
    binder_write(bs, &data, sizeof(data));
}
int binder_parse(struct binder_state *bs, struct binder_io *bio,
                 uintptr_t ptr, size_t size, binder_handler func)
{
    int r = 1;
    uintptr_t end = ptr + (uintptr_t) size;
    while (ptr < end) {
        uint32_t cmd = *(uint32_t *) ptr;
        ptr += sizeof(uint32_t);
#if TRACE
        fprintf(stderr,"%s:\n", cmd_name(cmd));
#endif
        switch(cmd) {
        case BR_NOOP:
            break;
        case BR_TRANSACTION_COMPLETE:
            break;
        case BR_INCREFS:
        case BR_ACQUIRE:
        case BR_RELEASE:
        case BR_DECREFS:
#if TRACE
            fprintf(stderr,"  %p, %p\n", (void *)ptr, (void *)(ptr + sizeof(void *)));
#endif
            ptr += sizeof(struct binder_ptr_cookie);
            break;
        case BR_TRANSACTION: {
            struct binder_transaction_data *txn = (struct binder_transaction_data *) ptr;
            if ((end - ptr) < sizeof(*txn)) {
                ALOGE("parse: txn too small!\n");
                return -1;
            }
            binder_dump_txn(txn);
            if (func) {
                unsigned rdata[256/4];
                struct binder_io msg;
                struct binder_io reply;
                int res;
                bio_init(&reply, rdata, sizeof(rdata), 4);
                bio_init_from_txn(&msg, txn);
                res = func(bs, txn, &msg, &reply);
                if ((txn->flags & 1) == 0) {
                    binder_send_reply(bs, &reply, txn->data.ptr.buffer, res);
                }
            }
            ptr += sizeof(*txn);
            break;
        }
        case BR_REPLY: {
            struct binder_transaction_data *txn = (struct binder_transaction_data *) ptr;
            if ((end - ptr) < sizeof(*txn)) {
                ALOGE("parse: reply too small!\n");
                return -1;
            }
            binder_dump_txn(txn);
            if (bio) {
                bio_init_from_txn(bio, txn);
                bio = 0;
            } else {
                /* todo FREE BUFFER */
            }
            ptr += sizeof(*txn);
            r = 0;
            break;
        }
        case BR_DEAD_BINDER: {
            struct binder_death *death = (struct binder_death *)(uintptr_t) *(binder_uintptr_t *)ptr;
            ptr += sizeof(binder_uintptr_t);
            death->func(bs, death->ptr);
            break;
        }
        case BR_FAILED_REPLY:
            r = -1;
            break;
        case BR_DEAD_REPLY:
            r = -1;
            break;
        default:
            ALOGE("parse: OOPS %d\n", cmd);
            return -1;
        }
    }
    return r;
}
void binder_acquire(struct binder_state *bs, uint32_t target)
{
    uint32_t cmd[2];
    cmd[0] = BC_ACQUIRE;
    cmd[1] = target;
    binder_write(bs, cmd, sizeof(cmd));
}
void binder_release(struct binder_state *bs, uint32_t target)
{
    uint32_t cmd[2];
    cmd[0] = BC_RELEASE;
    cmd[1] = target;
    binder_write(bs, cmd, sizeof(cmd));
}
void binder_increfs(struct binder_state *bs, uint32_t target)
{
    uint32_t cmd[2];
    cmd[0] = BC_INCREFS;
    cmd[1] = target;
    binder_write(bs, cmd, sizeof(cmd));
}
void binder_decrefs(struct binder_state *bs, uint32_t target)
{
    uint32_t cmd[2];
    cmd[0] = BC_DECREFS;
    cmd[1] = target;
    binder_write(bs, cmd, sizeof(cmd));
}
void binder_link_to_death(struct binder_state *bs, uint32_t target, struct binder_death *death)
{
    struct {
        uint32_t cmd;
        struct binder_handle_cookie payload;
    } __attribute__((packed)) data;
    data.cmd = BC_REQUEST_DEATH_NOTIFICATION;
    data.payload.handle = target;
    data.payload.cookie = (uintptr_t) death;
    binder_write(bs, &data, sizeof(data));
}
int binder_call(struct binder_state *bs,
                struct binder_io *msg, struct binder_io *reply,
                uint32_t target, uint32_t code)
{
    int res;
    struct binder_write_read bwr;
    struct {
        uint32_t cmd;
        struct binder_transaction_data_sg txn_sg;
    } __attribute__((packed)) writebuf;
    unsigned readbuf[32];
    if (msg->flags & BIO_F_OVERFLOW) {
        fprintf(stderr,"binder: txn buffer overflow\n");
        goto fail;
    }
    writebuf.cmd = BC_TRANSACTION_SG;
    writebuf.txn_sg.buffers_size = msg->buffers_size;
    writebuf.txn_sg.transaction_data.target.handle = target;
    writebuf.txn_sg.transaction_data.code = code;
    writebuf.txn_sg.transaction_data.flags = TF_ACCEPT_FDS;
    writebuf.txn_sg.transaction_data.data_size = msg->data - msg->data0;
    writebuf.txn_sg.transaction_data.offsets_size = ((char*) msg->offs) - ((char*) msg->offs0);
    writebuf.txn_sg.transaction_data.data.ptr.buffer = (uintptr_t)msg->data0;
    writebuf.txn_sg.transaction_data.data.ptr.offsets = (uintptr_t)msg->offs0;
    bwr.write_size = sizeof(writebuf);
    bwr.write_consumed = 0;
    bwr.write_buffer = (uintptr_t) &writebuf;
    hexdump(msg->data0, msg->data - msg->data0);
    bool first_iter = false;
    for (;;) {
        bwr.read_size = sizeof(readbuf);
        bwr.read_consumed = 0;
        bwr.read_buffer = (uintptr_t) readbuf;

        if (first_iter) {
            bwr.read_size = 0;
            printf("%d forking...\n", getpid());
            pid_t child = fork();
            if (child == -1) err(1, "fork");
            if (child == 0) {

                printf("entering child: %d\n", getpid());
                res = ioctl(bs->fd, BINDER_WRITE_READ, &bwr);
                if (res < 0) {
                    fprintf(stderr,"binder: ioctl failed (%s)\n", strerror(errno));
                }
                if (bwr.write_consumed != bwr.write_size) {
                    errx(1, "write_consumed != write_size");
                }

                printf("child exiting\n");
                exit(0);
            }
            int status;
            if (wait(&status) != child) err(1, "wait for child");
            bwr.write_consumed = bwr.write_size;
            printf("child is dead\n");
        } else {
            res = ioctl(bs->fd, BINDER_WRITE_READ, &bwr);
            if (res < 0) {
                fprintf(stderr,"binder: ioctl failed (%s)\n", strerror(errno));
                goto fail;
            }
            res = binder_parse(bs, reply, (uintptr_t) readbuf, bwr.read_consumed, 0);
            if (res == 0) return 0;
            if (res < 0) goto fail;
        }

        first_iter = false;
    }
fail:
    memset(reply, 0, sizeof(*reply));
    reply->flags |= BIO_F_IOERROR;
    return -1;
}
int binder_call_async(struct binder_state *bs,
                struct binder_io *msg, uint32_t target, uint32_t code)
{
    int res;
    struct binder_write_read bwr;
    struct {
        uint32_t cmd;
        struct binder_transaction_data txn;
    } __attribute__((packed)) writebuf;
    if (msg->flags & BIO_F_OVERFLOW) {
        fprintf(stderr,"binder: txn buffer overflow\n");
        goto fail;
    }
    writebuf.cmd = BC_TRANSACTION;
    writebuf.txn.target.handle = target;
    writebuf.txn.code = code;
    writebuf.txn.flags = TF_ACCEPT_FDS;
    writebuf.txn.data_size = msg->data - msg->data0;
    writebuf.txn.offsets_size = ((char*) msg->offs) - ((char*) msg->offs0);
    writebuf.txn.data.ptr.buffer = (uintptr_t)msg->data0;
    writebuf.txn.data.ptr.offsets = (uintptr_t)msg->offs0;
    bwr.write_size = sizeof(writebuf);
    bwr.write_consumed = 0;
    bwr.write_buffer = (uintptr_t) &writebuf;
    bwr.read_size = 0;
    bwr.read_buffer = 0;
    bwr.read_consumed = 0;
    hexdump(msg->data0, msg->data - msg->data0);
    res = ioctl(bs->fd, BINDER_WRITE_READ, &bwr);
    if (res < 0) {
        fprintf(stderr,"binder: ioctl failed (%s)\n", strerror(errno));
        goto fail;
    }
    return 0;
fail:
    return -1;
}


int binder_read_reply(struct binder_state* bs,
                struct binder_io* reply)
{
    int res;
    struct binder_write_read bwr;
    unsigned readbuf[32];
    bwr.write_size = 0;
    bwr.write_consumed = 0;
    bwr.write_buffer = 0;
    for (;;) {
        bwr.read_size = sizeof(readbuf);
        bwr.read_consumed = 0;
        bwr.read_buffer = (uintptr_t) readbuf;
        res = ioctl(bs->fd, BINDER_WRITE_READ, &bwr);
        if (res < 0) {
            fprintf(stderr,"binder: ioctl failed (%s)\n", strerror(errno));
            goto fail;
        }
        res = binder_parse(bs, reply, (uintptr_t) readbuf, bwr.read_consumed, 0);
        if (res == 0) return 0;
        if (res < 0) goto fail;
    }
fail:
    memset(reply, 0, sizeof(*reply));
    reply->flags |= BIO_F_IOERROR;
    return -1;
}

int binder_read_reply_handler(struct binder_state* bs,
                struct binder_io* reply, binder_handler func)
{
    int res;
    struct binder_write_read bwr;
    unsigned readbuf[32];
    bwr.write_size = 0;
    bwr.write_consumed = 0;
    bwr.write_buffer = 0;
    for (;;) {
        bwr.read_size = sizeof(readbuf);
        bwr.read_consumed = 0;
        bwr.read_buffer = (uintptr_t) readbuf;
        res = ioctl(bs->fd, BINDER_WRITE_READ, &bwr);
        if (res < 0) {
            fprintf(stderr,"binder: ioctl failed (%s)\n", strerror(errno));
            goto fail;
        }
        res = binder_parse(bs, reply, (uintptr_t) readbuf, bwr.read_consumed, func);
        if (res == 0) return 0;
        if (res < 0) goto fail;
    }
fail:
    memset(reply, 0, sizeof(*reply));
    reply->flags |= BIO_F_IOERROR;
    return -1;
}

void binder_loop(struct binder_state *bs, binder_handler func)
{
    int res;
    struct binder_write_read bwr;
    uint32_t readbuf[32];
    bwr.write_size = 0;
    bwr.write_consumed = 0;
    bwr.write_buffer = 0;
    readbuf[0] = BC_ENTER_LOOPER;
    binder_write(bs, readbuf, sizeof(uint32_t));
    for (;;) {
        bwr.read_size = sizeof(readbuf);
        bwr.read_consumed = 0;
        bwr.read_buffer = (uintptr_t) readbuf;
        res = ioctl(bs->fd, BINDER_WRITE_READ, &bwr);
        if (res < 0) {
            fprintf(stderr,"binder_loop: ioctl failed (%s)\n", strerror(errno));
            break;
        }
        res = binder_parse(bs, 0, (uintptr_t) readbuf, bwr.read_consumed, func);
        if (res == 0) {
            fprintf(stderr,"binder_loop: unexpected reply?!\n");
            break;
        }
        if (res < 0) {
            fprintf(stderr,"binder_loop: io error %d %s\n", res, strerror(errno));
            break;
        }
    }
}
void bio_init_from_txn(struct binder_io *bio, struct binder_transaction_data *txn)
{
    bio->data = bio->data0 = (char *)(intptr_t)txn->data.ptr.buffer;
    bio->offs = bio->offs0 = (binder_size_t *)(intptr_t)txn->data.ptr.offsets;
    bio->data_avail = txn->data_size;
    bio->offs_avail = txn->offsets_size / sizeof(size_t);
    bio->flags = BIO_F_SHARED;
}
void bio_init(struct binder_io *bio, void *data,
              size_t maxdata, size_t maxoffs)
{
    size_t n = maxoffs * sizeof(size_t);
    if (n > maxdata) {
        bio->flags = BIO_F_OVERFLOW;
        bio->data_avail = 0;
        bio->offs_avail = 0;
        return;
    }
    bio->data = bio->data0 = (char *) data + n;
    bio->offs = bio->offs0 = data;
    bio->data_avail = maxdata - n;
    bio->offs_avail = maxoffs;
    bio->flags = 0;
    bio->buffers_size = 0;
}
static void *bio_alloc(struct binder_io *bio, size_t size)
{
    size = (size + 3) & (~3);
    if (size > bio->data_avail) {
        bio->flags |= BIO_F_OVERFLOW;
        return NULL;
    } else {
        void *ptr = bio->data;
        bio->data += size;
        bio->data_avail -= size;
        return ptr;
    }
}
void binder_done(struct binder_state *bs,
                 struct binder_io *msg,
                 struct binder_io *reply)
{
    struct {
        uint32_t cmd;
        uintptr_t buffer;
    } __attribute__((packed)) data;
    if (reply->flags & BIO_F_SHARED) {
        printf("binder_done: freeing buffer\n");
        data.cmd = BC_FREE_BUFFER;
        data.buffer = (uintptr_t) reply->data0;
        binder_write(bs, &data, sizeof(data));
        reply->flags = 0;
        printf("binder_done: free done\n");
    }
}
static struct flat_binder_object *bio_alloc_obj(struct binder_io *bio)
{
    struct flat_binder_object *obj;
    obj = bio_alloc(bio, sizeof(*obj));
    if (obj && bio->offs_avail) {
        bio->offs_avail--;
        *bio->offs++ = ((char*) obj) - ((char*) bio->data0);
        return obj;
    }
    bio->flags |= BIO_F_OVERFLOW;
    return NULL;
}
static struct binder_fd_array_object *bio_alloc_fda(struct binder_io *bio)
{
    struct binder_fd_array_object *obj;
    obj = bio_alloc(bio, sizeof(*obj));
    if (obj && bio->offs_avail) {
        bio->offs_avail--;
        *bio->offs++ = ((char*) obj) - ((char*) bio->data0);
        return obj;
    }
    bio->flags |= BIO_F_OVERFLOW;
    return NULL;
}
static struct binder_buffer_object *bio_alloc_buf(struct binder_io *bio, int *buf_id)
{
    struct binder_buffer_object *obj;
    obj = bio_alloc(bio, sizeof(*obj));
    if (obj && bio->offs_avail) {
        bio->offs_avail--;
        if (buf_id) *buf_id = bio->offs - bio->offs0;
        *bio->offs++ = ((char*) obj) - ((char*) bio->data0);
        return obj;
    }
    bio->flags |= BIO_F_OVERFLOW;
    return NULL;
}
void bio_put_uint32(struct binder_io *bio, uint32_t n)
{
    uint32_t *ptr = bio_alloc(bio, sizeof(n));
    if (ptr)
        *ptr = n;
}
void bio_put_obj(struct binder_io *bio, void *ptr)
{
    struct flat_binder_object *obj;
    obj = bio_alloc_obj(bio);
    if (!obj)
        return;
    obj->flags = 0x7f | FLAT_BINDER_FLAG_ACCEPTS_FDS;
    obj->hdr.type = BINDER_TYPE_BINDER;
    obj->binder = (uintptr_t)ptr;
    obj->cookie = 0;
}
void bio_put_fd(struct binder_io *bio, int fd)
{
    struct flat_binder_object *obj;
    obj = bio_alloc_obj(bio);
    if (!obj)
        return;
    obj->flags = 0x7f | FLAT_BINDER_FLAG_ACCEPTS_FDS;
    obj->hdr.type = BINDER_TYPE_FD;
    obj->binder = (uintptr_t)fd;
    obj->cookie = 0;
}

void bio_put_buf(struct binder_io *bio, void *data, size_t len, int *buf_id) {
    struct binder_buffer_object *obj;
    obj = bio_alloc_buf(bio, buf_id);
    if (!obj)
        return;
    obj->hdr.type = BINDER_TYPE_PTR;
    obj->flags = 0;
    obj->buffer = (unsigned long)data;
    obj->length = len;
    obj->parent = 0; // unused
    obj->parent_offset = 0; // unused
    bio->buffers_size += (len+7)&~7UL; // TODO rounding blargh
}

void bio_put_sub_buf(struct binder_io *bio, int parent_id, int parent_offset, void *data, size_t len, int *buf_id) {
    struct binder_buffer_object *obj;
    obj = bio_alloc_buf(bio, buf_id);
    if (!obj)
        return;
    obj->hdr.type = BINDER_TYPE_PTR;
    obj->flags = BINDER_BUFFER_FLAG_HAS_PARENT;
    obj->buffer = (unsigned long)data;
    obj->length = len;
    obj->parent = parent_id;
    obj->parent_offset = parent_offset;
    bio->buffers_size += (len+7)&~7UL; // TODO rounding blargh
}

void bio_put_fda(struct binder_io *bio, int *fds, int fd_count) {
    int buf_id = -1;
    bio_put_buf(bio, fds, sizeof(int)*fd_count, &buf_id);
    if (buf_id == -1) errx(1, "bio_put_buf fail");
    struct binder_fd_array_object *obj;
    obj = bio_alloc_fda(bio);
    if (!obj)
        return;
    obj->hdr.type = BINDER_TYPE_FDA;
    obj->num_fds = fd_count;
    printf("fda->parent = %d\n", buf_id);
    obj->parent = buf_id;
    obj->parent_offset = 0;
}

void bio_put_ref(struct binder_io *bio, uint32_t handle)
{
    struct flat_binder_object *obj;
    if (handle)
        obj = bio_alloc_obj(bio);
    else
        obj = bio_alloc(bio, sizeof(*obj));
    if (!obj)
        return;
    obj->flags = 0x7f | FLAT_BINDER_FLAG_ACCEPTS_FDS;
    obj->hdr.type = BINDER_TYPE_HANDLE;
    obj->handle = handle;
    obj->cookie = 0;
}
void bio_put_string16(struct binder_io *bio, const uint16_t *str)
{
    size_t len;
    uint16_t *ptr;
    if (!str) {
        bio_put_uint32(bio, 0xffffffff);
        return;
    }
    len = 0;
    while (str[len]) len++;
    if (len >= (MAX_BIO_SIZE / sizeof(uint16_t))) {
        bio_put_uint32(bio, 0xffffffff);
        return;
    }
    /* Note: The payload will carry 32bit size instead of size_t */
    bio_put_uint32(bio, (uint32_t) len);
    len = (len + 1) * sizeof(uint16_t);
    ptr = bio_alloc(bio, len);
    if (ptr)
        memcpy(ptr, str, len);
}

void bio_put_cstring(struct binder_io *bio, const char *str)
{
    char* ptr = NULL;
    size_t len = 0;

    len = strlen(str) + 1;
    ptr = bio_alloc(bio, len);
    if (ptr)
        memcpy(ptr, str, len);
}

void bio_put_string16_x(struct binder_io *bio, const char *_str)
{
    unsigned char *str = (unsigned char*) _str;
    size_t len;
    uint16_t *ptr;
    if (!str) {
        bio_put_uint32(bio, 0xffffffff);
        return;
    }
    len = strlen(_str);
    if (len >= (MAX_BIO_SIZE / sizeof(uint16_t))) {
        bio_put_uint32(bio, 0xffffffff);
        return;
    }
    /* Note: The payload will carry 32bit size instead of size_t */
    bio_put_uint32(bio, len);
    ptr = bio_alloc(bio, (len + 1) * sizeof(uint16_t));
    if (!ptr)
        return;
    while (*str)
        *ptr++ = *str++;
    *ptr++ = 0;
}

void bio_put_string8_x(struct binder_io *bio, const char *_str)
{
    unsigned char *str = (unsigned char*) _str;
    size_t len;
    uint8_t *ptr;
    if (!str) {
        bio_put_uint32(bio, 0xffffffff);
        return;
    }
    len = strlen(_str);
    if (len >= (MAX_BIO_SIZE / sizeof(uint8_t))) {
        bio_put_uint32(bio, 0xffffffff);
        return;
    }
    /* Note: The payload will carry 32bit size instead of size_t */
    bio_put_uint32(bio, len);
    ptr = bio_alloc(bio, (len + 1) * sizeof(uint8_t));
    if (!ptr)
        return;
    while (*str)
        *ptr++ = *str++;
    *ptr++ = 0;
}
static void *bio_get(struct binder_io *bio, size_t size)
{
    size = (size + 3) & (~3);
    if (bio->data_avail < size){
        bio->data_avail = 0;
        bio->flags |= BIO_F_OVERFLOW;
        return NULL;
    }  else {
        void *ptr = bio->data;
        bio->data += size;
        bio->data_avail -= size;
        return ptr;
    }
}
uint32_t bio_get_uint32(struct binder_io *bio)
{
    uint32_t *ptr = bio_get(bio, sizeof(*ptr));
    return ptr ? *ptr : 0;
}
uint16_t *bio_get_string16(struct binder_io *bio, size_t *sz)
{
    size_t len;
    /* Note: The payload will carry 32bit size instead of size_t */
    len = (size_t) bio_get_uint32(bio);
    if (sz)
        *sz = len;
    return bio_get(bio, (len + 1) * sizeof(uint16_t));
}
char *bio_get_string8(struct binder_io *bio, size_t *sz) {
    size_t len;
    /* Note: The payload will carry 32bit size instead of size_t */
    len = (size_t) bio_get_uint32(bio);
    if (sz)
        *sz = len;
    return bio_get(bio, len + 1);  
}
static struct flat_binder_object *_bio_get_obj(struct binder_io *bio)
{
    size_t n;
    size_t off = bio->data - bio->data0;
    /* TODO: be smarter about this? */
    for (n = 0; n < bio->offs_avail; n++) {
        if (bio->offs[n] == off)
            return bio_get(bio, sizeof(struct flat_binder_object));
    }
    bio->data_avail = 0;
    bio->flags |= BIO_F_OVERFLOW;
    return NULL;
}
uint32_t bio_get_ref(struct binder_io *bio)
{
    struct flat_binder_object *obj;
    obj = _bio_get_obj(bio);
    if (!obj)
        return 0;
    if (obj->hdr.type == BINDER_TYPE_HANDLE)
        return obj->handle;
    return 0;
}


uint32_t bio_get_ref_cookie(struct binder_io *bio, uint64_t* cookie) {
    struct flat_binder_object *obj;
    obj = _bio_get_obj(bio);
    if (!obj)
        return 0;
    if (obj->hdr.type == BINDER_TYPE_HANDLE || obj->hdr.type == BINDER_TYPE_WEAK_HANDLE) {
        *cookie = obj->cookie;
        return obj->handle;
    }
    return 0;
}

[-- Attachment #4: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] binder: prevent transactions to context manager from its own process.
  2019-10-11 21:59 ` Jann Horn
@ 2019-10-11 22:11   ` Jann Horn
  2019-10-14 17:37     ` Hridya Valsaraju
  0 siblings, 1 reply; 6+ messages in thread
From: Jann Horn @ 2019-10-11 22:11 UTC (permalink / raw)
  To: Hridya Valsaraju, Todd Kjos
  Cc: open list:ANDROID DRIVERS, kernel-team,
	syzbot+8b3c354d33c4ac78bfad, Greg Kroah-Hartman, kernel list,
	Arve Hjønnevåg, Joel Fernandes, Martijn Coenen,
	Christian Brauner

On Fri, Oct 11, 2019 at 11:59 PM Jann Horn <jannh@google.com> wrote:
> (I think you could also let A receive a handle
> to itself and then transact with itself, but I haven't tested that.)

Ignore this sentence, that's obviously wrong because same-binder_proc
nodes will always show up as a binder, not a handle.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] binder: prevent transactions to context manager from its own process.
  2019-10-11 22:11   ` Jann Horn
@ 2019-10-14 17:37     ` Hridya Valsaraju
  2019-10-14 19:35       ` Jann Horn
  0 siblings, 1 reply; 6+ messages in thread
From: Hridya Valsaraju @ 2019-10-14 17:37 UTC (permalink / raw)
  To: Jann Horn
  Cc: open list:ANDROID DRIVERS, kernel-team, Todd Kjos,
	syzbot+8b3c354d33c4ac78bfad, Greg Kroah-Hartman, kernel list,
	Arve Hjønnevåg, Joel Fernandes, Martijn Coenen,
	Christian Brauner

On Fri, Oct 11, 2019 at 3:11 PM Jann Horn <jannh@google.com> wrote:
>
> On Fri, Oct 11, 2019 at 11:59 PM Jann Horn <jannh@google.com> wrote:
> > (I think you could also let A receive a handle
> > to itself and then transact with itself, but I haven't tested that.)
>
> Ignore this sentence, that's obviously wrong because same-binder_proc
> nodes will always show up as a binder, not a handle.

Thank you for the email and steps to reproduce the issue Jann. I need
some time to take a look at the same and I will get back to you once I
understand it and hopefully have a fix. We do want to disallow
same-process transactions. Here is a little bit more of context for
the patch: https://lkml.org/lkml/2018/3/28/173
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] binder: prevent transactions to context manager from its own process.
  2019-10-14 17:37     ` Hridya Valsaraju
@ 2019-10-14 19:35       ` Jann Horn
  0 siblings, 0 replies; 6+ messages in thread
From: Jann Horn @ 2019-10-14 19:35 UTC (permalink / raw)
  To: Hridya Valsaraju
  Cc: open list:ANDROID DRIVERS, kernel-team, Todd Kjos,
	syzbot+8b3c354d33c4ac78bfad, Greg Kroah-Hartman, kernel list,
	Arve Hjønnevåg, Joel Fernandes, Martijn Coenen,
	Christian Brauner

On Mon, Oct 14, 2019 at 7:38 PM Hridya Valsaraju <hridya@google.com> wrote:
> On Fri, Oct 11, 2019 at 3:11 PM Jann Horn <jannh@google.com> wrote:
> > On Fri, Oct 11, 2019 at 11:59 PM Jann Horn <jannh@google.com> wrote:
> > > (I think you could also let A receive a handle
> > > to itself and then transact with itself, but I haven't tested that.)
> >
> > Ignore this sentence, that's obviously wrong because same-binder_proc
> > nodes will always show up as a binder, not a handle.
>
> Thank you for the email and steps to reproduce the issue Jann. I need
> some time to take a look at the same and I will get back to you once I
> understand it and hopefully have a fix. We do want to disallow
> same-process transactions. Here is a little bit more of context for
> the patch: https://lkml.org/lkml/2018/3/28/173

That patch (commit 7aa135fcf26377f92dc0680a57566b4c7f3e281b) prevented
transactions within one *binder_proc*, which makes sense to me; that
still allows same-process transactions, so long as they are between
different binder_proc instances. What I don't understand is your
follow-up in commit 49ed96943a8e0c62cc5a9b0a6cfc88be87d1fcec, where
you try to block transactions within the same process (well, kind of,
the semantics of the term "process" are quite fuzzy here and don't map
onto binder well).
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-10-14 19:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-15 19:18 [PATCH] binder: prevent transactions to context manager from its own process Hridya Valsaraju
2019-07-15 20:36 ` Todd Kjos
2019-10-11 21:59 ` Jann Horn
2019-10-11 22:11   ` Jann Horn
2019-10-14 17:37     ` Hridya Valsaraju
2019-10-14 19:35       ` Jann Horn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).