binder: fix UAF in binder_thread_release()
Subsystems (2)
Carlos Llamas
maintainerPATCH 1/1 binder: fix UAF in binder_thread_release()
When a thread exits, binder_thread_release() walks its transaction stack to clear the t->from and t->to_proc that correspond with the exiting thread. However, a process dying in parallel might attempt to kfree some of these transactions. And if one of them has no associated t->to_proc, the t->to_proc->inner_lock will not be acquired.
This means that transaction accesses in binder_thread_release() after t->to_proc has been cleared might race with binder_free_transaction() and cause a use-after-free error as reported by KASAN:
==================================================================
BUG: KASAN: slab-use-after-free in binder_thread_release+0x5d0/0x798 Write of size 8 at addr ffff000016627500 by task X/715 CPU: 17 UID: 0 PID: 715 Comm: X Not tainted 7.1.0-rc5-00149-g8fde5d1d47f6 #30 PREEMPT Hardware name: linux,dummy-virt (DT) Call trace: binder_thread_release+0x5d0/0x798 binder_ioctl+0x12c0/0x299c [...] Allocated by task 717 on cpu 18 at 67.267803s: __kasan_kmalloc+0xa0/0xbc __kmalloc_cache_noprof+0x174/0x444 binder_transaction+0x554/0x8150 binder_thread_write+0xa30/0x4354 binder_ioctl+0x20f0/0x299c [...] Freed by task 202 on cpu 18 at 90.416221s: __kasan_slab_free+0x58/0x80 kfree+0x1a0/0x4a4 binder_free_transaction+0x150/0x294 binder_send_failed_reply+0x398/0x6d8 binder_release_work+0x3e4/0x4ec binder_deferred_func+0xbd8/0x104c [...] ==================================================================
In order to avoid this, make sure that binder_free_transaction() reads the t->to_proc under the transaction lock. This will serialize the transaction release with the accesses in binder_thread_release(). Plus, it matches the documented locking rules for @to_proc.
--- drivers/android/binder.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/android/binder.c +5 -1
@@ -1658,7 +1658,11 @@ static void binder_txn_latency_free(struct binder_transaction *t)static void binder_free_transaction (struct binder_transaction *t) { -struct binder_proc *target_proc = t->to_proc; +struct binder_proc *target_proc; + +spin_lock (&t->lock); + target_proc = t->to_proc; +spin_unlock (&t->lock);if (target_proc) {binder_inner_proc_lock (target_proc);
Signature · 1 line
2.54.0.1032.g2f8565e1d1-goog
Quoted 56 lines · Carlos Llamas
When a thread exits,
binder_thread_release()walks its transaction stack to clear thet->fromandt->to_procthat correspond with the exiting thread. However, a process dying in parallel might attempt to kfree some of these transactions. And if one of them has no associatedt->to_proc, thet->to_proc->inner_lockwill not be acquired.This means that transaction accesses in
binder_thread_release()aftert->to_prochas been cleared might race withbinder_free_transaction()and cause a use-after-free error as reported by KASAN:==================================================================
BUG: KASAN: slab-use-after-free in binder_thread_release+0x5d0/0x798 Write of size 8 at addr ffff000016627500 by task X/715 CPU: 17 UID: 0 PID: 715 Comm: X Not tainted 7.1.0-rc5-00149-g8fde5d1d47f6 #30 PREEMPT Hardware name: linux,dummy-virt (DT) Call trace: binder_thread_release+0x5d0/0x798 binder_ioctl+0x12c0/0x299c [...] Allocated by task 717 on cpu 18 at 67.267803s: __kasan_kmalloc+0xa0/0xbc __kmalloc_cache_noprof+0x174/0x444 binder_transaction+0x554/0x8150 binder_thread_write+0xa30/0x4354 binder_ioctl+0x20f0/0x299c [...] Freed by task 202 on cpu 18 at 90.416221s: __kasan_slab_free+0x58/0x80 kfree+0x1a0/0x4a4 binder_free_transaction+0x150/0x294 binder_send_failed_reply+0x398/0x6d8 binder_release_work+0x3e4/0x4ec binder_deferred_func+0xbd8/0x104c [...] ==================================================================In order to avoid this, make sure that
binder_free_transaction()reads thet->to_procunder the transaction lock. This will serialize the transaction release with the accesses inbinder_thread_release(). Plus, it matches the documented locking rules for @to_proc.Fixes 7a4408c6bd3e ("binder: make sure accesses to proc/thread are safe")Signed-off-by Carlos Llamas <[email protected]>--- drivers/android/binder.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/android/binder.c +0
-0
drivers/android/binder.c static void binder_txn_latency_free(struct binder_transaction *t) · line 1665
Quoted 9 lines
drivers/android/binder.c +5
-1@@ -1658,7 +1658,11 @@ static void binder_txn_latency_free(struct binder_transaction *t)static void binder_free_transaction (struct binder_transaction *t) { -struct binder_proc *target_proc = t->to_proc; +struct binder_proc *target_proc; + +spin_lock (&t->lock); + target_proc = t->to_proc; +spin_unlock (&t->lock);
Although I don't think this fixes all issues here, as we discussed more in private, this does fix the specific UAF referenced in this patch, so:
The logic is that either binder_free_transaction() reads a non-null target_proc, in which case we take the inner proc lock and fully synchronize with the entirety of binder_thread_release(), or we read a null target_proc in which case the transaction spinlock ensures that we wait for the part of binder_thread_release() touching this particular 'struct binder_transaction'.
Alice
Quoted 5 lines
drivers/android/binder.c +0
-0if (target_proc) {binder_inner_proc_lock (target_proc);Signature · 1 line
2.54.0.1032.g2f8565e1d1-goog
Carlos Llamas
maintainerQuoted 4 lines · Alice Ryhl
Although I don't think this fixes all issues here, as we discussed more in private, this does fix the specific UAF referenced in this patch, so:
Reviewed-by Alice Ryhl <[email protected]>
Thanks. I've managed to reproduce the UAF that you pointed out. It was a bit tricky but I have a fix for it now. C binder just keeps proving we should switch over to Rust ASAP.
It doesn't seem like Greg has picked up this patch yet, so I'll resend this as v2 along with a separate fix for the UAF you reported.
Cheers,
Carlos Llamas