linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels
@ 2019-10-07 20:49 Jann Horn
  2019-10-07 21:04 ` Todd Kjos
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jann Horn @ 2019-10-07 20:49 UTC (permalink / raw)
  To: Todd Kjos, Martijn Coenen, Arve Hjønnevåg,
	Joel Fernandes, Christian Brauner
  Cc: open list:ANDROID DRIVERS, kernel list, Greg Kroah-Hartman

Hi!

There is a use-after-free read in print_binder_transaction_log_entry()
on ANDROID_BINDERFS kernels because
print_binder_transaction_log_entry() prints the char* e->context_name
as string, and if the transaction occurred on a binder device from
binderfs, e->context_name belongs to the binder device and is freed
when the inode disappears.

Luckily this shouldn't have security implications, since:

a) reading the binder transaction log is already a pretty privileged operation
b) I can't find any actual users of ANDROID_BINDERFS

I guess there are three ways to fix it:
1) Create a new shared global spinlock for binderfs_evict_inode() and
binder_transaction_log_show(), and let binderfs_evict_inode() scan the
transaction log for pointers to its name and replace them with
pointers to a statically-allocated string "{DELETED}" or something
like that.
2) Let the transaction log contain non-reusable device identifiers
instead of name pointers, and let print_binder_transaction_log_entry()
look them up in something like a hashtable.
3) Just copy the name into the transaction log every time.

I'm not sure which one is better, or whether there's a nicer fourth
option, so I'm leaving writing a patch for this to y'all.


Trigger instructions (requires you to have some helpers that can
register a context manager and send some transaction to it):
==============
root@test:/home/user# mkdir /tmp/binder
root@test:/home/user# mount -t binder -o stats=global /dev/null /tmp/binder
root@test:/home/user# ls -l /tmp/binder
total 0
crw------- 1 root root 248, 1 Oct  7 20:34 binder
crw------- 1 root root 248, 0 Oct  7 20:34 binder-control
drwxr-xr-x 3 root root      0 Oct  7 20:34 binder_logs
crw------- 1 root root 248, 2 Oct  7 20:34 hwbinder
crw------- 1 root root 248, 3 Oct  7 20:34 vndbinder
root@test:/home/user# ln -s /tmp/binder/binder /dev/binder
[run some simple binder demo code to temporarily register a context
manager and send a binder transaction]
root@test:/home/user# rm /tmp/binder/binder
root@test:/home/user# cat /tmp/binder/binder_logs/transaction_log
2: call  from 2277:2277 to 2273:0 context @������� node 1 handle 0
size 24:8 ret 0/0 l=0
5: call  from 2273:2273 to 2277:2277 context @������� node 3 handle 1
size 0:0 ret 0/0 l=0
6: reply from 2277:2277 to 2273:2273 context @������� node 0 handle 0
size 4:0 ret 0/0 l=0
7: reply from 2273:2273 to 2277:2277 context @������� node 0 handle 0
size 4:0 ret 0/0 l=0
root@test:/home/user#
==============

ASAN splat:
[  333.300753] ==================================================================
[  333.303197] BUG: KASAN: use-after-free in string_nocheck+0x9d/0x160
[  333.305081] Read of size 1 at addr ffff8880b0981258 by task cat/2279

[  333.307415] CPU: 1 PID: 2279 Comm: cat Not tainted 5.4.0-rc1+ #513
[  333.309304] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.12.0-1 04/01/2014
[  333.310987] Call Trace:
[  333.312032]  dump_stack+0x7c/0xc0
[  333.312581]  ? string_nocheck+0x9d/0x160
[  333.313157]  print_address_description.constprop.7+0x36/0x50
[  333.314030]  ? string_nocheck+0x9d/0x160
[  333.314603]  ? string_nocheck+0x9d/0x160
[  333.315236]  __kasan_report.cold.10+0x1a/0x35
[  333.315972]  ? string_nocheck+0x9d/0x160
[  333.316545]  kasan_report+0xe/0x20
[  333.317104]  string_nocheck+0x9d/0x160
[  333.317652]  ? widen_string+0x160/0x160
[  333.318270]  ? string_nocheck+0x160/0x160
[  333.318857]  ? unwind_get_return_address+0x2a/0x40
[  333.319636]  ? profile_setup.cold.9+0x96/0x96
[  333.320359]  string+0xb6/0xc0
[  333.320800]  ? hex_string+0x280/0x280
[  333.321398]  vsnprintf+0x20c/0x780
[  333.321898]  ? num_to_str+0x180/0x180
[  333.322503]  ? __kasan_kmalloc.constprop.6+0xc1/0xd0
[  333.323235]  ? vfs_read+0xbc/0x1e0
[  333.323814]  ? ksys_read+0xb5/0x150
[  333.324323]  ? do_syscall_64+0xb9/0x3b0
[  333.324948]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  333.325756]  seq_vprintf+0x78/0xb0
[  333.326253]  seq_printf+0x96/0xc0
[  333.327132]  ? seq_vprintf+0xb0/0xb0
[  333.327678]  ? match_held_lock+0x2e/0x240
[  333.328450]  binder_transaction_log_show+0x237/0x2d0
[  333.329163]  seq_read+0x266/0x690
[  333.329705]  vfs_read+0xbc/0x1e0
[  333.330178]  ksys_read+0xb5/0x150
[  333.330724]  ? kernel_write+0xb0/0xb0
[  333.331257]  ? trace_hardirqs_off_caller+0x57/0x130
[  333.332045]  ? mark_held_locks+0x29/0xa0
[  333.332678]  ? do_syscall_64+0x6b/0x3b0
[  333.333235]  do_syscall_64+0xb9/0x3b0
[  333.333856]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  333.334635] RIP: 0033:0x7fbbb95d4461
[  333.335153] Code: fe ff ff 50 48 8d 3d fe d0 09 00 e8 e9 03 02 00
66 0f 1f 84 00 00 00 00 00 48 8d 05 99 62 0d 00 8b 00 85 c0 75 13 31
c0 0f 05 <48> 3d 00 f0 ff ff 77 57 c3 66 0f 1f 44 00 00 41 54 49 89 d4
55 48
[  333.337950] RSP: 002b:00007ffcbe6438e8 EFLAGS: 00000246 ORIG_RAX:
0000000000000000
[  333.339072] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007fbbb95d4461
[  333.340157] RDX: 0000000000020000 RSI: 00007fbbb9324000 RDI: 0000000000000003
[  333.341320] RBP: 00007fbbb9324000 R08: 00000000ffffffff R09: 0000000000000000
[  333.342454] R10: fffffffffffffb9c R11: 0000000000000246 R12: 00007fbbb9324000
[  333.343550] R13: 0000000000000003 R14: 0000000000000fff R15: 0000000000020000

[  333.344845] Allocated by task 2259:
[  333.345416]  save_stack+0x19/0x80
[  333.345899]  __kasan_kmalloc.constprop.6+0xc1/0xd0
[  333.346636]  __kmalloc_track_caller+0xf4/0x2e0
[  333.347271]  kmemdup+0x17/0x40
[  333.347796]  binderfs_binder_device_create.isra.6+0x217/0x530
[  333.348674]  binderfs_fill_super+0x486/0x81e
[  333.349309]  mount_nodev+0x41/0xb0
[  333.349860]  legacy_get_tree+0x7b/0xc0
[  333.350398]  vfs_get_tree+0x40/0x130
[  333.350970]  do_mount+0xacb/0xea0
[  333.351449]  ksys_mount+0xb1/0xd0
[  333.352007]  __x64_sys_mount+0x5d/0x70
[  333.352545]  do_syscall_64+0xb9/0x3b0
[  333.353144]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

[  333.354144] Freed by task 2278:
[  333.354598]  save_stack+0x19/0x80
[  333.355135]  __kasan_slab_free+0x12e/0x180
[  333.355734]  kfree+0xe6/0x310
[  333.356234]  binderfs_evict_inode+0xb8/0xd0
[  333.356831]  evict+0x16f/0x290
[  333.358081]  do_unlinkat+0x2f6/0x420
[  333.358593]  do_syscall_64+0xb9/0x3b0
[  333.359176]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

[  333.360196] The buggy address belongs to the object at ffff8880b0981258
                which belongs to the cache kmalloc-8 of size 8
[  333.361991] The buggy address is located 0 bytes inside of
                8-byte region [ffff8880b0981258, ffff8880b0981260)
[  333.363796] The buggy address belongs to the page:
[  333.364538] page:ffffea0002c26040 refcount:1 mapcount:0
mapping:ffff8880b6c03c80 index:0x0
[  333.365765] flags: 0x1fffc0000000200(slab)
[  333.366402] raw: 01fffc0000000200 ffffea0002cb1d80 0000001400000014
ffff8880b6c03c80
[  333.367546] raw: 0000000000000000 0000000000aa00aa 00000001ffffffff
0000000000000000
[  333.369030] page dumped because: kasan: bad access detected

[  333.370095] Memory state around the buggy address:
[  333.370824]  ffff8880b0981100: fc fb fc fc fb fc fc fb fc fc fb fc
fc fb fc fc
[  333.371907]  ffff8880b0981180: fb fc fc fb fc fc fb fc fc fb fc fc
fb fc fc fb
[  333.372969] >ffff8880b0981200: fc fc fb fc fc fb fc fc fb fc fc fb
fc fc fb fc
[  333.374033]                                                     ^
[  333.374884]  ffff8880b0981280: fc fb fc fc fb fc fc fb fc fc fb fc
fc fb fc fc
[  333.375957]  ffff8880b0981300: fb fc fc fb fc fc fb fc fc fb fc fc
fb fc fc fb
[  333.377013] ==================================================================

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

* Re: UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels
  2019-10-07 20:49 UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels Jann Horn
@ 2019-10-07 21:04 ` Todd Kjos
  2019-10-07 21:16   ` Hridya Valsaraju
  2019-10-07 21:05 ` Christian Brauner
  2019-10-08 13:01 ` [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry() Christian Brauner
  2 siblings, 1 reply; 15+ messages in thread
From: Todd Kjos @ 2019-10-07 21:04 UTC (permalink / raw)
  To: Jann Horn, Hridya Valsaraju
  Cc: Martijn Coenen, Arve Hjønnevåg, Joel Fernandes,
	Christian Brauner, open list:ANDROID DRIVERS, kernel list,
	Greg Kroah-Hartman

+Hridya Valsaraju


On Mon, Oct 7, 2019 at 1:50 PM Jann Horn <jannh@google.com> wrote:
>
> Hi!
>
> There is a use-after-free read in print_binder_transaction_log_entry()
> on ANDROID_BINDERFS kernels because
> print_binder_transaction_log_entry() prints the char* e->context_name
> as string, and if the transaction occurred on a binder device from
> binderfs, e->context_name belongs to the binder device and is freed
> when the inode disappears.
>
> Luckily this shouldn't have security implications, since:
>
> a) reading the binder transaction log is already a pretty privileged operation
> b) I can't find any actual users of ANDROID_BINDERFS
>
> I guess there are three ways to fix it:
> 1) Create a new shared global spinlock for binderfs_evict_inode() and
> binder_transaction_log_show(), and let binderfs_evict_inode() scan the
> transaction log for pointers to its name and replace them with
> pointers to a statically-allocated string "{DELETED}" or something
> like that.
> 2) Let the transaction log contain non-reusable device identifiers
> instead of name pointers, and let print_binder_transaction_log_entry()
> look them up in something like a hashtable.
> 3) Just copy the name into the transaction log every time.
>
> I'm not sure which one is better, or whether there's a nicer fourth
> option, so I'm leaving writing a patch for this to y'all.
>
>
> Trigger instructions (requires you to have some helpers that can
> register a context manager and send some transaction to it):
> ==============
> root@test:/home/user# mkdir /tmp/binder
> root@test:/home/user# mount -t binder -o stats=global /dev/null /tmp/binder
> root@test:/home/user# ls -l /tmp/binder
> total 0
> crw------- 1 root root 248, 1 Oct  7 20:34 binder
> crw------- 1 root root 248, 0 Oct  7 20:34 binder-control
> drwxr-xr-x 3 root root      0 Oct  7 20:34 binder_logs
> crw------- 1 root root 248, 2 Oct  7 20:34 hwbinder
> crw------- 1 root root 248, 3 Oct  7 20:34 vndbinder
> root@test:/home/user# ln -s /tmp/binder/binder /dev/binder
> [run some simple binder demo code to temporarily register a context
> manager and send a binder transaction]
> root@test:/home/user# rm /tmp/binder/binder
> root@test:/home/user# cat /tmp/binder/binder_logs/transaction_log
> 2: call  from 2277:2277 to 2273:0 context @������� node 1 handle 0
> size 24:8 ret 0/0 l=0
> 5: call  from 2273:2273 to 2277:2277 context @������� node 3 handle 1
> size 0:0 ret 0/0 l=0
> 6: reply from 2277:2277 to 2273:2273 context @������� node 0 handle 0
> size 4:0 ret 0/0 l=0
> 7: reply from 2273:2273 to 2277:2277 context @������� node 0 handle 0
> size 4:0 ret 0/0 l=0
> root@test:/home/user#
> ==============
>
> ASAN splat:
> [  333.300753] ==================================================================
> [  333.303197] BUG: KASAN: use-after-free in string_nocheck+0x9d/0x160
> [  333.305081] Read of size 1 at addr ffff8880b0981258 by task cat/2279
>
> [  333.307415] CPU: 1 PID: 2279 Comm: cat Not tainted 5.4.0-rc1+ #513
> [  333.309304] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS 1.12.0-1 04/01/2014
> [  333.310987] Call Trace:
> [  333.312032]  dump_stack+0x7c/0xc0
> [  333.312581]  ? string_nocheck+0x9d/0x160
> [  333.313157]  print_address_description.constprop.7+0x36/0x50
> [  333.314030]  ? string_nocheck+0x9d/0x160
> [  333.314603]  ? string_nocheck+0x9d/0x160
> [  333.315236]  __kasan_report.cold.10+0x1a/0x35
> [  333.315972]  ? string_nocheck+0x9d/0x160
> [  333.316545]  kasan_report+0xe/0x20
> [  333.317104]  string_nocheck+0x9d/0x160
> [  333.317652]  ? widen_string+0x160/0x160
> [  333.318270]  ? string_nocheck+0x160/0x160
> [  333.318857]  ? unwind_get_return_address+0x2a/0x40
> [  333.319636]  ? profile_setup.cold.9+0x96/0x96
> [  333.320359]  string+0xb6/0xc0
> [  333.320800]  ? hex_string+0x280/0x280
> [  333.321398]  vsnprintf+0x20c/0x780
> [  333.321898]  ? num_to_str+0x180/0x180
> [  333.322503]  ? __kasan_kmalloc.constprop.6+0xc1/0xd0
> [  333.323235]  ? vfs_read+0xbc/0x1e0
> [  333.323814]  ? ksys_read+0xb5/0x150
> [  333.324323]  ? do_syscall_64+0xb9/0x3b0
> [  333.324948]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [  333.325756]  seq_vprintf+0x78/0xb0
> [  333.326253]  seq_printf+0x96/0xc0
> [  333.327132]  ? seq_vprintf+0xb0/0xb0
> [  333.327678]  ? match_held_lock+0x2e/0x240
> [  333.328450]  binder_transaction_log_show+0x237/0x2d0
> [  333.329163]  seq_read+0x266/0x690
> [  333.329705]  vfs_read+0xbc/0x1e0
> [  333.330178]  ksys_read+0xb5/0x150
> [  333.330724]  ? kernel_write+0xb0/0xb0
> [  333.331257]  ? trace_hardirqs_off_caller+0x57/0x130
> [  333.332045]  ? mark_held_locks+0x29/0xa0
> [  333.332678]  ? do_syscall_64+0x6b/0x3b0
> [  333.333235]  do_syscall_64+0xb9/0x3b0
> [  333.333856]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [  333.334635] RIP: 0033:0x7fbbb95d4461
> [  333.335153] Code: fe ff ff 50 48 8d 3d fe d0 09 00 e8 e9 03 02 00
> 66 0f 1f 84 00 00 00 00 00 48 8d 05 99 62 0d 00 8b 00 85 c0 75 13 31
> c0 0f 05 <48> 3d 00 f0 ff ff 77 57 c3 66 0f 1f 44 00 00 41 54 49 89 d4
> 55 48
> [  333.337950] RSP: 002b:00007ffcbe6438e8 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000000
> [  333.339072] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007fbbb95d4461
> [  333.340157] RDX: 0000000000020000 RSI: 00007fbbb9324000 RDI: 0000000000000003
> [  333.341320] RBP: 00007fbbb9324000 R08: 00000000ffffffff R09: 0000000000000000
> [  333.342454] R10: fffffffffffffb9c R11: 0000000000000246 R12: 00007fbbb9324000
> [  333.343550] R13: 0000000000000003 R14: 0000000000000fff R15: 0000000000020000
>
> [  333.344845] Allocated by task 2259:
> [  333.345416]  save_stack+0x19/0x80
> [  333.345899]  __kasan_kmalloc.constprop.6+0xc1/0xd0
> [  333.346636]  __kmalloc_track_caller+0xf4/0x2e0
> [  333.347271]  kmemdup+0x17/0x40
> [  333.347796]  binderfs_binder_device_create.isra.6+0x217/0x530
> [  333.348674]  binderfs_fill_super+0x486/0x81e
> [  333.349309]  mount_nodev+0x41/0xb0
> [  333.349860]  legacy_get_tree+0x7b/0xc0
> [  333.350398]  vfs_get_tree+0x40/0x130
> [  333.350970]  do_mount+0xacb/0xea0
> [  333.351449]  ksys_mount+0xb1/0xd0
> [  333.352007]  __x64_sys_mount+0x5d/0x70
> [  333.352545]  do_syscall_64+0xb9/0x3b0
> [  333.353144]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> [  333.354144] Freed by task 2278:
> [  333.354598]  save_stack+0x19/0x80
> [  333.355135]  __kasan_slab_free+0x12e/0x180
> [  333.355734]  kfree+0xe6/0x310
> [  333.356234]  binderfs_evict_inode+0xb8/0xd0
> [  333.356831]  evict+0x16f/0x290
> [  333.358081]  do_unlinkat+0x2f6/0x420
> [  333.358593]  do_syscall_64+0xb9/0x3b0
> [  333.359176]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> [  333.360196] The buggy address belongs to the object at ffff8880b0981258
>                 which belongs to the cache kmalloc-8 of size 8
> [  333.361991] The buggy address is located 0 bytes inside of
>                 8-byte region [ffff8880b0981258, ffff8880b0981260)
> [  333.363796] The buggy address belongs to the page:
> [  333.364538] page:ffffea0002c26040 refcount:1 mapcount:0
> mapping:ffff8880b6c03c80 index:0x0
> [  333.365765] flags: 0x1fffc0000000200(slab)
> [  333.366402] raw: 01fffc0000000200 ffffea0002cb1d80 0000001400000014
> ffff8880b6c03c80
> [  333.367546] raw: 0000000000000000 0000000000aa00aa 00000001ffffffff
> 0000000000000000
> [  333.369030] page dumped because: kasan: bad access detected
>
> [  333.370095] Memory state around the buggy address:
> [  333.370824]  ffff8880b0981100: fc fb fc fc fb fc fc fb fc fc fb fc
> fc fb fc fc
> [  333.371907]  ffff8880b0981180: fb fc fc fb fc fc fb fc fc fb fc fc
> fb fc fc fb
> [  333.372969] >ffff8880b0981200: fc fc fb fc fc fb fc fc fb fc fc fb
> fc fc fb fc
> [  333.374033]                                                     ^
> [  333.374884]  ffff8880b0981280: fc fb fc fc fb fc fc fb fc fc fb fc
> fc fb fc fc
> [  333.375957]  ffff8880b0981300: fb fc fc fb fc fc fb fc fc fb fc fc
> fb fc fc fb
> [  333.377013] ==================================================================

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

* Re: UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels
  2019-10-07 20:49 UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels Jann Horn
  2019-10-07 21:04 ` Todd Kjos
