[PATCH 0/4] binder: several fixes for frozen notification

[PATCH 0/4] binder: several fixes for frozen notification

[Carlos Llamas]

These are all fixes for the frozen notification patch [1], which as of
today hasn't landed in mainline yet. As such, this patchset is rebased
on top of the char-misc-next branch.

[1] https://lore.kernel.org/all/20240709070047.4055369-2-yutingtseng@google.com/

Cc: stable@vger.kernel.org
Cc: Yu-Ting Tseng <yutingtseng@google.com>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Martijn Coenen <maco@google.com>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Viktor Martensson <vmartensson@google.com>

Carlos Llamas (4):
  binder: fix node UAF in binder_add_freeze_work()
  binder: fix OOB in binder_add_freeze_work()
  binder: fix freeze UAF in binder_release_work()
  binder: fix BINDER_WORK_FROZEN_BINDER debug logs

 drivers/android/binder.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.46.0.792.g87dc391469-goog

[PATCH 1/4] binder: fix node UAF in binder_add_freeze_work()

[Carlos Llamas]

In binder_add_freeze_work() we iterate over the proc->nodes with the
proc->inner_lock held. However, this lock is temporarily dropped in
order to acquire the node->lock first (lock nesting order). This can
race with binder_node_release() and trigger a use-after-free:

  ==================================================================
  BUG: KASAN: slab-use-after-free in _raw_spin_lock+0xe4/0x19c
  Write of size 4 at addr ffff53c04c29dd04 by task freeze/640

  CPU: 5 UID: 0 PID: 640 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #17
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   _raw_spin_lock+0xe4/0x19c
   binder_add_freeze_work+0x148/0x478
   binder_ioctl+0x1e70/0x25ac
   __arm64_sys_ioctl+0x124/0x190

  Allocated by task 637:
   __kmalloc_cache_noprof+0x12c/0x27c
   binder_new_node+0x50/0x700
   binder_transaction+0x35ac/0x6f74
   binder_thread_write+0xfb8/0x42a0
   binder_ioctl+0x18f0/0x25ac
   __arm64_sys_ioctl+0x124/0x190

  Freed by task 637:
   kfree+0xf0/0x330
   binder_thread_read+0x1e88/0x3a68
   binder_ioctl+0x16d8/0x25ac
   __arm64_sys_ioctl+0x124/0x190
  ==================================================================