@ 2019-10-07 21:05 ` Christian Brauner
  2019-10-08 13:01 ` [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry() Christian Brauner
  2 siblings, 0 replies; 15+ messages in thread
From: Christian Brauner @ 2019-10-07 21:05 UTC (permalink / raw)
  To: Jann Horn
  Cc: Todd Kjos, Martijn Coenen, Arve Hjønnevåg,
	Joel Fernandes, open list:ANDROID DRIVERS, kernel list,
	Greg Kroah-Hartman

On Mon, Oct 07, 2019 at 10:49:57PM +0200, Jann Horn wrote:
> Hi!
> 
> There is a use-after-free read in print_binder_transaction_log_entry()
> on ANDROID_BINDERFS kernels because
> print_binder_transaction_log_entry() prints the char* e->context_name
> as string, and if the transaction occurred on a binder device from
> binderfs, e->context_name belongs to the binder device and is freed
> when the inode disappears.
> 
> Luckily this shouldn't have security implications, since:
> 
> a) reading the binder transaction log is already a pretty privileged operation
> b) I can't find any actual users of ANDROID_BINDERFS
> 
> I guess there are three ways to fix it:
> 1) Create a new shared global spinlock for binderfs_evict_inode() and
> binder_transaction_log_show(), and let binderfs_evict_inode() scan the
> transaction log for pointers to its name and replace them with
> pointers to a statically-allocated string "{DELETED}" or something
> like that.
> 2) Let the transaction log contain non-reusable device identifiers
> instead of name pointers, and let print_binder_transaction_log_entry()
> look them up in something like a hashtable.
> 3) Just copy the name into the transaction log every time.
> 
> I'm not sure which one is better, or whether there's a nicer fourth
> option, so I'm leaving writing a patch for this to y'all.

Moin,

Thanks for the report.
Android binderfs is enabled by default on Ubuntu and - iirc - Debian
kernels. It is actively used by Anbox to run Android in containers.

The codepath you're referring to is specific to the stats=global mount
option. This was contributed by the Android team for the 5.4 cycle (cf.
[1]). That means there is no released kernel that supports the
stats=global mount option. So all current users cannot be affected by
this bug.

I'll take a look at this tomorrow and see what makes the most sense. I
agree that this is not a security issue. Thanks for catching this early.

If you already have a script that trivially reproduces the bug it'd be
nice if you could paste it. Otherwise we can just add a reproducer based
on your snippet from below. I want to add a regression test for this.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f00834518ed3194b866f5f3d63b71e0ed7f6bc00
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e13e452dafc009049a9a5a4153e2f9e51b23915
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=03e2e07e38147917482d595ad3cf193212ded8ac
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4feb80faf428a02d407a9ea1952004af01308765

Thanks!
Christian

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

* Re: UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels
  2019-10-07 21:04 ` Todd Kjos
@ 2019-10-07 21:16   ` Hridya Valsaraju
  0 siblings, 0 replies; 15+ messages in thread
From: Hridya Valsaraju @ 2019-10-07 21:16 UTC (permalink / raw)
  To: Todd Kjos
  Cc: Jann Horn, Martijn Coenen, Arve Hjønnevåg,
	Joel Fernandes, Christian Brauner, open list:ANDROID DRIVERS,
	kernel list, Greg Kroah-Hartman

Thank you for letting us know about the issue Jann. I will work on a
fix and send out the same for review once ready.

Regards,
Hridya