Fix the race by taking a temporary reference on the node before
releasing the proc->inner lock. This ensures the node remains alive
while in use.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 978740537a1a..4d90203ea048 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5552,6 +5552,7 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
 
 static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 {
+	struct binder_node *prev = NULL;
 	struct rb_node *n;
 	struct binder_ref *ref;
 
@@ -5560,7 +5561,10 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 		struct binder_node *node;
 
 		node = rb_entry(n, struct binder_node, rb_node);
+		binder_inc_node_tmpref_ilocked(node);
 		binder_inner_proc_unlock(proc);
+		if (prev)
+			binder_put_node(prev);
 		binder_node_lock(node);
 		hlist_for_each_entry(ref, &node->refs, node_entry) {
 			/*
@@ -5586,10 +5590,13 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 			}
 			binder_inner_proc_unlock(ref->proc);
 		}
+		prev = node;
 		binder_node_unlock(node);
 		binder_inner_proc_lock(proc);
 	}
 	binder_inner_proc_unlock(proc);
+	if (prev)
+		binder_put_node(prev);
 }
 
 static int binder_ioctl_freeze(struct binder_freeze_info *info,
-- 
2.46.0.792.g87dc391469-goog

[PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Carlos Llamas]

In binder_add_freeze_work() we iterate over the proc->nodes with the
proc->inner_lock held. However, this lock is temporarily dropped to
acquire the node->lock first (lock nesting order). This can race with
binder_deferred_release() which removes the nodes from the proc->nodes
rbtree and adds them into binder_dead_nodes list. This leads to a broken
iteration in binder_add_freeze_work() as rb_next() will use data from
binder_dead_nodes, triggering an out-of-bounds access:

  ==================================================================
  BUG: KASAN: global-out-of-bounds in rb_next+0xfc/0x124
  Read of size 8 at addr ffffcb84285f7170 by task freeze/660

  CPU: 8 UID: 0 PID: 660 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #18
  Hardware name: linux,dummy-virt (DT)
  Call trace:
   rb_next+0xfc/0x124
   binder_add_freeze_work+0x344/0x534
   binder_ioctl+0x1e70/0x25ac
   __arm64_sys_ioctl+0x124/0x190

  The buggy address belongs to the variable:
   binder_dead_nodes+0x10/0x40
  [...]
  ==================================================================

This is possible because proc->nodes (rbtree) and binder_dead_nodes
(list) share entries in binder_node through a union:

	struct binder_node {
	[...]
		union {
			struct rb_node rb_node;
			struct hlist_node dead_node;
		};

Fix the race by checking that the proc is still alive. If not, simply
break out of the iteration.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 4d90203ea048..8bca2de6fa24 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5593,6 +5593,8 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
 		prev = node;
 		binder_node_unlock(node);
 		binder_inner_proc_lock(proc);
+		if (proc->is_dead)
+			break;
 	}
 	binder_inner_proc_unlock(proc);
 	if (prev)
-- 
2.46.0.792.g87dc391469-goog

[PATCH 3/4] binder: fix freeze UAF in binder_release_work()

[Carlos Llamas]

When a binder reference is cleaned up, any freeze work queued in the
associated process should also be removed. Otherwise, the reference is
freed while its ref->freeze.work is still queued in proc->work leading
to a use-after-free issue as shown by the following KASAN report:

  ==================================================================
  BUG: KASAN: slab-use-after-free in binder_release_work+0x398/0x3d0
  Read of size 8 at addr ffff31600ee91488 by task kworker/5:1/211

  CPU: 5 UID: 0 PID: 211 Comm: kworker/5:1 Not tainted 6.11.0-rc7-00382-gfc6c92196396 #22
  Hardware name: linux,dummy-virt (DT)
  Workqueue: events binder_deferred_func
  Call trace:
   binder_release_work+0x398/0x3d0
   binder_deferred_func+0xb60/0x109c
   process_one_work+0x51c/0xbd4
   worker_thread+0x608/0xee8

  Allocated by task 703:
   __kmalloc_cache_noprof+0x130/0x280
   binder_thread_write+0xdb4/0x42a0
   binder_ioctl+0x18f0/0x25ac
   __arm64_sys_ioctl+0x124/0x190
   invoke_syscall+0x6c/0x254

  Freed by task 211:
   kfree+0xc4/0x230
   binder_deferred_func+0xae8/0x109c
   process_one_work+0x51c/0xbd4
   worker_thread+0x608/0xee8
  ==================================================================

This commit fixes the issue by ensuring any queued freeze work is removed
when cleaning up a binder reference.

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 8bca2de6fa24..d955135ee37a 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1225,6 +1225,12 @@ static void binder_cleanup_ref_olocked(struct binder_ref *ref)
 		binder_dequeue_work(ref->proc, &ref->death->work);
 		binder_stats_deleted(BINDER_STAT_DEATH);
 	}
+
+	if (ref->freeze) {
+		binder_dequeue_work(ref->proc, &ref->freeze->work);
+		binder_stats_deleted(BINDER_STAT_FREEZE);
+	}
+
 	binder_stats_deleted(BINDER_STAT_REF);
 }
 
-- 
2.46.0.792.g87dc391469-goog

[PATCH 4/4] binder: fix BINDER_WORK_FROZEN_BINDER debug logs

[Carlos Llamas]

The BINDER_WORK_FROZEN_BINDER type is not handled in the binder_logs
entries and it shows up as "unknown work" when logged:

  proc 649
  context binder-test
    thread 649: l 00 need_return 0 tr 0
    ref 13: desc 1 node 8 s 1 w 0 d 0000000053c4c0c3
    unknown work: type 10

This patch add the freeze work type and is now logged as such:

  proc 637
  context binder-test
    thread 637: l 00 need_return 0 tr 0
    ref 8: desc 1 node 3 s 1 w 0 d 00000000dc39e9c6
    has frozen binder

Fixes: d579b04a52a1 ("binder: frozen notification")
Cc: stable@vger.kernel.org
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
 drivers/android/binder.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index d955135ee37a..2be9f3559ed7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -6408,6 +6408,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
 	case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
 		seq_printf(m, "%shas cleared death notification\n", prefix);
 		break;
+	case BINDER_WORK_FROZEN_BINDER:
+		seq_printf(m, "%shas frozen binder\n", prefix);
+		break;
 	default:
 		seq_printf(m, "%sunknown work: type %d\n", prefix, w->type);
 		break;
-- 
2.46.0.792.g87dc391469-goog

Re: [PATCH 4/4] binder: fix BINDER_WORK_FROZEN_BINDER debug logs

[Todd Kjos]

On Tue, Sep 24, 2024 at 11:44 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> The BINDER_WORK_FROZEN_BINDER type is not handled in the binder_logs
> entries and it shows up as "unknown work" when logged:
>
>   proc 649
>   context binder-test
>     thread 649: l 00 need_return 0 tr 0
>     ref 13: desc 1 node 8 s 1 w 0 d 0000000053c4c0c3
>     unknown work: type 10
>
> This patch add the freeze work type and is now logged as such:
>
>   proc 637
>   context binder-test
>     thread 637: l 00 need_return 0 tr 0
>     ref 8: desc 1 node 3 s 1 w 0 d 00000000dc39e9c6
>     has frozen binder
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index d955135ee37a..2be9f3559ed7 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6408,6 +6408,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
>         case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
>                 seq_printf(m, "%shas cleared death notification\n", prefix);
>                 break;
> +       case BINDER_WORK_FROZEN_BINDER:
> +               seq_printf(m, "%shas frozen binder\n", prefix);
> +               break;
>         default:
>                 seq_printf(m, "%sunknown work: type %d\n", prefix, w->type);
>                 break;
> --
> 2.46.0.792.g87dc391469-goog
>

Re: [PATCH 3/4] binder: fix freeze UAF in binder_release_work()

[Todd Kjos]

On Tue, Sep 24, 2024 at 11:44 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> When a binder reference is cleaned up, any freeze work queued in the
> associated process should also be removed. Otherwise, the reference is
> freed while its ref->freeze.work is still queued in proc->work leading
> to a use-after-free issue as shown by the following KASAN report:
>
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in binder_release_work+0x398/0x3d0
>   Read of size 8 at addr ffff31600ee91488 by task kworker/5:1/211
>
>   CPU: 5 UID: 0 PID: 211 Comm: kworker/5:1 Not tainted 6.11.0-rc7-00382-gfc6c92196396 #22
>   Hardware name: linux,dummy-virt (DT)
>   Workqueue: events binder_deferred_func
>   Call trace:
>    binder_release_work+0x398/0x3d0
>    binder_deferred_func+0xb60/0x109c
>    process_one_work+0x51c/0xbd4
>    worker_thread+0x608/0xee8
>
>   Allocated by task 703:
>    __kmalloc_cache_noprof+0x130/0x280
>    binder_thread_write+0xdb4/0x42a0
>    binder_ioctl+0x18f0/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>    invoke_syscall+0x6c/0x254
>
>   Freed by task 211:
>    kfree+0xc4/0x230
>    binder_deferred_func+0xae8/0x109c
>    process_one_work+0x51c/0xbd4
>    worker_thread+0x608/0xee8
>   ==================================================================
>
> This commit fixes the issue by ensuring any queued freeze work is removed
> when cleaning up a binder reference.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 8bca2de6fa24..d955135ee37a 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1225,6 +1225,12 @@ static void binder_cleanup_ref_olocked(struct binder_ref *ref)
>                 binder_dequeue_work(ref->proc, &ref->death->work);
>                 binder_stats_deleted(BINDER_STAT_DEATH);
>         }
> +
> +       if (ref->freeze) {
> +               binder_dequeue_work(ref->proc, &ref->freeze->work);
> +               binder_stats_deleted(BINDER_STAT_FREEZE);
> +       }
> +
>         binder_stats_deleted(BINDER_STAT_REF);
>  }
>
> --
> 2.46.0.792.g87dc391469-goog
>

Re: [PATCH 1/4] binder: fix node UAF in binder_add_freeze_work()

[Todd Kjos]

On Tue, Sep 24, 2024 at 11:44 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> In binder_add_freeze_work() we iterate over the proc->nodes with the
> proc->inner_lock held. However, this lock is temporarily dropped in
> order to acquire the node->lock first (lock nesting order). This can
> race with binder_node_release() and trigger a use-after-free:
>
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in _raw_spin_lock+0xe4/0x19c
>   Write of size 4 at addr ffff53c04c29dd04 by task freeze/640
>
>   CPU: 5 UID: 0 PID: 640 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #17
>   Hardware name: linux,dummy-virt (DT)
>   Call trace:
>    _raw_spin_lock+0xe4/0x19c
>    binder_add_freeze_work+0x148/0x478
>    binder_ioctl+0x1e70/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>
>   Allocated by task 637:
>    __kmalloc_cache_noprof+0x12c/0x27c
>    binder_new_node+0x50/0x700
>    binder_transaction+0x35ac/0x6f74
>    binder_thread_write+0xfb8/0x42a0
>    binder_ioctl+0x18f0/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>
>   Freed by task 637:
>    kfree+0xf0/0x330
>    binder_thread_read+0x1e88/0x3a68
>    binder_ioctl+0x16d8/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>   ==================================================================
>
> Fix the race by taking a temporary reference on the node before
> releasing the proc->inner lock. This ensures the node remains alive
> while in use.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 978740537a1a..4d90203ea048 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5552,6 +5552,7 @@ static bool binder_txns_pending_ilocked(struct binder_proc *proc)
>
>  static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
>  {
> +       struct binder_node *prev = NULL;
>         struct rb_node *n;
>         struct binder_ref *ref;
>
> @@ -5560,7 +5561,10 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
>                 struct binder_node *node;
>
>                 node = rb_entry(n, struct binder_node, rb_node);
> +               binder_inc_node_tmpref_ilocked(node);
>                 binder_inner_proc_unlock(proc);
> +               if (prev)
> +                       binder_put_node(prev);
>                 binder_node_lock(node);
>                 hlist_for_each_entry(ref, &node->refs, node_entry) {
>                         /*
> @@ -5586,10 +5590,13 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
>                         }
>                         binder_inner_proc_unlock(ref->proc);
>                 }
> +               prev = node;
>                 binder_node_unlock(node);
>                 binder_inner_proc_lock(proc);
>         }
>         binder_inner_proc_unlock(proc);
> +       if (prev)
> +               binder_put_node(prev);
>  }
>
>  static int binder_ioctl_freeze(struct binder_freeze_info *info,
> --
> 2.46.0.792.g87dc391469-goog
>

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Todd Kjos]

On Tue, Sep 24, 2024 at 11:44 AM Carlos Llamas <cmllamas@google.com> wrote:
>
> In binder_add_freeze_work() we iterate over the proc->nodes with the
> proc->inner_lock held. However, this lock is temporarily dropped to
> acquire the node->lock first (lock nesting order). This can race with
> binder_deferred_release() which removes the nodes from the proc->nodes
> rbtree and adds them into binder_dead_nodes list. This leads to a broken
> iteration in binder_add_freeze_work() as rb_next() will use data from
> binder_dead_nodes, triggering an out-of-bounds access:
>
>   ==================================================================
>   BUG: KASAN: global-out-of-bounds in rb_next+0xfc/0x124
>   Read of size 8 at addr ffffcb84285f7170 by task freeze/660
>
>   CPU: 8 UID: 0 PID: 660 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #18
>   Hardware name: linux,dummy-virt (DT)
>   Call trace:
>    rb_next+0xfc/0x124
>    binder_add_freeze_work+0x344/0x534
>    binder_ioctl+0x1e70/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>
>   The buggy address belongs to the variable:
>    binder_dead_nodes+0x10/0x40
>   [...]
>   ==================================================================
>
> This is possible because proc->nodes (rbtree) and binder_dead_nodes
> (list) share entries in binder_node through a union:
>
>         struct binder_node {
>         [...]
>                 union {
>                         struct rb_node rb_node;
>                         struct hlist_node dead_node;
>                 };
>
> Fix the race by checking that the proc is still alive. If not, simply
> break out of the iteration.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

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

> ---
>  drivers/android/binder.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 4d90203ea048..8bca2de6fa24 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5593,6 +5593,8 @@ static void binder_add_freeze_work(struct binder_proc *proc, bool is_frozen)
>                 prev = node;
>                 binder_node_unlock(node);
>                 binder_inner_proc_lock(proc);
> +               if (proc->is_dead)
> +                       break;
>         }
>         binder_inner_proc_unlock(proc);
>         if (prev)
> --
> 2.46.0.792.g87dc391469-goog
>

Re: [PATCH 4/4] binder: fix BINDER_WORK_FROZEN_BINDER debug logs

[Alice Ryhl]

On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> The BINDER_WORK_FROZEN_BINDER type is not handled in the binder_logs
> entries and it shows up as "unknown work" when logged:
>
>   proc 649
>   context binder-test
>     thread 649: l 00 need_return 0 tr 0
>     ref 13: desc 1 node 8 s 1 w 0 d 0000000053c4c0c3
>     unknown work: type 10
>
> This patch add the freeze work type and is now logged as such:
>
>   proc 637
>   context binder-test
>     thread 637: l 00 need_return 0 tr 0
>     ref 8: desc 1 node 3 s 1 w 0 d 00000000dc39e9c6
>     has frozen binder
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>
> ---
>  drivers/android/binder.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index d955135ee37a..2be9f3559ed7 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -6408,6 +6408,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
>         case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
>                 seq_printf(m, "%shas cleared death notification\n", prefix);
>                 break;
> +       case BINDER_WORK_FROZEN_BINDER:
> +               seq_printf(m, "%shas frozen binder\n", prefix);
> +               break;

What about BINDER_WORK_CLEAR_FREEZE_NOTIFICATION?

Alice

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Alice Ryhl]

On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> In binder_add_freeze_work() we iterate over the proc->nodes with the
> proc->inner_lock held. However, this lock is temporarily dropped to
> acquire the node->lock first (lock nesting order). This can race with
> binder_deferred_release() which removes the nodes from the proc->nodes
> rbtree and adds them into binder_dead_nodes list. This leads to a broken
> iteration in binder_add_freeze_work() as rb_next() will use data from
> binder_dead_nodes, triggering an out-of-bounds access:
>
>   ==================================================================
>   BUG: KASAN: global-out-of-bounds in rb_next+0xfc/0x124
>   Read of size 8 at addr ffffcb84285f7170 by task freeze/660
>
>   CPU: 8 UID: 0 PID: 660 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #18
>   Hardware name: linux,dummy-virt (DT)
>   Call trace:
>    rb_next+0xfc/0x124
>    binder_add_freeze_work+0x344/0x534
>    binder_ioctl+0x1e70/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>
>   The buggy address belongs to the variable:
>    binder_dead_nodes+0x10/0x40
>   [...]
>   ==================================================================
>
> This is possible because proc->nodes (rbtree) and binder_dead_nodes
> (list) share entries in binder_node through a union:
>
>         struct binder_node {
>         [...]
>                 union {
>                         struct rb_node rb_node;
>                         struct hlist_node dead_node;
>                 };
>
> Fix the race by checking that the proc is still alive. If not, simply
> break out of the iteration.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

This change LGTM.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

I reviewed some other code paths to verify whether there are other
problems with processes dying concurrently with operations on freeze
notifications. I didn't notice any other memory safety issues, but I
noticed that binder_request_freeze_notification returns EINVAL if you
try to use it with a node from a dead process. That seems problematic,
as this means that there's no way to invoke that command without
risking an EINVAL error if the remote process dies. We should not
return EINVAL errors on correct usage of the driver.

Alice

Re: [PATCH 1/4] binder: fix node UAF in binder_add_freeze_work()

[Alice Ryhl]

On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> In binder_add_freeze_work() we iterate over the proc->nodes with the
> proc->inner_lock held. However, this lock is temporarily dropped in
> order to acquire the node->lock first (lock nesting order). This can
> race with binder_node_release() and trigger a use-after-free:
>
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in _raw_spin_lock+0xe4/0x19c
>   Write of size 4 at addr ffff53c04c29dd04 by task freeze/640
>
>   CPU: 5 UID: 0 PID: 640 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #17
>   Hardware name: linux,dummy-virt (DT)
>   Call trace:
>    _raw_spin_lock+0xe4/0x19c
>    binder_add_freeze_work+0x148/0x478
>    binder_ioctl+0x1e70/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>
>   Allocated by task 637:
>    __kmalloc_cache_noprof+0x12c/0x27c
>    binder_new_node+0x50/0x700
>    binder_transaction+0x35ac/0x6f74
>    binder_thread_write+0xfb8/0x42a0
>    binder_ioctl+0x18f0/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>
>   Freed by task 637:
>    kfree+0xf0/0x330
>    binder_thread_read+0x1e88/0x3a68
>    binder_ioctl+0x16d8/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>   ==================================================================
>
> Fix the race by taking a temporary reference on the node before
> releasing the proc->inner lock. This ensures the node remains alive
> while in use.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 3/4] binder: fix freeze UAF in binder_release_work()

[Alice Ryhl]

On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> When a binder reference is cleaned up, any freeze work queued in the
> associated process should also be removed. Otherwise, the reference is
> freed while its ref->freeze.work is still queued in proc->work leading
> to a use-after-free issue as shown by the following KASAN report:
>
>   ==================================================================
>   BUG: KASAN: slab-use-after-free in binder_release_work+0x398/0x3d0
>   Read of size 8 at addr ffff31600ee91488 by task kworker/5:1/211
>
>   CPU: 5 UID: 0 PID: 211 Comm: kworker/5:1 Not tainted 6.11.0-rc7-00382-gfc6c92196396 #22
>   Hardware name: linux,dummy-virt (DT)
>   Workqueue: events binder_deferred_func
>   Call trace:
>    binder_release_work+0x398/0x3d0
>    binder_deferred_func+0xb60/0x109c
>    process_one_work+0x51c/0xbd4
>    worker_thread+0x608/0xee8
>
>   Allocated by task 703:
>    __kmalloc_cache_noprof+0x130/0x280
>    binder_thread_write+0xdb4/0x42a0
>    binder_ioctl+0x18f0/0x25ac
>    __arm64_sys_ioctl+0x124/0x190
>    invoke_syscall+0x6c/0x254
>
>   Freed by task 211:
>    kfree+0xc4/0x230
>    binder_deferred_func+0xae8/0x109c
>    process_one_work+0x51c/0xbd4
>    worker_thread+0x608/0xee8
>   ==================================================================
>
> This commit fixes the issue by ensuring any queued freeze work is removed
> when cleaning up a binder reference.
>
> Fixes: d579b04a52a1 ("binder: frozen notification")
> Cc: stable@vger.kernel.org
> Signed-off-by: Carlos Llamas <cmllamas@google.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 4/4] binder: fix BINDER_WORK_FROZEN_BINDER debug logs

[Carlos Llamas]

On Wed, Sep 25, 2024 at 09:36:10AM +0200, 'Alice Ryhl' via kernel-team wrote:
> On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > The BINDER_WORK_FROZEN_BINDER type is not handled in the binder_logs
> > entries and it shows up as "unknown work" when logged:
> >
> >   proc 649
> >   context binder-test
> >     thread 649: l 00 need_return 0 tr 0
> >     ref 13: desc 1 node 8 s 1 w 0 d 0000000053c4c0c3
> >     unknown work: type 10
> >
> > This patch add the freeze work type and is now logged as such:
> >
> >   proc 637
> >   context binder-test
> >     thread 637: l 00 need_return 0 tr 0
> >     ref 8: desc 1 node 3 s 1 w 0 d 00000000dc39e9c6
> >     has frozen binder
> >
> > Fixes: d579b04a52a1 ("binder: frozen notification")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > ---
> >  drivers/android/binder.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index d955135ee37a..2be9f3559ed7 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -6408,6 +6408,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
> >         case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
> >                 seq_printf(m, "%shas cleared death notification\n", prefix);
> >                 break;
> > +       case BINDER_WORK_FROZEN_BINDER:
> > +               seq_printf(m, "%shas frozen binder\n", prefix);
> > +               break;
> 
> What about BINDER_WORK_CLEAR_FREEZE_NOTIFICATION?

Oh, you are right! We also need this type here. I haven't played with
the clear notification path just yet (as you can tell). Thanks for
pointing it out though, I'll send a v2.

Looking closer, I see that BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT
is also missing, so I'll send a separate patch for that too.

Thanks,
--
Carlos Llamas

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Carlos Llamas]

On Wed, Sep 25, 2024 at 10:02:51AM +0200, 'Alice Ryhl' via kernel-team wrote:
> On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > In binder_add_freeze_work() we iterate over the proc->nodes with the
> > proc->inner_lock held. However, this lock is temporarily dropped to
> > acquire the node->lock first (lock nesting order). This can race with
> > binder_deferred_release() which removes the nodes from the proc->nodes
> > rbtree and adds them into binder_dead_nodes list. This leads to a broken
> > iteration in binder_add_freeze_work() as rb_next() will use data from
> > binder_dead_nodes, triggering an out-of-bounds access:
> >
> >   ==================================================================
> >   BUG: KASAN: global-out-of-bounds in rb_next+0xfc/0x124
> >   Read of size 8 at addr ffffcb84285f7170 by task freeze/660
> >
> >   CPU: 8 UID: 0 PID: 660 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #18
> >   Hardware name: linux,dummy-virt (DT)
> >   Call trace:
> >    rb_next+0xfc/0x124
> >    binder_add_freeze_work+0x344/0x534
> >    binder_ioctl+0x1e70/0x25ac
> >    __arm64_sys_ioctl+0x124/0x190
> >
> >   The buggy address belongs to the variable:
> >    binder_dead_nodes+0x10/0x40
> >   [...]
> >   ==================================================================
> >
> > This is possible because proc->nodes (rbtree) and binder_dead_nodes
> > (list) share entries in binder_node through a union:
> >
> >         struct binder_node {
> >         [...]
> >                 union {
> >                         struct rb_node rb_node;
> >                         struct hlist_node dead_node;
> >                 };
> >
> > Fix the race by checking that the proc is still alive. If not, simply
> > break out of the iteration.
> >
> > Fixes: d579b04a52a1 ("binder: frozen notification")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> 
> This change LGTM.
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
> I reviewed some other code paths to verify whether there are other
> problems with processes dying concurrently with operations on freeze
> notifications. I didn't notice any other memory safety issues, but I

Yeah most other paths are protected with binder_procs_lock mutex.

> noticed that binder_request_freeze_notification returns EINVAL if you
> try to use it with a node from a dead process. That seems problematic,
> as this means that there's no way to invoke that command without
> risking an EINVAL error if the remote process dies. We should not
> return EINVAL errors on correct usage of the driver.

Agreed, this should probably be -ESRCH or something. I'll add it to v2,
thanks for the suggestion.

Cheers,
Carlos Llamas

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Alice Ryhl]

On Wed, Sep 25, 2024 at 7:48 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Wed, Sep 25, 2024 at 10:02:51AM +0200, 'Alice Ryhl' via kernel-team wrote:
> > On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > In binder_add_freeze_work() we iterate over the proc->nodes with the
> > > proc->inner_lock held. However, this lock is temporarily dropped to
> > > acquire the node->lock first (lock nesting order). This can race with
> > > binder_deferred_release() which removes the nodes from the proc->nodes
> > > rbtree and adds them into binder_dead_nodes list. This leads to a broken
> > > iteration in binder_add_freeze_work() as rb_next() will use data from
> > > binder_dead_nodes, triggering an out-of-bounds access:
> > >
> > >   ==================================================================
> > >   BUG: KASAN: global-out-of-bounds in rb_next+0xfc/0x124
> > >   Read of size 8 at addr ffffcb84285f7170 by task freeze/660
> > >
> > >   CPU: 8 UID: 0 PID: 660 Comm: freeze Not tainted 6.11.0-07343-ga727812a8d45 #18
> > >   Hardware name: linux,dummy-virt (DT)
> > >   Call trace:
> > >    rb_next+0xfc/0x124
> > >    binder_add_freeze_work+0x344/0x534
> > >    binder_ioctl+0x1e70/0x25ac
> > >    __arm64_sys_ioctl+0x124/0x190
> > >
> > >   The buggy address belongs to the variable:
> > >    binder_dead_nodes+0x10/0x40
> > >   [...]
> > >   ==================================================================
> > >
> > > This is possible because proc->nodes (rbtree) and binder_dead_nodes
> > > (list) share entries in binder_node through a union:
> > >
> > >         struct binder_node {
> > >         [...]
> > >                 union {
> > >                         struct rb_node rb_node;
> > >                         struct hlist_node dead_node;
> > >                 };
> > >
> > > Fix the race by checking that the proc is still alive. If not, simply
> > > break out of the iteration.
> > >
> > > Fixes: d579b04a52a1 ("binder: frozen notification")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> >
> > This change LGTM.
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >
> > I reviewed some other code paths to verify whether there are other
> > problems with processes dying concurrently with operations on freeze
> > notifications. I didn't notice any other memory safety issues, but I
>
> Yeah most other paths are protected with binder_procs_lock mutex.
>
> > noticed that binder_request_freeze_notification returns EINVAL if you
> > try to use it with a node from a dead process. That seems problematic,
> > as this means that there's no way to invoke that command without
> > risking an EINVAL error if the remote process dies. We should not
> > return EINVAL errors on correct usage of the driver.
>
> Agreed, this should probably be -ESRCH or something. I'll add it to v2,
> thanks for the suggestion.

Well, maybe? I think it's best to not return errnos from these
commands at all, as they obscure how many commands were processed.

Since the node still exists even if the process dies, perhaps we can
just let you create the freeze notification even if it's dead? We can
make it end up in the same state as if you request a freeze
notification and the process then dies afterwards.

Alice

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Carlos Llamas]

On Wed, Sep 25, 2024 at 07:52:37PM +0200, Alice Ryhl wrote:
> > > I reviewed some other code paths to verify whether there are other
> > > problems with processes dying concurrently with operations on freeze
> > > notifications. I didn't notice any other memory safety issues, but I
> >
> > Yeah most other paths are protected with binder_procs_lock mutex.
> >
> > > noticed that binder_request_freeze_notification returns EINVAL if you
> > > try to use it with a node from a dead process. That seems problematic,
> > > as this means that there's no way to invoke that command without
> > > risking an EINVAL error if the remote process dies. We should not
> > > return EINVAL errors on correct usage of the driver.
> >
> > Agreed, this should probably be -ESRCH or something. I'll add it to v2,
> > thanks for the suggestion.
> 
> Well, maybe? I think it's best to not return errnos from these
> commands at all, as they obscure how many commands were processed.

This is problematic, particularly when it's a multi-command buffer.
Userspace doesn't really know which one failed and if any of them
succeeded. Agreed.

> 
> Since the node still exists even if the process dies, perhaps we can
> just let you create the freeze notification even if it's dead? We can
> make it end up in the same state as if you request a freeze
> notification and the process then dies afterwards.

It's a dead node, there is no process associated with it. It would be
incorrect to setup the notification as it doesn't have a frozen status
anymore. We can't determine the ref->node->proc->is_frozen?

We could silently fail and skip the notification, but I don't know if
userspace will attempt to release it later... and fail with EINVAL.

--
Carlos Llamas

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Carlos Llamas]

On Wed, Sep 25, 2024 at 06:06:30PM +0000, Carlos Llamas wrote:
> On Wed, Sep 25, 2024 at 07:52:37PM +0200, Alice Ryhl wrote:
> > > > I reviewed some other code paths to verify whether there are other
> > > > problems with processes dying concurrently with operations on freeze
> > > > notifications. I didn't notice any other memory safety issues, but I
> > >
> > > Yeah most other paths are protected with binder_procs_lock mutex.
> > >
> > > > noticed that binder_request_freeze_notification returns EINVAL if you
> > > > try to use it with a node from a dead process. That seems problematic,
> > > > as this means that there's no way to invoke that command without
> > > > risking an EINVAL error if the remote process dies. We should not
> > > > return EINVAL errors on correct usage of the driver.
> > >
> > > Agreed, this should probably be -ESRCH or something. I'll add it to v2,
> > > thanks for the suggestion.
> > 
> > Well, maybe? I think it's best to not return errnos from these
> > commands at all, as they obscure how many commands were processed.
> 
> This is problematic, particularly when it's a multi-command buffer.
> Userspace doesn't really know which one failed and if any of them
> succeeded. Agreed.
> 
> > 
> > Since the node still exists even if the process dies, perhaps we can
> > just let you create the freeze notification even if it's dead? We can
> > make it end up in the same state as if you request a freeze
> > notification and the process then dies afterwards.
> 
> It's a dead node, there is no process associated with it. It would be
> incorrect to setup the notification as it doesn't have a frozen status
> anymore. We can't determine the ref->node->proc->is_frozen?
> 
> We could silently fail and skip the notification, but I don't know if
> userspace will attempt to release it later... and fail with EINVAL.
> 

FWIW, we already propagate errors when the target process or node is
dead in some other places. It makes sense to me to let userspace know
that we couldn't setup the frozen notification.

Re: [PATCH 4/4] binder: fix BINDER_WORK_FROZEN_BINDER debug logs

[Carlos Llamas]

On Wed, Sep 25, 2024 at 05:25:42PM +0000, Carlos Llamas wrote:
> On Wed, Sep 25, 2024 at 09:36:10AM +0200, 'Alice Ryhl' via kernel-team wrote:
> > On Tue, Sep 24, 2024 at 8:44 PM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > The BINDER_WORK_FROZEN_BINDER type is not handled in the binder_logs
> > > entries and it shows up as "unknown work" when logged:
> > >
> > >   proc 649
> > >   context binder-test
> > >     thread 649: l 00 need_return 0 tr 0
> > >     ref 13: desc 1 node 8 s 1 w 0 d 0000000053c4c0c3
> > >     unknown work: type 10
> > >
> > > This patch add the freeze work type and is now logged as such:
> > >
> > >   proc 637
> > >   context binder-test
> > >     thread 637: l 00 need_return 0 tr 0
> > >     ref 8: desc 1 node 3 s 1 w 0 d 00000000dc39e9c6
> > >     has frozen binder
> > >
> > > Fixes: d579b04a52a1 ("binder: frozen notification")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Carlos Llamas <cmllamas@google.com>
> > > ---
> > >  drivers/android/binder.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index d955135ee37a..2be9f3559ed7 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -6408,6 +6408,9 @@ static void print_binder_work_ilocked(struct seq_file *m,
> > >         case BINDER_WORK_CLEAR_DEATH_NOTIFICATION:
> > >                 seq_printf(m, "%shas cleared death notification\n", prefix);
> > >                 break;
> > > +       case BINDER_WORK_FROZEN_BINDER:
> > > +               seq_printf(m, "%shas frozen binder\n", prefix);
> > > +               break;
> > 
> > What about BINDER_WORK_CLEAR_FREEZE_NOTIFICATION?
> 
> Oh, you are right! We also need this type here. I haven't played with
> the clear notification path just yet (as you can tell). Thanks for
> pointing it out though, I'll send a v2.
> 
> Looking closer, I see that BINDER_WORK_TRANSACTION_ONEWAY_SPAM_SUSPECT
> is also missing, so I'll send a separate patch for that too.

After playing with clear-freeze I've also found a memleak. This work is
not being released unless it is processed. I'll add one more patch to
the v2 series fixing it (tomorrow).

unreferenced object 0xffff03ced63cd280 (size 64):
  comm "binder-util", pid 947, jiffies 4295183379
  hex dump (first 32 bytes):
    80 d2 3c d6 ce 03 ff ff 80 d2 3c d6 ce 03 ff ff  ..<.......<.....
    0b 00 00 00 00 00 00 00 3c 1f 4a 00 00 00 00 00  ........<.J.....
  backtrace (crc 718d20df):
    [<00000000188a0477>] kmemleak_alloc+0x34/0x40
    [<00000000fe1c45bc>] __kmalloc_cache_noprof+0x208/0x280
    [<000000005ebe1c53>] binder_thread_write+0xdec/0x439c
    [<000000004b0e4ffa>] binder_ioctl+0x1b68/0x22cc
    [<000000001786d65d>] __arm64_sys_ioctl+0x124/0x190
    [<00000000fb1e34f9>] invoke_syscall+0x6c/0x254
    [<00000000826a09b6>] el0_svc_common.constprop.0+0xac/0x230
    [<000000004389c74d>] do_el0_svc+0x40/0x58
    [<000000008b9dd949>] el0_svc+0x38/0x78
    [<00000000c66f77b2>] el0t_64_sync_handler+0x120/0x12c
    [<00000000a4cd389b>] el0t_64_sync+0x190/0x194> 

--
Carlos Llamas

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Alice Ryhl]

On Wed, Sep 25, 2024 at 8:06 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Wed, Sep 25, 2024 at 07:52:37PM +0200, Alice Ryhl wrote:
> > > > I reviewed some other code paths to verify whether there are other
> > > > problems with processes dying concurrently with operations on freeze
> > > > notifications. I didn't notice any other memory safety issues, but I
> > >
> > > Yeah most other paths are protected with binder_procs_lock mutex.
> > >
> > > > noticed that binder_request_freeze_notification returns EINVAL if you
> > > > try to use it with a node from a dead process. That seems problematic,
> > > > as this means that there's no way to invoke that command without
> > > > risking an EINVAL error if the remote process dies. We should not
> > > > return EINVAL errors on correct usage of the driver.
> > >
> > > Agreed, this should probably be -ESRCH or something. I'll add it to v2,
> > > thanks for the suggestion.
> >
> > Well, maybe? I think it's best to not return errnos from these
> > commands at all, as they obscure how many commands were processed.
>
> This is problematic, particularly when it's a multi-command buffer.
> Userspace doesn't really know which one failed and if any of them
> succeeded. Agreed.
>
> >
> > Since the node still exists even if the process dies, perhaps we can
> > just let you create the freeze notification even if it's dead? We can
> > make it end up in the same state as if you request a freeze
> > notification and the process then dies afterwards.
>
> It's a dead node, there is no process associated with it. It would be
> incorrect to setup the notification as it doesn't have a frozen status
> anymore. We can't determine the ref->node->proc->is_frozen?
>
> We could silently fail and skip the notification, but I don't know if
> userspace will attempt to release it later... and fail with EINVAL.

I mean, userspace *has* to be able to deal with the case where the
process dies *right after* the freeze notification is registered. If
we make the behavior where it's already dead be the same as that case,
then we're not giving userspace any new things it needs to care about.

Alice

Re: [PATCH 2/4] binder: fix OOB in binder_add_freeze_work()

[Carlos Llamas]

On Thu, Sep 26, 2024 at 10:06:14AM +0200, Alice Ryhl wrote:
> On Wed, Sep 25, 2024 at 8:06 PM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > On Wed, Sep 25, 2024 at 07:52:37PM +0200, Alice Ryhl wrote:
> > > > > I reviewed some other code paths to verify whether there are other
> > > > > problems with processes dying concurrently with operations on freeze
> > > > > notifications. I didn't notice any other memory safety issues, but I
> > > >
> > > > Yeah most other paths are protected with binder_procs_lock mutex.
> > > >
> > > > > noticed that binder_request_freeze_notification returns EINVAL if you
> > > > > try to use it with a node from a dead process. That seems problematic,
> > > > > as this means that there's no way to invoke that command without
> > > > > risking an EINVAL error if the remote process dies. We should not
> > > > > return EINVAL errors on correct usage of the driver.
> > > >
> > > > Agreed, this should probably be -ESRCH or something. I'll add it to v2,
> > > > thanks for the suggestion.
> > >
> > > Well, maybe? I think it's best to not return errnos from these
> > > commands at all, as they obscure how many commands were processed.
> >
> > This is problematic, particularly when it's a multi-command buffer.
> > Userspace doesn't really know which one failed and if any of them
> > succeeded. Agreed.
> >
> > >
> > > Since the node still exists even if the process dies, perhaps we can
> > > just let you create the freeze notification even if it's dead? We can
> > > make it end up in the same state as if you request a freeze
> > > notification and the process then dies afterwards.
> >
> > It's a dead node, there is no process associated with it. It would be
> > incorrect to setup the notification as it doesn't have a frozen status
> > anymore. We can't determine the ref->node->proc->is_frozen?
> >
> > We could silently fail and skip the notification, but I don't know if
> > userspace will attempt to release it later... and fail with EINVAL.
> 
> I mean, userspace *has* to be able to deal with the case where the
> process dies *right after* the freeze notification is registered. If
> we make the behavior where it's already dead be the same as that case,
> then we're not giving userspace any new things it needs to care about.

This is a fair point. To make it happen though, we would need to modify
the behavior of the request a bit. If the node is dead, we could still
attach the freeze notification to the reference but then we would skip
sending the "current" frozen state. This last bit won't be guaranteed
anymore. I _suppose_ this is ok, since as you mention, userspace should
have to deal with the process dying anyway.

I honestly don't really like this "fake successful" approach but then we
don't handle driver errors very well either. So it might be worth it to
avoid propagating this "dead node" error if we can. I'll do this for v2.

Thanks,
Carlos Llamas