On Mon, Oct 7, 2019 at 2:04 PM Todd Kjos <tkjos@google.com> wrote:
>
> +Hridya Valsaraju
>
>
> On Mon, Oct 7, 2019 at 1:50 PM Jann Horn <jannh@google.com> wrote:
> >
> > Hi!
> >
> > There is a use-after-free read in print_binder_transaction_log_entry()
> > on ANDROID_BINDERFS kernels because
> > print_binder_transaction_log_entry() prints the char* e->context_name
> > as string, and if the transaction occurred on a binder device from
> > binderfs, e->context_name belongs to the binder device and is freed
> > when the inode disappears.
> >
> > Luckily this shouldn't have security implications, since:
> >
> > a) reading the binder transaction log is already a pretty privileged operation
> > b) I can't find any actual users of ANDROID_BINDERFS
> >
> > I guess there are three ways to fix it:
> > 1) Create a new shared global spinlock for binderfs_evict_inode() and
> > binder_transaction_log_show(), and let binderfs_evict_inode() scan the
> > transaction log for pointers to its name and replace them with
> > pointers to a statically-allocated string "{DELETED}" or something
> > like that.
> > 2) Let the transaction log contain non-reusable device identifiers
> > instead of name pointers, and let print_binder_transaction_log_entry()
> > look them up in something like a hashtable.
> > 3) Just copy the name into the transaction log every time.
> >
> > I'm not sure which one is better, or whether there's a nicer fourth
> > option, so I'm leaving writing a patch for this to y'all.
> >
> >
> > Trigger instructions (requires you to have some helpers that can
> > register a context manager and send some transaction to it):
> > ==============
> > root@test:/home/user# mkdir /tmp/binder
> > root@test:/home/user# mount -t binder -o stats=global /dev/null /tmp/binder
> > root@test:/home/user# ls -l /tmp/binder
> > total 0
> > crw------- 1 root root 248, 1 Oct  7 20:34 binder
> > crw------- 1 root root 248, 0 Oct  7 20:34 binder-control
> > drwxr-xr-x 3 root root      0 Oct  7 20:34 binder_logs
> > crw------- 1 root root 248, 2 Oct  7 20:34 hwbinder
> > crw------- 1 root root 248, 3 Oct  7 20:34 vndbinder
> > root@test:/home/user# ln -s /tmp/binder/binder /dev/binder
> > [run some simple binder demo code to temporarily register a context
> > manager and send a binder transaction]
> > root@test:/home/user# rm /tmp/binder/binder
> > root@test:/home/user# cat /tmp/binder/binder_logs/transaction_log
> > 2: call  from 2277:2277 to 2273:0 context @������� node 1 handle 0
> > size 24:8 ret 0/0 l=0
> > 5: call  from 2273:2273 to 2277:2277 context @������� node 3 handle 1
> > size 0:0 ret 0/0 l=0
> > 6: reply from 2277:2277 to 2273:2273 context @������� node 0 handle 0
> > size 4:0 ret 0/0 l=0
> > 7: reply from 2273:2273 to 2277:2277 context @������� node 0 handle 0
> > size 4:0 ret 0/0 l=0
> > root@test:/home/user#
> > ==============
> >
> > ASAN splat:
> > [  333.300753] ==================================================================
> > [  333.303197] BUG: KASAN: use-after-free in string_nocheck+0x9d/0x160
> > [  333.305081] Read of size 1 at addr ffff8880b0981258 by task cat/2279
> >
> > [  333.307415] CPU: 1 PID: 2279 Comm: cat Not tainted 5.4.0-rc1+ #513
> > [  333.309304] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS 1.12.0-1 04/01/2014
> > [  333.310987] Call Trace:
> > [  333.312032]  dump_stack+0x7c/0xc0
> > [  333.312581]  ? string_nocheck+0x9d/0x160
> > [  333.313157]  print_address_description.constprop.7+0x36/0x50
> > [  333.314030]  ? string_nocheck+0x9d/0x160
> > [  333.314603]  ? string_nocheck+0x9d/0x160
> > [  333.315236]  __kasan_report.cold.10+0x1a/0x35
> > [  333.315972]  ? string_nocheck+0x9d/0x160
> > [  333.316545]  kasan_report+0xe/0x20
> > [  333.317104]  string_nocheck+0x9d/0x160
> > [  333.317652]  ? widen_string+0x160/0x160
> > [  333.318270]  ? string_nocheck+0x160/0x160
> > [  333.318857]  ? unwind_get_return_address+0x2a/0x40
> > [  333.319636]  ? profile_setup.cold.9+0x96/0x96
> > [  333.320359]  string+0xb6/0xc0
> > [  333.320800]  ? hex_string+0x280/0x280
> > [  333.321398]  vsnprintf+0x20c/0x780
> > [  333.321898]  ? num_to_str+0x180/0x180
> > [  333.322503]  ? __kasan_kmalloc.constprop.6+0xc1/0xd0
> > [  333.323235]  ? vfs_read+0xbc/0x1e0
> > [  333.323814]  ? ksys_read+0xb5/0x150
> > [  333.324323]  ? do_syscall_64+0xb9/0x3b0
> > [  333.324948]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [  333.325756]  seq_vprintf+0x78/0xb0
> > [  333.326253]  seq_printf+0x96/0xc0
> > [  333.327132]  ? seq_vprintf+0xb0/0xb0
> > [  333.327678]  ? match_held_lock+0x2e/0x240
> > [  333.328450]  binder_transaction_log_show+0x237/0x2d0
> > [  333.329163]  seq_read+0x266/0x690
> > [  333.329705]  vfs_read+0xbc/0x1e0
> > [  333.330178]  ksys_read+0xb5/0x150
> > [  333.330724]  ? kernel_write+0xb0/0xb0
> > [  333.331257]  ? trace_hardirqs_off_caller+0x57/0x130
> > [  333.332045]  ? mark_held_locks+0x29/0xa0
> > [  333.332678]  ? do_syscall_64+0x6b/0x3b0
> > [  333.333235]  do_syscall_64+0xb9/0x3b0
> > [  333.333856]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [  333.334635] RIP: 0033:0x7fbbb95d4461
> > [  333.335153] Code: fe ff ff 50 48 8d 3d fe d0 09 00 e8 e9 03 02 00
> > 66 0f 1f 84 00 00 00 00 00 48 8d 05 99 62 0d 00 8b 00 85 c0 75 13 31
> > c0 0f 05 <48> 3d 00 f0 ff ff 77 57 c3 66 0f 1f 44 00 00 41 54 49 89 d4
> > 55 48
> > [  333.337950] RSP: 002b:00007ffcbe6438e8 EFLAGS: 00000246 ORIG_RAX:
> > 0000000000000000
> > [  333.339072] RAX: ffffffffffffffda RBX: 0000000000020000 RCX: 00007fbbb95d4461
> > [  333.340157] RDX: 0000000000020000 RSI: 00007fbbb9324000 RDI: 0000000000000003
> > [  333.341320] RBP: 00007fbbb9324000 R08: 00000000ffffffff R09: 0000000000000000
> > [  333.342454] R10: fffffffffffffb9c R11: 0000000000000246 R12: 00007fbbb9324000
> > [  333.343550] R13: 0000000000000003 R14: 0000000000000fff R15: 0000000000020000
> >
> > [  333.344845] Allocated by task 2259:
> > [  333.345416]  save_stack+0x19/0x80
> > [  333.345899]  __kasan_kmalloc.constprop.6+0xc1/0xd0
> > [  333.346636]  __kmalloc_track_caller+0xf4/0x2e0
> > [  333.347271]  kmemdup+0x17/0x40
> > [  333.347796]  binderfs_binder_device_create.isra.6+0x217/0x530
> > [  333.348674]  binderfs_fill_super+0x486/0x81e
> > [  333.349309]  mount_nodev+0x41/0xb0
> > [  333.349860]  legacy_get_tree+0x7b/0xc0
> > [  333.350398]  vfs_get_tree+0x40/0x130
> > [  333.350970]  do_mount+0xacb/0xea0
> > [  333.351449]  ksys_mount+0xb1/0xd0
> > [  333.352007]  __x64_sys_mount+0x5d/0x70
> > [  333.352545]  do_syscall_64+0xb9/0x3b0
> > [  333.353144]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > [  333.354144] Freed by task 2278:
> > [  333.354598]  save_stack+0x19/0x80
> > [  333.355135]  __kasan_slab_free+0x12e/0x180
> > [  333.355734]  kfree+0xe6/0x310
> > [  333.356234]  binderfs_evict_inode+0xb8/0xd0
> > [  333.356831]  evict+0x16f/0x290
> > [  333.358081]  do_unlinkat+0x2f6/0x420
> > [  333.358593]  do_syscall_64+0xb9/0x3b0
> > [  333.359176]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > [  333.360196] The buggy address belongs to the object at ffff8880b0981258
> >                 which belongs to the cache kmalloc-8 of size 8
> > [  333.361991] The buggy address is located 0 bytes inside of
> >                 8-byte region [ffff8880b0981258, ffff8880b0981260)
> > [  333.363796] The buggy address belongs to the page:
> > [  333.364538] page:ffffea0002c26040 refcount:1 mapcount:0
> > mapping:ffff8880b6c03c80 index:0x0
> > [  333.365765] flags: 0x1fffc0000000200(slab)
> > [  333.366402] raw: 01fffc0000000200 ffffea0002cb1d80 0000001400000014
> > ffff8880b6c03c80
> > [  333.367546] raw: 0000000000000000 0000000000aa00aa 00000001ffffffff
> > 0000000000000000
> > [  333.369030] page dumped because: kasan: bad access detected
> >
> > [  333.370095] Memory state around the buggy address:
> > [  333.370824]  ffff8880b0981100: fc fb fc fc fb fc fc fb fc fc fb fc
> > fc fb fc fc
> > [  333.371907]  ffff8880b0981180: fb fc fc fb fc fc fb fc fc fb fc fc
> > fb fc fc fb
> > [  333.372969] >ffff8880b0981200: fc fc fb fc fc fb fc fc fb fc fc fb
> > fc fc fb fc
> > [  333.374033]                                                     ^
> > [  333.374884]  ffff8880b0981280: fc fb fc fc fb fc fc fb fc fc fb fc
> > fc fb fc fc
> > [  333.375957]  ffff8880b0981300: fb fc fc fb fc fc fb fc fc fb fc fc
> > fb fc fc fb
> > [  333.377013] ==================================================================

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

* [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-07 20:49 UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels Jann Horn
  2019-10-07 21:04 ` Todd Kjos
  2019-10-07 21:05 ` Christian Brauner
@ 2019-10-08 13:01 ` Christian Brauner
  2019-10-08 17:18   ` Hridya Valsaraju
                     ` (2 more replies)
  2 siblings, 3 replies; 15+ messages in thread
From: Christian Brauner @ 2019-10-08 13:01 UTC (permalink / raw)
  To: jannh
  Cc: arve, christian, devel, gregkh, joel, linux-kernel, maco, tkjos,
	Christian Brauner, Todd Kjos, Hridya Valsaraju

When a binder transaction is initiated on a binder device coming from a
binderfs instance, a pointer to the name of the binder device is stashed
in the binder_transaction_log_entry's context_name member. Later on it
is used to print the name in print_binder_transaction_log_entry(). By
the time print_binder_transaction_log_entry() accesses context_name
binderfs_evict_inode() might have already freed the associated memory
thereby causing a UAF. Do the simple thing and prevent this by copying
the name of the binder device instead of stashing a pointer to it.

Reported-by: Jann Horn <jannh@google.com>
Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Todd Kjos <tkjos@android.com>
Cc: Hridya Valsaraju <hridya@google.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 drivers/android/binder.c          | 4 +++-
 drivers/android/binder_internal.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c0a491277aca..5b9ac2122e89 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -57,6 +57,7 @@
 #include <linux/sched/signal.h>
 #include <linux/sched/mm.h>
 #include <linux/seq_file.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
 #include <linux/pid_namespace.h>
 #include <linux/security.h>
@@ -66,6 +67,7 @@
 #include <linux/task_work.h>
 
 #include <uapi/linux/android/binder.h>
+#include <uapi/linux/android/binderfs.h>
 
 #include <asm/cacheflush.h>
 
@@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
 	e->target_handle = tr->target.handle;
 	e->data_size = tr->data_size;
 	e->offsets_size = tr->offsets_size;
-	e->context_name = proc->context->name;
+	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
 
 	if (reply) {
 		binder_inner_proc_lock(proc);
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index bd47f7f72075..ae991097d14d 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -130,7 +130,7 @@ struct binder_transaction_log_entry {
 	int return_error_line;
 	uint32_t return_error;
 	uint32_t return_error_param;
-	const char *context_name;
+	char context_name[BINDERFS_MAX_NAME + 1];
 };
 
 struct binder_transaction_log {
-- 
2.23.0


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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-08 13:01 ` [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry() Christian Brauner
@ 2019-10-08 17:18   ` Hridya Valsaraju
  2019-10-08 18:05   ` Joel Fernandes
  2019-10-08 18:52   ` Todd Kjos
  2 siblings, 0 replies; 15+ messages in thread
From: Hridya Valsaraju @ 2019-10-08 17:18 UTC (permalink / raw)
  To: Christian Brauner
  Cc: jannh, Arve Hjønnevåg, Christian Brauner,
	open list:ANDROID DRIVERS, Greg Kroah-Hartman, Joel Fernandes,
	LKML, Martijn Coenen, Todd Kjos, Todd Kjos

On Tue, Oct 8, 2019 at 6:02 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> When a binder transaction is initiated on a binder device coming from a
> binderfs instance, a pointer to the name of the binder device is stashed
> in the binder_transaction_log_entry's context_name member. Later on it
> is used to print the name in print_binder_transaction_log_entry(). By
> the time print_binder_transaction_log_entry() accesses context_name
> binderfs_evict_inode() might have already freed the associated memory
> thereby causing a UAF. Do the simple thing and prevent this by copying
> the name of the binder device instead of stashing a pointer to it.
>
> Reported-by: Jann Horn <jannh@google.com>
> Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Todd Kjos <tkjos@android.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Reviewed-by: Hridya Valsaraju <hridya@google.com>

Thank you for sending out this fix Christian!

Regards,
Hridya

> ---
>  drivers/android/binder.c          | 4 +++-
>  drivers/android/binder_internal.h | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c0a491277aca..5b9ac2122e89 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -57,6 +57,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/sched/mm.h>
>  #include <linux/seq_file.h>
> +#include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/security.h>
> @@ -66,6 +67,7 @@
>  #include <linux/task_work.h>
>
>  #include <uapi/linux/android/binder.h>
> +#include <uapi/linux/android/binderfs.h>
>
>  #include <asm/cacheflush.h>
>
> @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
>         e->target_handle = tr->target.handle;
>         e->data_size = tr->data_size;
>         e->offsets_size = tr->offsets_size;
> -       e->context_name = proc->context->name;
> +       strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
>
>         if (reply) {
>                 binder_inner_proc_lock(proc);
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index bd47f7f72075..ae991097d14d 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -130,7 +130,7 @@ struct binder_transaction_log_entry {
>         int return_error_line;
>         uint32_t return_error;
>         uint32_t return_error_param;
> -       const char *context_name;
> +       char context_name[BINDERFS_MAX_NAME + 1];
>  };
>
>  struct binder_transaction_log {
> --
> 2.23.0
>

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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-08 13:01 ` [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry() Christian Brauner
  2019-10-08 17:18   ` Hridya Valsaraju
@ 2019-10-08 18:05   ` Joel Fernandes
  2019-10-09 10:40     ` Christian Brauner
  2019-10-08 18:52   ` Todd Kjos
  2 siblings, 1 reply; 15+ messages in thread
From: Joel Fernandes @ 2019-10-08 18:05 UTC (permalink / raw)
  To: Christian Brauner
  Cc: jannh, arve, christian, devel, gregkh, linux-kernel, maco, tkjos,
	Todd Kjos, Hridya Valsaraju

On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> When a binder transaction is initiated on a binder device coming from a
> binderfs instance, a pointer to the name of the binder device is stashed
> in the binder_transaction_log_entry's context_name member. Later on it
> is used to print the name in print_binder_transaction_log_entry(). By
> the time print_binder_transaction_log_entry() accesses context_name
> binderfs_evict_inode() might have already freed the associated memory
> thereby causing a UAF. Do the simple thing and prevent this by copying
> the name of the binder device instead of stashing a pointer to it.
> 
> Reported-by: Jann Horn <jannh@google.com>
> Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Todd Kjos <tkjos@android.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  drivers/android/binder.c          | 4 +++-
>  drivers/android/binder_internal.h | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c0a491277aca..5b9ac2122e89 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -57,6 +57,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/sched/mm.h>
>  #include <linux/seq_file.h>
> +#include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/security.h>
> @@ -66,6 +67,7 @@
>  #include <linux/task_work.h>
>  
>  #include <uapi/linux/android/binder.h>
> +#include <uapi/linux/android/binderfs.h>
>  
>  #include <asm/cacheflush.h>
>  
> @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
>  	e->target_handle = tr->target.handle;
>  	e->data_size = tr->data_size;
>  	e->offsets_size = tr->offsets_size;
> -	e->context_name = proc->context->name;
> +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);

Strictly speaking, proc-context->name can also be initialized for !BINDERFS
so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
fit within the MAX.

>  	if (reply) {

>  		binder_inner_proc_lock(proc);
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index bd47f7f72075..ae991097d14d 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -130,7 +130,7 @@ struct binder_transaction_log_entry {
>  	int return_error_line;
>  	uint32_t return_error;
>  	uint32_t return_error_param;
> -	const char *context_name;
> +	char context_name[BINDERFS_MAX_NAME + 1];

Same comment here, context_name can be used for non-BINDERFS transactions as
well such as default binder devices.

One more thought, this can be made dependent on CONFIG_BINDERFS since regular
binder devices cannot be unregistered AFAICS and as Jann said, the problem is
BINDERFS specific. That way we avoid the memcpy for _every_ transaction.
These can be thundering when Android starts up.

(I secretly wish C strings could be refcounted to avoid exactly this issue,
that should not be hard to develop but I am not sure if it is worth it for
this problem :) - For one, it will avoid having to do the strcpy for _every_
transaction).

Other than these nits, please add my tag on whichever is the final solution:

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel


>  };
>  
>  struct binder_transaction_log {
> -- 
> 2.23.0
> 

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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-08 13:01 ` [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry() Christian Brauner
  2019-10-08 17:18   ` Hridya Valsaraju
  2019-10-08 18:05   ` Joel Fernandes
@ 2019-10-08 18:52   ` Todd Kjos
  2 siblings, 0 replies; 15+ messages in thread
From: Todd Kjos @ 2019-10-08 18:52 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Jann Horn, Arve Hjønnevåg, Christian Brauner,
	open list:ANDROID DRIVERS, Greg Kroah-Hartman,
	Joel Fernandes (Google),
	LKML, Martijn Coenen, Todd Kjos, Hridya Valsaraju

On Tue, Oct 8, 2019 at 6:02 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> When a binder transaction is initiated on a binder device coming from a
> binderfs instance, a pointer to the name of the binder device is stashed
> in the binder_transaction_log_entry's context_name member. Later on it
> is used to print the name in print_binder_transaction_log_entry(). By
> the time print_binder_transaction_log_entry() accesses context_name
> binderfs_evict_inode() might have already freed the associated memory
> thereby causing a UAF. Do the simple thing and prevent this by copying
> the name of the binder device instead of stashing a pointer to it.
>
> Reported-by: Jann Horn <jannh@google.com>
> Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Todd Kjos <tkjos@android.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  drivers/android/binder.c          | 4 +++-
>  drivers/android/binder_internal.h | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c0a491277aca..5b9ac2122e89 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -57,6 +57,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/sched/mm.h>
>  #include <linux/seq_file.h>
> +#include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/security.h>
> @@ -66,6 +67,7 @@
>  #include <linux/task_work.h>
>
>  #include <uapi/linux/android/binder.h>
> +#include <uapi/linux/android/binderfs.h>
>
>  #include <asm/cacheflush.h>
>
> @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
>         e->target_handle = tr->target.handle;
>         e->data_size = tr->data_size;
>         e->offsets_size = tr->offsets_size;
> -       e->context_name = proc->context->name;
> +       strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
>
>         if (reply) {
>                 binder_inner_proc_lock(proc);
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index bd47f7f72075..ae991097d14d 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -130,7 +130,7 @@ struct binder_transaction_log_entry {
>         int return_error_line;
>         uint32_t return_error;
>         uint32_t return_error_param;
> -       const char *context_name;
> +       char context_name[BINDERFS_MAX_NAME + 1];
>  };
>
>  struct binder_transaction_log {
> --
> 2.23.0
>

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

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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-08 18:05   ` Joel Fernandes
@ 2019-10-09 10:40     ` Christian Brauner
  2019-10-09 14:21       ` Joel Fernandes
  2019-10-09 15:56       ` Todd Kjos
  0 siblings, 2 replies; 15+ messages in thread
From: Christian Brauner @ 2019-10-09 10:40 UTC (permalink / raw)
  To: Joel Fernandes, Todd Kjos
  Cc: jannh, arve, christian, devel, gregkh, linux-kernel, maco, tkjos,
	Todd Kjos, Hridya Valsaraju

On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > When a binder transaction is initiated on a binder device coming from a
> > binderfs instance, a pointer to the name of the binder device is stashed
> > in the binder_transaction_log_entry's context_name member. Later on it
> > is used to print the name in print_binder_transaction_log_entry(). By
> > the time print_binder_transaction_log_entry() accesses context_name
> > binderfs_evict_inode() might have already freed the associated memory
> > thereby causing a UAF. Do the simple thing and prevent this by copying
> > the name of the binder device instead of stashing a pointer to it.
> > 
> > Reported-by: Jann Horn <jannh@google.com>
> > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > Cc: Joel Fernandes <joel@joelfernandes.org>
> > Cc: Todd Kjos <tkjos@android.com>
> > Cc: Hridya Valsaraju <hridya@google.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> >  drivers/android/binder.c          | 4 +++-
> >  drivers/android/binder_internal.h | 2 +-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index c0a491277aca..5b9ac2122e89 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -57,6 +57,7 @@
> >  #include <linux/sched/signal.h>
> >  #include <linux/sched/mm.h>
> >  #include <linux/seq_file.h>
> > +#include <linux/string.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/pid_namespace.h>
> >  #include <linux/security.h>
> > @@ -66,6 +67,7 @@
> >  #include <linux/task_work.h>
> >  
> >  #include <uapi/linux/android/binder.h>
> > +#include <uapi/linux/android/binderfs.h>
> >  
> >  #include <asm/cacheflush.h>
> >  
> > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> >  	e->target_handle = tr->target.handle;
> >  	e->data_size = tr->data_size;
> >  	e->offsets_size = tr->offsets_size;
> > -	e->context_name = proc->context->name;
> > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> 
> Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> fit within the MAX.

I know but I don't think it's worth special-casing non-binderfs devices.
First, non-binderfs devices can only be created through a KCONFIG option
determined at compile time. For stock Android the names are the same for
all vendors afaik.
Second, BINDERFS_MAX_NAME is set to the maximum path name component
length that nearly all filesystems support (256 chars). If you exceed
that then you run afoul of a bunch of other assumptions already and will
cause trouble.
Third, even if there is someone crazy and uses more than 256 chars for a
non-binderfs device at KCONFIG time strscpy will do the right thing and
truncate and you'd see a truncated binder device name. This doesn't seem
to be a big deal for a debugfs interface.
Fourth, the check for non-binderfs devices technically has nothing to do
with this patch. This patch should really just do the minimal thing and
fix the UAF. Which it does.
Fifth, I already tried to push for validation of non-binderfs binder
devices a while back when I wrote binderfs and was told that it's not
needed. Hrydia tried the same and we decided the same thing. So you get
to be the next person to send a patch. :)

> 
> >  	if (reply) {
> 
> >  		binder_inner_proc_lock(proc);
> > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> > index bd47f7f72075..ae991097d14d 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -130,7 +130,7 @@ struct binder_transaction_log_entry {
> >  	int return_error_line;
> >  	uint32_t return_error;
> >  	uint32_t return_error_param;
> > -	const char *context_name;
> > +	char context_name[BINDERFS_MAX_NAME + 1];
> 
> Same comment here, context_name can be used for non-BINDERFS transactions as
> well such as default binder devices.

See above.

> 
> One more thought, this can be made dependent on CONFIG_BINDERFS since regular
> binder devices cannot be unregistered AFAICS and as Jann said, the problem is
> BINDERFS specific. That way we avoid the memcpy for _every_ transaction.
> These can be thundering when Android starts up.

Unless Todd sees this as a real performance problem I'm weary to
introduce additional checking and record a pointer for non-binderfs and
a memcpy() for binderfs devices. :)

> 
> (I secretly wish C strings could be refcounted to avoid exactly this issue,
> that should not be hard to develop but I am not sure if it is worth it for
> this problem :) - For one, it will avoid having to do the strcpy for _every_
> transaction).
> 
> Other than these nits, please add my tag on whichever is the final solution:
> 
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks for the review, Joel. :)
Christian

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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-09 10:40     ` Christian Brauner
@ 2019-10-09 14:21       ` Joel Fernandes
  2019-10-09 14:29         ` Christian Brauner
  2019-10-09 15:56       ` Todd Kjos
  1 sibling, 1 reply; 15+ messages in thread
From: Joel Fernandes @ 2019-10-09 14:21 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Todd Kjos, jannh, arve, christian, devel, gregkh, linux-kernel,
	maco, tkjos, Hridya Valsaraju

On Wed, Oct 09, 2019 at 12:40:12PM +0200, Christian Brauner wrote:
> On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > > When a binder transaction is initiated on a binder device coming from a
> > > binderfs instance, a pointer to the name of the binder device is stashed
> > > in the binder_transaction_log_entry's context_name member. Later on it
> > > is used to print the name in print_binder_transaction_log_entry(). By
> > > the time print_binder_transaction_log_entry() accesses context_name
> > > binderfs_evict_inode() might have already freed the associated memory
> > > thereby causing a UAF. Do the simple thing and prevent this by copying
> > > the name of the binder device instead of stashing a pointer to it.
> > > 
> > > Reported-by: Jann Horn <jannh@google.com>
> > > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > > Cc: Joel Fernandes <joel@joelfernandes.org>
> > > Cc: Todd Kjos <tkjos@android.com>
> > > Cc: Hridya Valsaraju <hridya@google.com>
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > ---
> > >  drivers/android/binder.c          | 4 +++-
> > >  drivers/android/binder_internal.h | 2 +-
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index c0a491277aca..5b9ac2122e89 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -57,6 +57,7 @@
> > >  #include <linux/sched/signal.h>
> > >  #include <linux/sched/mm.h>
> > >  #include <linux/seq_file.h>
> > > +#include <linux/string.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/pid_namespace.h>
> > >  #include <linux/security.h>
> > > @@ -66,6 +67,7 @@
> > >  #include <linux/task_work.h>
> > >  
> > >  #include <uapi/linux/android/binder.h>
> > > +#include <uapi/linux/android/binderfs.h>
> > >  
> > >  #include <asm/cacheflush.h>
> > >  
> > > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> > >  	e->target_handle = tr->target.handle;
> > >  	e->data_size = tr->data_size;
> > >  	e->offsets_size = tr->offsets_size;
> > > -	e->context_name = proc->context->name;
> > > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> > 
> > Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > fit within the MAX.
> 
> I know but I don't think it's worth special-casing non-binderfs devices.
> First, non-binderfs devices can only be created through a KCONFIG option
> determined at compile time. For stock Android the names are the same for
> all vendors afaik.

I am just talking about the name of weirdly named macro here.

> Second, BINDERFS_MAX_NAME is set to the maximum path name component
> length that nearly all filesystems support (256 chars). If you exceed
> that then you run afoul of a bunch of other assumptions already and will
> cause trouble.

Again, just talking about the name.

> Third, even if there is someone crazy and uses more than 256 chars for a
> non-binderfs device at KCONFIG time strscpy will do the right thing and
> truncate and you'd see a truncated binder device name. This doesn't seem
> to be a big deal for a debugfs interface.

Sure I never said the patch has a bug.

> Fourth, the check for non-binderfs devices technically has nothing to do
> with this patch. This patch should really just do the minimal thing and
> fix the UAF. Which it does.

Again, never said the patch is buggy.

> Fifth, I already tried to push for validation of non-binderfs binder
> devices a while back when I wrote binderfs and was told that it's not
> needed. Hrydia tried the same and we decided the same thing. So you get
> to be the next person to send a patch. :)

I don't follow why we are talking about non-binderfs validation. I am just
saying a memcpy of the name could have been avoided for regular binder
devices. But since Todd Acked it, I wont stand in the way..

> > One more thought, this can be made dependent on CONFIG_BINDERFS since regular
> > binder devices cannot be unregistered AFAICS and as Jann said, the problem is
> > BINDERFS specific. That way we avoid the memcpy for _every_ transaction.
> > These can be thundering when Android starts up.
> 
> Unless Todd sees this as a real performance problem I'm weary to
> introduce additional checking and record a pointer for non-binderfs and
> a memcpy() for binderfs devices. :)

Ok.

> > (I secretly wish C strings could be refcounted to avoid exactly this issue,
> > that should not be hard to develop but I am not sure if it is worth it for
> > this problem :) - For one, it will avoid having to do the strcpy for _every_
> > transaction).
> > 
> > Other than these nits, please add my tag on whichever is the final solution:
> > 
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Thanks for the review, Joel. :)

My duty!! ;-)

thanks,

 - Joel


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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-09 14:21       ` Joel Fernandes
@ 2019-10-09 14:29         ` Christian Brauner
  2019-10-09 14:55           ` Joel Fernandes
  0 siblings, 1 reply; 15+ messages in thread
From: Christian Brauner @ 2019-10-09 14:29 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Todd Kjos, jannh, arve, christian, devel, gregkh, linux-kernel,
	maco, tkjos, Hridya Valsaraju

On Wed, Oct 09, 2019 at 10:21:29AM -0400, Joel Fernandes wrote:
> On Wed, Oct 09, 2019 at 12:40:12PM +0200, Christian Brauner wrote:
> > On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > > > When a binder transaction is initiated on a binder device coming from a
> > > > binderfs instance, a pointer to the name of the binder device is stashed
> > > > in the binder_transaction_log_entry's context_name member. Later on it
> > > > is used to print the name in print_binder_transaction_log_entry(). By
> > > > the time print_binder_transaction_log_entry() accesses context_name
> > > > binderfs_evict_inode() might have already freed the associated memory
> > > > thereby causing a UAF. Do the simple thing and prevent this by copying
> > > > the name of the binder device instead of stashing a pointer to it.
> > > > 
> > > > Reported-by: Jann Horn <jannh@google.com>
> > > > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > > > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > > > Cc: Joel Fernandes <joel@joelfernandes.org>
> > > > Cc: Todd Kjos <tkjos@android.com>
> > > > Cc: Hridya Valsaraju <hridya@google.com>
> > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > ---
> > > >  drivers/android/binder.c          | 4 +++-
> > > >  drivers/android/binder_internal.h | 2 +-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > index c0a491277aca..5b9ac2122e89 100644
> > > > --- a/drivers/android/binder.c
> > > > +++ b/drivers/android/binder.c
> > > > @@ -57,6 +57,7 @@
> > > >  #include <linux/sched/signal.h>
> > > >  #include <linux/sched/mm.h>
> > > >  #include <linux/seq_file.h>
> > > > +#include <linux/string.h>
> > > >  #include <linux/uaccess.h>
> > > >  #include <linux/pid_namespace.h>
> > > >  #include <linux/security.h>
> > > > @@ -66,6 +67,7 @@
> > > >  #include <linux/task_work.h>
> > > >  
> > > >  #include <uapi/linux/android/binder.h>
> > > > +#include <uapi/linux/android/binderfs.h>
> > > >  
> > > >  #include <asm/cacheflush.h>
> > > >  
> > > > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> > > >  	e->target_handle = tr->target.handle;
> > > >  	e->data_size = tr->data_size;
> > > >  	e->offsets_size = tr->offsets_size;
> > > > -	e->context_name = proc->context->name;
> > > > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> > > 
> > > Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > fit within the MAX.
> > 
> > I know but I don't think it's worth special-casing non-binderfs devices.
> > First, non-binderfs devices can only be created through a KCONFIG option
> > determined at compile time. For stock Android the names are the same for
> > all vendors afaik.
> 
> I am just talking about the name of weirdly named macro here.

You might miss context here: It's named that way because currently only
binderfs binder devices are bound to that limit. That's a point I made
further below in my previous mail. Non-binderfs devices are not subject
to that restriction and when we tried to make them subject to the same
it as rejected.

<snip>

> 
> > Fifth, I already tried to push for validation of non-binderfs binder
> > devices a while back when I wrote binderfs and was told that it's not
> > needed. Hrydia tried the same and we decided the same thing. So you get
> > to be the next person to send a patch. :)
> 
> I don't follow why we are talking about non-binderfs validation. I am just

Because above you said

> > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > fit within the MAX.

which to me reads like you want generic checks for _all_ binder devices
not just for the ones from binderfs.

(Btw, I didn't read your comments as pointing it out the patch is buggy.
I mostly wanted to provide context why we ended up with the
binderfs-specific restriction. Maybe the list sounded like a complaint
but it wasn't meant to. :))

Thanks!
Christian

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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-09 14:29         ` Christian Brauner
@ 2019-10-09 14:55           ` Joel Fernandes
  2019-10-09 15:10             ` Christian Brauner
  0 siblings, 1 reply; 15+ messages in thread
From: Joel Fernandes @ 2019-10-09 14:55 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Todd Kjos, jannh, arve, christian, devel, gregkh, linux-kernel,
	maco, tkjos, Hridya Valsaraju

On Wed, Oct 09, 2019 at 04:29:11PM +0200, Christian Brauner wrote:
> On Wed, Oct 09, 2019 at 10:21:29AM -0400, Joel Fernandes wrote:
> > On Wed, Oct 09, 2019 at 12:40:12PM +0200, Christian Brauner wrote:
> > > On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > > > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > > > > When a binder transaction is initiated on a binder device coming from a
> > > > > binderfs instance, a pointer to the name of the binder device is stashed
> > > > > in the binder_transaction_log_entry's context_name member. Later on it
> > > > > is used to print the name in print_binder_transaction_log_entry(). By
> > > > > the time print_binder_transaction_log_entry() accesses context_name
> > > > > binderfs_evict_inode() might have already freed the associated memory
> > > > > thereby causing a UAF. Do the simple thing and prevent this by copying
> > > > > the name of the binder device instead of stashing a pointer to it.
> > > > > 
> > > > > Reported-by: Jann Horn <jannh@google.com>
> > > > > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > > > > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > > > > Cc: Joel Fernandes <joel@joelfernandes.org>
> > > > > Cc: Todd Kjos <tkjos@android.com>
> > > > > Cc: Hridya Valsaraju <hridya@google.com>
> > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > ---
> > > > >  drivers/android/binder.c          | 4 +++-
> > > > >  drivers/android/binder_internal.h | 2 +-
> > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > index c0a491277aca..5b9ac2122e89 100644
> > > > > --- a/drivers/android/binder.c
> > > > > +++ b/drivers/android/binder.c
> > > > > @@ -57,6 +57,7 @@
> > > > >  #include <linux/sched/signal.h>
> > > > >  #include <linux/sched/mm.h>
> > > > >  #include <linux/seq_file.h>
> > > > > +#include <linux/string.h>
> > > > >  #include <linux/uaccess.h>
> > > > >  #include <linux/pid_namespace.h>
> > > > >  #include <linux/security.h>
> > > > > @@ -66,6 +67,7 @@
> > > > >  #include <linux/task_work.h>
> > > > >  
> > > > >  #include <uapi/linux/android/binder.h>
> > > > > +#include <uapi/linux/android/binderfs.h>
> > > > >  
> > > > >  #include <asm/cacheflush.h>
> > > > >  
> > > > > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> > > > >  	e->target_handle = tr->target.handle;
> > > > >  	e->data_size = tr->data_size;
> > > > >  	e->offsets_size = tr->offsets_size;
> > > > > -	e->context_name = proc->context->name;
> > > > > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> > > > 
> > > > Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > fit within the MAX.
> > > 
> > > I know but I don't think it's worth special-casing non-binderfs devices.
> > > First, non-binderfs devices can only be created through a KCONFIG option
> > > determined at compile time. For stock Android the names are the same for
> > > all vendors afaik.
> > 
> > I am just talking about the name of weirdly named macro here.
> 
> You might miss context here: It's named that way because currently only
> binderfs binder devices are bound to that limit. That's a point I made
> further below in my previous mail. Non-binderfs devices are not subject
> to that restriction and when we tried to make them subject to the same
> it as rejected.

I know that. I am saying the memcpy is happening for regular binder devices
as well but the macro has BINDERFS in the name. That's all. It is not a
significant eye sore. But is a bit odd.

> <snip>
> 
> > 
> > > Fifth, I already tried to push for validation of non-binderfs binder
> > > devices a while back when I wrote binderfs and was told that it's not
> > > needed. Hrydia tried the same and we decided the same thing. So you get
> > > to be the next person to send a patch. :)
> > 
> > I don't follow why we are talking about non-binderfs validation. I am just
> 
> Because above you said
> 
> > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > fit within the MAX.
> 
> which to me reads like you want generic checks for _all_ binder devices
> not just for the ones from binderfs.

No I am not talking about the checks at all, I am talking about the unwanted
mem copy you are doing for regular binder devices now. Before binderfs this
would not have happened, but now for regular binder devices we have to do the
extra mem copies which were avoided before. That was what I was talking about.

But this discussing is getting to bike shedding at this point, and since the
overhead is likely small, I am Ok with the change (though I don't like very
much the additional memcpy and the associated space wastage in the
transaction buffer for regular binder devices).

thanks!

 - Joel

> 
> (Btw, I didn't read your comments as pointing it out the patch is buggy.
> I mostly wanted to provide context why we ended up with the
> binderfs-specific restriction. Maybe the list sounded like a complaint
> but it wasn't meant to. :))
> 
> Thanks!
> Christian

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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-09 14:55           ` Joel Fernandes
@ 2019-10-09 15:10             ` Christian Brauner
  2019-10-09 15:37               ` Joel Fernandes
  0 siblings, 1 reply; 15+ messages in thread
From: Christian Brauner @ 2019-10-09 15:10 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Todd Kjos, jannh, arve, christian, devel, gregkh, linux-kernel,
	maco, tkjos, Hridya Valsaraju

On Wed, Oct 09, 2019 at 10:55:58AM -0400, Joel Fernandes wrote:
> On Wed, Oct 09, 2019 at 04:29:11PM +0200, Christian Brauner wrote:
> > On Wed, Oct 09, 2019 at 10:21:29AM -0400, Joel Fernandes wrote:
> > > On Wed, Oct 09, 2019 at 12:40:12PM +0200, Christian Brauner wrote:
> > > > On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > > > > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > > > > > When a binder transaction is initiated on a binder device coming from a
> > > > > > binderfs instance, a pointer to the name of the binder device is stashed
> > > > > > in the binder_transaction_log_entry's context_name member. Later on it
> > > > > > is used to print the name in print_binder_transaction_log_entry(). By
> > > > > > the time print_binder_transaction_log_entry() accesses context_name
> > > > > > binderfs_evict_inode() might have already freed the associated memory
> > > > > > thereby causing a UAF. Do the simple thing and prevent this by copying
> > > > > > the name of the binder device instead of stashing a pointer to it.
> > > > > > 
> > > > > > Reported-by: Jann Horn <jannh@google.com>
> > > > > > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > > > > > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > > > > > Cc: Joel Fernandes <joel@joelfernandes.org>
> > > > > > Cc: Todd Kjos <tkjos@android.com>
> > > > > > Cc: Hridya Valsaraju <hridya@google.com>
> > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > > ---
> > > > > >  drivers/android/binder.c          | 4 +++-
> > > > > >  drivers/android/binder_internal.h | 2 +-
> > > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > > index c0a491277aca..5b9ac2122e89 100644
> > > > > > --- a/drivers/android/binder.c
> > > > > > +++ b/drivers/android/binder.c
> > > > > > @@ -57,6 +57,7 @@
> > > > > >  #include <linux/sched/signal.h>
> > > > > >  #include <linux/sched/mm.h>
> > > > > >  #include <linux/seq_file.h>
> > > > > > +#include <linux/string.h>
> > > > > >  #include <linux/uaccess.h>
> > > > > >  #include <linux/pid_namespace.h>
> > > > > >  #include <linux/security.h>
> > > > > > @@ -66,6 +67,7 @@
> > > > > >  #include <linux/task_work.h>
> > > > > >  
> > > > > >  #include <uapi/linux/android/binder.h>
> > > > > > +#include <uapi/linux/android/binderfs.h>
> > > > > >  
> > > > > >  #include <asm/cacheflush.h>
> > > > > >  
> > > > > > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> > > > > >  	e->target_handle = tr->target.handle;
> > > > > >  	e->data_size = tr->data_size;
> > > > > >  	e->offsets_size = tr->offsets_size;
> > > > > > -	e->context_name = proc->context->name;
> > > > > > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> > > > > 
> > > > > Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> > > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > > fit within the MAX.
> > > > 
> > > > I know but I don't think it's worth special-casing non-binderfs devices.
> > > > First, non-binderfs devices can only be created through a KCONFIG option
> > > > determined at compile time. For stock Android the names are the same for
> > > > all vendors afaik.
> > > 
> > > I am just talking about the name of weirdly named macro here.
> > 
> > You might miss context here: It's named that way because currently only
> > binderfs binder devices are bound to that limit. That's a point I made
> > further below in my previous mail. Non-binderfs devices are not subject
> > to that restriction and when we tried to make them subject to the same
> > it as rejected.
> 
> I know that. I am saying the memcpy is happening for regular binder devices
> as well but the macro has BINDERFS in the name. That's all. It is not a
> significant eye sore. But is a bit odd.

Right, and I told you that we _can't_ rename it to BINDER_MAX because
that check only happens for binderfs devices since you were suggesting
this. If you want to rename to get rid of the this being somehow
apparently odd then you need to introduce that check for non-binderfs
devices too. Or just rename the macro in a follow-up patch. I don't care.

> 
> > <snip>
> > 
> > > 
> > > > Fifth, I already tried to push for validation of non-binderfs binder
> > > > devices a while back when I wrote binderfs and was told that it's not
> > > > needed. Hrydia tried the same and we decided the same thing. So you get
> > > > to be the next person to send a patch. :)
> > > 
> > > I don't follow why we are talking about non-binderfs validation. I am just
> > 
> > Because above you said
> > 
> > > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > > fit within the MAX.
> > 
> > which to me reads like you want generic checks for _all_ binder devices
> > not just for the ones from binderfs.
> 
> No I am not talking about the checks at all, I am talking about the unwanted
> mem copy you are doing for regular binder devices now. Before binderfs this
> would not have happened, but now for regular binder devices we have to do the
> extra mem copies which were avoided before. That was what I was talking about.

I'm sorry but I did not get this at all from:
"So probably there should be a BINDER_MAX_NAME (and associated checks
for whether non BINDERFS names fit within the MAX." 

> 
> But this discussing is getting to bike shedding at this point, and since the
> overhead is likely small, I am Ok with the change (though I don't like very
> much the additional memcpy and the associated space wastage in the
> transaction buffer for regular binder devices).

Feel free to send a follow-up patch handling both separately.

Thanks!
Christian

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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-09 15:10             ` Christian Brauner
@ 2019-10-09 15:37               ` Joel Fernandes
  0 siblings, 0 replies; 15+ messages in thread
From: Joel Fernandes @ 2019-10-09 15:37 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Todd Kjos, jannh, arve, christian, devel, gregkh, linux-kernel,
	maco, tkjos, Hridya Valsaraju

On Wed, Oct 09, 2019 at 05:10:45PM +0200, Christian Brauner wrote:
> On Wed, Oct 09, 2019 at 10:55:58AM -0400, Joel Fernandes wrote:
> > On Wed, Oct 09, 2019 at 04:29:11PM +0200, Christian Brauner wrote:
> > > On Wed, Oct 09, 2019 at 10:21:29AM -0400, Joel Fernandes wrote:
> > > > On Wed, Oct 09, 2019 at 12:40:12PM +0200, Christian Brauner wrote:
> > > > > On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > > > > > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > > > > > > When a binder transaction is initiated on a binder device coming from a
> > > > > > > binderfs instance, a pointer to the name of the binder device is stashed
> > > > > > > in the binder_transaction_log_entry's context_name member. Later on it
> > > > > > > is used to print the name in print_binder_transaction_log_entry(). By
> > > > > > > the time print_binder_transaction_log_entry() accesses context_name
> > > > > > > binderfs_evict_inode() might have already freed the associated memory
> > > > > > > thereby causing a UAF. Do the simple thing and prevent this by copying
> > > > > > > the name of the binder device instead of stashing a pointer to it.
> > > > > > > 
> > > > > > > Reported-by: Jann Horn <jannh@google.com>
> > > > > > > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > > > > > > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > > > > > > Cc: Joel Fernandes <joel@joelfernandes.org>
> > > > > > > Cc: Todd Kjos <tkjos@android.com>
> > > > > > > Cc: Hridya Valsaraju <hridya@google.com>
> > > > > > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > > > > > ---
> > > > > > >  drivers/android/binder.c          | 4 +++-
> > > > > > >  drivers/android/binder_internal.h | 2 +-
> > > > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > > > index c0a491277aca..5b9ac2122e89 100644
> > > > > > > --- a/drivers/android/binder.c
> > > > > > > +++ b/drivers/android/binder.c
> > > > > > > @@ -57,6 +57,7 @@
> > > > > > >  #include <linux/sched/signal.h>
> > > > > > >  #include <linux/sched/mm.h>
> > > > > > >  #include <linux/seq_file.h>
> > > > > > > +#include <linux/string.h>
> > > > > > >  #include <linux/uaccess.h>
> > > > > > >  #include <linux/pid_namespace.h>
> > > > > > >  #include <linux/security.h>
> > > > > > > @@ -66,6 +67,7 @@
> > > > > > >  #include <linux/task_work.h>
> > > > > > >  
> > > > > > >  #include <uapi/linux/android/binder.h>
> > > > > > > +#include <uapi/linux/android/binderfs.h>
> > > > > > >  
> > > > > > >  #include <asm/cacheflush.h>
> > > > > > >  
> > > > > > > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> > > > > > >  	e->target_handle = tr->target.handle;
> > > > > > >  	e->data_size = tr->data_size;
> > > > > > >  	e->offsets_size = tr->offsets_size;
> > > > > > > -	e->context_name = proc->context->name;
> > > > > > > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> > > > > > 
> > > > > > Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> > > > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > > > fit within the MAX.
> > > > > 
> > > > > I know but I don't think it's worth special-casing non-binderfs devices.
> > > > > First, non-binderfs devices can only be created through a KCONFIG option
> > > > > determined at compile time. For stock Android the names are the same for
> > > > > all vendors afaik.
> > > > 
> > > > I am just talking about the name of weirdly named macro here.
> > > 
> > > You might miss context here: It's named that way because currently only
> > > binderfs binder devices are bound to that limit. That's a point I made
> > > further below in my previous mail. Non-binderfs devices are not subject
> > > to that restriction and when we tried to make them subject to the same
> > > it as rejected.
> > 
> > I know that. I am saying the memcpy is happening for regular binder devices
> > as well but the macro has BINDERFS in the name. That's all. It is not a
> > significant eye sore. But is a bit odd.
> 
> Right, and I told you that we _can't_ rename it to BINDER_MAX because
> that check only happens for binderfs devices since you were suggesting
> this. If you want to rename to get rid of the this being somehow
> apparently odd then you need to introduce that check for non-binderfs
> devices too. Or just rename the macro in a follow-up patch. I don't care.

Here in this patch we are doing mem copy for regular binder device name using
a BINDERFS macro name.

> > 
> > > <snip>
> > > 
> > > > 
> > > > > Fifth, I already tried to push for validation of non-binderfs binder
> > > > > devices a while back when I wrote binderfs and was told that it's not
> > > > > needed. Hrydia tried the same and we decided the same thing. So you get
> > > > > to be the next person to send a patch. :)
> > > > 
> > > > I don't follow why we are talking about non-binderfs validation. I am just
> > > 
> > > Because above you said
> > > 
> > > > > > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > > > > > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > > > > > fit within the MAX.
> > > 
> > > which to me reads like you want generic checks for _all_ binder devices
> > > not just for the ones from binderfs.
> > 
> > No I am not talking about the checks at all, I am talking about the unwanted
> > mem copy you are doing for regular binder devices now. Before binderfs this
> > would not have happened, but now for regular binder devices we have to do the
> > extra mem copies which were avoided before. That was what I was talking about.
> 
> I'm sorry but I did not get this at all from:
> "So probably there should be a BINDER_MAX_NAME (and associated checks
> for whether non BINDERFS names fit within the MAX." 

Sorry for the misleading statement. That means I have to improve my
communication game, sorry it is my fault.

> > 
> > But this discussing is getting to bike shedding at this point, and since the
> > overhead is likely small, I am Ok with the change (though I don't like very
> > much the additional memcpy and the associated space wastage in the
> > transaction buffer for regular binder devices).
> 
> Feel free to send a follow-up patch handling both separately.

Ok will do once I get a chance. Thanks for working on the fix!

-  Joel


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

* Re: [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry()
  2019-10-09 10:40     ` Christian Brauner
  2019-10-09 14:21       ` Joel Fernandes
@ 2019-10-09 15:56       ` Todd Kjos
  1 sibling, 0 replies; 15+ messages in thread
From: Todd Kjos @ 2019-10-09 15:56 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Joel Fernandes, Todd Kjos, Jann Horn, Arve Hjønnevåg,
	Christian Brauner, open list:ANDROID DRIVERS, Greg Kroah-Hartman,
	LKML, Martijn Coenen, Hridya Valsaraju

On Wed, Oct 9, 2019 at 3:40 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:

[...]

> >
> > One more thought, this can be made dependent on CONFIG_BINDERFS since regular
> > binder devices cannot be unregistered AFAICS and as Jann said, the problem is
> > BINDERFS specific. That way we avoid the memcpy for _every_ transaction.
> > These can be thundering when Android starts up.
>
> Unless Todd sees this as a real performance problem I'm weary to
> introduce additional checking and record a pointer for non-binderfs and
> a memcpy() for binderfs devices. :)
>

I don't see this as a real problem. In practice, memcpy will be moving
< 10 bytes. Also, by the time this code is in an android device,
CONFIG_BINDERFS will always be enabled since this is how we are
removing binder's use of debugfs. So a micro-optimization of the
!BINDERFS case will not be meaningful.

[...]

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

end of thread, other threads:[~2019-10-09 15:56 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 20:49 UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels Jann Horn
2019-10-07 21:04 ` Todd Kjos
2019-10-07 21:16   ` Hridya Valsaraju
2019-10-07 21:05 ` Christian Brauner
2019-10-08 13:01 ` [PATCH] binder: prevent UAF read in print_binder_transaction_log_entry() Christian Brauner
2019-10-08 17:18   ` Hridya Valsaraju
2019-10-08 18:05   ` Joel Fernandes
2019-10-09 10:40     ` Christian Brauner
2019-10-09 14:21       ` Joel Fernandes
2019-10-09 14:29         ` Christian Brauner
2019-10-09 14:55           ` Joel Fernandes
2019-10-09 15:10             ` Christian Brauner
2019-10-09 15:37               ` Joel Fernandes
2019-10-09 15:56       ` Todd Kjos
2019-10-08 18:52   ` Todd Kjos

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).