linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free.
@ 2020-03-11  7:52 Naresh Kamboju
  2020-03-11  7:52 ` Naresh Kamboju
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Naresh Kamboju @ 2020-03-11  7:52 UTC (permalink / raw)
  To: open list:KERNEL SELFTEST FRAMEWORK, open list
  Cc: John Stultz, tkjos, Christian Brauner, Shuah Khan,
	Greg Kroah-Hartman, Ard Biesheuvel, ardb, Kees Cook, lkft-triage

While running selftest binderfs_test on linux mainline the following
warning on arm64, arm, x86_64 and i386.

[  329.383391] refcount_t: underflow; use-after-free.
[  329.391025] WARNING: CPU: 0 PID: 2604 at
/usr/src/kernel/lib/refcount.c:28 refcount_warn_saturate+0xd4/0x150
[  329.403319] Modules linked in: cls_bpf sch_fq algif_hash af_alg
rfkill tda998x drm_kms_helper drm crct10dif_ce fuse
[  329.413828] CPU: 0 PID: 2604 Comm: binderfs_test Not tainted 5.6.0-rc5 #1
[  329.420640] Hardware name: ARM Juno development board (r2) (DT)
[  329.426584] pstate: 40000005 (nZcv daif -PAN -UAO)
[  329.431402] pc : refcount_warn_saturate+0xd4/0x150
[  329.436216] lr : refcount_warn_saturate+0xd4/0x150
[  329.441026] sp : ffff800013d03a70
[  329.444356] x29: ffff800013d03a70 x28: ffff00092c3f8000
[  329.449694] x27: 0000000000000000 x26: ffff80001236f000
[  329.455033] x25: ffff800012656000 x24: 0000000000000001
[  329.460371] x23: ffff800012656f76 x22: ffff80001265b2c0
[  329.465709] x21: ffff000929035c00 x20: ffff00095cd8ce00
[  329.471048] x19: ffff80001261c848 x18: ffffffffffffffff
[  329.476386] x17: 0000000000000000 x16: 0000000000000000
[  329.481724] x15: ffff80001236fa88 x14: ffff800093d03767
[  329.487062] x13: ffff800013d03775 x12: ffff80001239e000
[  329.492400] x11: 0000000005f5e0ff x10: ffff800013d03700
[  329.497738] x9 : ffff8000126ddc68 x8 : 0000000000000028
[  329.503076] x7 : ffff800010190a5c x6 : ffff00097ef0b428
[  329.508414] x5 : ffff00097ef0b428 x4 : ffff00092c3f8000
[  329.513752] x3 : ffff800012370000 x2 : 0000000000000000
[  329.519090] x1 : 295161095161e100 x0 : 0000000000000000
[  329.524429] Call trace:
[  329.526894]  refcount_warn_saturate+0xd4/0x150
[  329.531362]  binderfs_evict_inode+0xcc/0xe8
[  329.535567]  evict+0xa8/0x188
[  329.538552]  iput+0x278/0x318
[  329.541537]  dentry_unlink_inode+0x154/0x170
[  329.545827]  __dentry_kill+0xc4/0x1d8
[  329.549509]  shrink_dentry_list+0xf4/0x210
[  329.553625]  shrink_dcache_parent+0x124/0x210
[  329.558002]  do_one_tree+0x20/0x50
[  329.561423]  shrink_dcache_for_umount+0x30/0x98
[  329.565975]  generic_shutdown_super+0x2c/0xf8
[  329.570354]  kill_anon_super+0x24/0x48
[  329.574122]  kill_litter_super+0x2c/0x38
[  329.578065]  binderfs_kill_super+0x24/0x48
[  329.582182]  deactivate_locked_super+0x74/0xa0
[  329.586647]  deactivate_super+0x8c/0x98
[  329.590502]  cleanup_mnt+0xd8/0x130
[  329.594008]  __cleanup_mnt+0x20/0x30
[  329.597605]  task_work_run+0x90/0x150
[  329.601287]  do_notify_resume+0x130/0x498
[  329.605317]  work_pending+0x8/0x14
[  329.608736] irq event stamp: 1612
[  329.612072] hardirqs last  enabled at (1611): [<ffff800010190bf4>]
console_unlock+0x514/0x5d8
[  329.620631] hardirqs last disabled at (1612): [<ffff8000100a904c>]
debug_exception_enter+0xac/0xe8
[  329.629622] softirqs last  enabled at (1608): [<ffff8000100818bc>]
__do_softirq+0x4c4/0x578
[  329.638005] softirqs last disabled at (1561): [<ffff80001010b6ac>]
irq_exit+0x144/0x150
[  329.646035] ---[ end trace bac6584738d9306f ]---

Metadata:
---------------
  git branch: master
  git repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  git describe: v5.6-rc5
  kernel-config:
http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/intel-corei7-64/lkft/linux-mainline/2518/config

Full test log,
https://lkft.validation.linaro.org/scheduler/job/1273667#L6591
https://lkft.validation.linaro.org/scheduler/job/1273569#L6222
https://lkft.validation.linaro.org/scheduler/job/1273548#L6126
https://lkft.validation.linaro.org/scheduler/job/1273596#L4687

-- 
Linaro LKFT
https://lkft.linaro.org

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

* WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free.
  2020-03-11  7:52 WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free Naresh Kamboju
@ 2020-03-11  7:52 ` Naresh Kamboju
  2020-03-11  9:13 ` Christian Brauner
  2020-03-11 10:53 ` [PATCH] binderfs: use refcount for binder control devices too Christian Brauner
  2 siblings, 0 replies; 29+ messages in thread
From: Naresh Kamboju @ 2020-03-11  7:52 UTC (permalink / raw)
  To: open list:KERNEL SELFTEST FRAMEWORK, open list
  Cc: John Stultz, tkjos, Christian Brauner, Shuah Khan,
	Greg Kroah-Hartman, Ard Biesheuvel, ardb, Kees Cook, lkft-triage

While running selftest binderfs_test on linux mainline the following
warning on arm64, arm, x86_64 and i386.

[  329.383391] refcount_t: underflow; use-after-free.
[  329.391025] WARNING: CPU: 0 PID: 2604 at
/usr/src/kernel/lib/refcount.c:28 refcount_warn_saturate+0xd4/0x150
[  329.403319] Modules linked in: cls_bpf sch_fq algif_hash af_alg
rfkill tda998x drm_kms_helper drm crct10dif_ce fuse
[  329.413828] CPU: 0 PID: 2604 Comm: binderfs_test Not tainted 5.6.0-rc5 #1
[  329.420640] Hardware name: ARM Juno development board (r2) (DT)
[  329.426584] pstate: 40000005 (nZcv daif -PAN -UAO)
[  329.431402] pc : refcount_warn_saturate+0xd4/0x150
[  329.436216] lr : refcount_warn_saturate+0xd4/0x150
[  329.441026] sp : ffff800013d03a70
[  329.444356] x29: ffff800013d03a70 x28: ffff00092c3f8000
[  329.449694] x27: 0000000000000000 x26: ffff80001236f000
[  329.455033] x25: ffff800012656000 x24: 0000000000000001
[  329.460371] x23: ffff800012656f76 x22: ffff80001265b2c0
[  329.465709] x21: ffff000929035c00 x20: ffff00095cd8ce00
[  329.471048] x19: ffff80001261c848 x18: ffffffffffffffff
[  329.476386] x17: 0000000000000000 x16: 0000000000000000
[  329.481724] x15: ffff80001236fa88 x14: ffff800093d03767
[  329.487062] x13: ffff800013d03775 x12: ffff80001239e000
[  329.492400] x11: 0000000005f5e0ff x10: ffff800013d03700
[  329.497738] x9 : ffff8000126ddc68 x8 : 0000000000000028
[  329.503076] x7 : ffff800010190a5c x6 : ffff00097ef0b428
[  329.508414] x5 : ffff00097ef0b428 x4 : ffff00092c3f8000
[  329.513752] x3 : ffff800012370000 x2 : 0000000000000000
[  329.519090] x1 : 295161095161e100 x0 : 0000000000000000
[  329.524429] Call trace:
[  329.526894]  refcount_warn_saturate+0xd4/0x150
[  329.531362]  binderfs_evict_inode+0xcc/0xe8
[  329.535567]  evict+0xa8/0x188
[  329.538552]  iput+0x278/0x318
[  329.541537]  dentry_unlink_inode+0x154/0x170
[  329.545827]  __dentry_kill+0xc4/0x1d8
[  329.549509]  shrink_dentry_list+0xf4/0x210
[  329.553625]  shrink_dcache_parent+0x124/0x210
[  329.558002]  do_one_tree+0x20/0x50
[  329.561423]  shrink_dcache_for_umount+0x30/0x98
[  329.565975]  generic_shutdown_super+0x2c/0xf8
[  329.570354]  kill_anon_super+0x24/0x48
[  329.574122]  kill_litter_super+0x2c/0x38
[  329.578065]  binderfs_kill_super+0x24/0x48
[  329.582182]  deactivate_locked_super+0x74/0xa0
[  329.586647]  deactivate_super+0x8c/0x98
[  329.590502]  cleanup_mnt+0xd8/0x130
[  329.594008]  __cleanup_mnt+0x20/0x30
[  329.597605]  task_work_run+0x90/0x150
[  329.601287]  do_notify_resume+0x130/0x498
[  329.605317]  work_pending+0x8/0x14
[  329.608736] irq event stamp: 1612
[  329.612072] hardirqs last  enabled at (1611): [<ffff800010190bf4>]
console_unlock+0x514/0x5d8
[  329.620631] hardirqs last disabled at (1612): [<ffff8000100a904c>]
debug_exception_enter+0xac/0xe8
[  329.629622] softirqs last  enabled at (1608): [<ffff8000100818bc>]
__do_softirq+0x4c4/0x578
[  329.638005] softirqs last disabled at (1561): [<ffff80001010b6ac>]
irq_exit+0x144/0x150
[  329.646035] ---[ end trace bac6584738d9306f ]---

Metadata:
---------------
  git branch: master
  git repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
  git describe: v5.6-rc5
  kernel-config:
http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/intel-corei7-64/lkft/linux-mainline/2518/config

Full test log,
https://lkft.validation.linaro.org/scheduler/job/1273667#L6591
https://lkft.validation.linaro.org/scheduler/job/1273569#L6222
https://lkft.validation.linaro.org/scheduler/job/1273548#L6126
https://lkft.validation.linaro.org/scheduler/job/1273596#L4687

-- 
Linaro LKFT
https://lkft.linaro.org


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

* Re: WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free.
  2020-03-11  7:52 WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free Naresh Kamboju
  2020-03-11  7:52 ` Naresh Kamboju
@ 2020-03-11  9:13 ` Christian Brauner
  2020-03-11  9:13   ` Christian Brauner
  2020-03-11 10:53 ` [PATCH] binderfs: use refcount for binder control devices too Christian Brauner
  2 siblings, 1 reply; 29+ messages in thread
From: Christian Brauner @ 2020-03-11  9:13 UTC (permalink / raw)
  To: Naresh Kamboju, open list:KERNEL SELFTEST FRAMEWORK, open list
  Cc: John Stultz, tkjos, Shuah Khan, Greg Kroah-Hartman,
	Ard Biesheuvel, ardb, Kees Cook, lkft-triage

On March 11, 2020 8:52:16 AM GMT+01:00, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>While running selftest binderfs_test on linux mainline the following
>warning on arm64, arm, x86_64 and i386.
>
>[  329.383391] refcount_t: underflow; use-after-free.
>[  329.391025] WARNING: CPU: 0 PID: 2604 at
>/usr/src/kernel/lib/refcount.c:28 refcount_warn_saturate+0xd4/0x150
>[  329.403319] Modules linked in: cls_bpf sch_fq algif_hash af_alg
>rfkill tda998x drm_kms_helper drm crct10dif_ce fuse
>[  329.413828] CPU: 0 PID: 2604 Comm: binderfs_test Not tainted
>5.6.0-rc5 #1
>[  329.420640] Hardware name: ARM Juno development board (r2) (DT)
>[  329.426584] pstate: 40000005 (nZcv daif -PAN -UAO)
>[  329.431402] pc : refcount_warn_saturate+0xd4/0x150
>[  329.436216] lr : refcount_warn_saturate+0xd4/0x150
>[  329.441026] sp : ffff800013d03a70
>[  329.444356] x29: ffff800013d03a70 x28: ffff00092c3f8000
>[  329.449694] x27: 0000000000000000 x26: ffff80001236f000
>[  329.455033] x25: ffff800012656000 x24: 0000000000000001
>[  329.460371] x23: ffff800012656f76 x22: ffff80001265b2c0
>[  329.465709] x21: ffff000929035c00 x20: ffff00095cd8ce00
>[  329.471048] x19: ffff80001261c848 x18: ffffffffffffffff
>[  329.476386] x17: 0000000000000000 x16: 0000000000000000
>[  329.481724] x15: ffff80001236fa88 x14: ffff800093d03767
>[  329.487062] x13: ffff800013d03775 x12: ffff80001239e000
>[  329.492400] x11: 0000000005f5e0ff x10: ffff800013d03700
>[  329.497738] x9 : ffff8000126ddc68 x8 : 0000000000000028
>[  329.503076] x7 : ffff800010190a5c x6 : ffff00097ef0b428
>[  329.508414] x5 : ffff00097ef0b428 x4 : ffff00092c3f8000
>[  329.513752] x3 : ffff800012370000 x2 : 0000000000000000
>[  329.519090] x1 : 295161095161e100 x0 : 0000000000000000
>[  329.524429] Call trace:
>[  329.526894]  refcount_warn_saturate+0xd4/0x150
>[  329.531362]  binderfs_evict_inode+0xcc/0xe8
>[  329.535567]  evict+0xa8/0x188
>[  329.538552]  iput+0x278/0x318
>[  329.541537]  dentry_unlink_inode+0x154/0x170
>[  329.545827]  __dentry_kill+0xc4/0x1d8
>[  329.549509]  shrink_dentry_list+0xf4/0x210
>[  329.553625]  shrink_dcache_parent+0x124/0x210
>[  329.558002]  do_one_tree+0x20/0x50
>[  329.561423]  shrink_dcache_for_umount+0x30/0x98
>[  329.565975]  generic_shutdown_super+0x2c/0xf8
>[  329.570354]  kill_anon_super+0x24/0x48
>[  329.574122]  kill_litter_super+0x2c/0x38
>[  329.578065]  binderfs_kill_super+0x24/0x48
>[  329.582182]  deactivate_locked_super+0x74/0xa0
>[  329.586647]  deactivate_super+0x8c/0x98
>[  329.590502]  cleanup_mnt+0xd8/0x130
>[  329.594008]  __cleanup_mnt+0x20/0x30
>[  329.597605]  task_work_run+0x90/0x150
>[  329.601287]  do_notify_resume+0x130/0x498
>[  329.605317]  work_pending+0x8/0x14
>[  329.608736] irq event stamp: 1612
>[  329.612072] hardirqs last  enabled at (1611): [<ffff800010190bf4>]
>console_unlock+0x514/0x5d8
>[  329.620631] hardirqs last disabled at (1612): [<ffff8000100a904c>]
>debug_exception_enter+0xac/0xe8
>[  329.629622] softirqs last  enabled at (1608): [<ffff8000100818bc>]
>__do_softirq+0x4c4/0x578
>[  329.638005] softirqs last disabled at (1561): [<ffff80001010b6ac>]
>irq_exit+0x144/0x150
>[  329.646035] ---[ end trace bac6584738d9306f ]---
>
>Metadata:
>---------------
>  git branch: master
>git repo:
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>  git describe: v5.6-rc5
>  kernel-config:
>http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/intel-corei7-64/lkft/linux-mainline/2518/config
>
>Full test log,
>https://lkft.validation.linaro.org/scheduler/job/1273667#L6591
>https://lkft.validation.linaro.org/scheduler/job/1273569#L6222
>https://lkft.validation.linaro.org/scheduler/job/1273548#L6126
>https://lkft.validation.linaro.org/scheduler/job/1273596#L4687

Thanks, I'll take a look in a little bit.

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

* Re: WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free.
  2020-03-11  9:13 ` Christian Brauner
@ 2020-03-11  9:13   ` Christian Brauner
  0 siblings, 0 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-11  9:13 UTC (permalink / raw)
  To: Naresh Kamboju, open list:KERNEL SELFTEST FRAMEWORK, open list
  Cc: John Stultz, tkjos, Shuah Khan, Greg Kroah-Hartman,
	Ard Biesheuvel, ardb, Kees Cook, lkft-triage

On March 11, 2020 8:52:16 AM GMT+01:00, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>While running selftest binderfs_test on linux mainline the following
>warning on arm64, arm, x86_64 and i386.
>
>[  329.383391] refcount_t: underflow; use-after-free.
>[  329.391025] WARNING: CPU: 0 PID: 2604 at
>/usr/src/kernel/lib/refcount.c:28 refcount_warn_saturate+0xd4/0x150
>[  329.403319] Modules linked in: cls_bpf sch_fq algif_hash af_alg
>rfkill tda998x drm_kms_helper drm crct10dif_ce fuse
>[  329.413828] CPU: 0 PID: 2604 Comm: binderfs_test Not tainted
>5.6.0-rc5 #1
>[  329.420640] Hardware name: ARM Juno development board (r2) (DT)
>[  329.426584] pstate: 40000005 (nZcv daif -PAN -UAO)
>[  329.431402] pc : refcount_warn_saturate+0xd4/0x150
>[  329.436216] lr : refcount_warn_saturate+0xd4/0x150
>[  329.441026] sp : ffff800013d03a70
>[  329.444356] x29: ffff800013d03a70 x28: ffff00092c3f8000
>[  329.449694] x27: 0000000000000000 x26: ffff80001236f000
>[  329.455033] x25: ffff800012656000 x24: 0000000000000001
>[  329.460371] x23: ffff800012656f76 x22: ffff80001265b2c0
>[  329.465709] x21: ffff000929035c00 x20: ffff00095cd8ce00
>[  329.471048] x19: ffff80001261c848 x18: ffffffffffffffff
>[  329.476386] x17: 0000000000000000 x16: 0000000000000000
>[  329.481724] x15: ffff80001236fa88 x14: ffff800093d03767
>[  329.487062] x13: ffff800013d03775 x12: ffff80001239e000
>[  329.492400] x11: 0000000005f5e0ff x10: ffff800013d03700
>[  329.497738] x9 : ffff8000126ddc68 x8 : 0000000000000028
>[  329.503076] x7 : ffff800010190a5c x6 : ffff00097ef0b428
>[  329.508414] x5 : ffff00097ef0b428 x4 : ffff00092c3f8000
>[  329.513752] x3 : ffff800012370000 x2 : 0000000000000000
>[  329.519090] x1 : 295161095161e100 x0 : 0000000000000000
>[  329.524429] Call trace:
>[  329.526894]  refcount_warn_saturate+0xd4/0x150
>[  329.531362]  binderfs_evict_inode+0xcc/0xe8
>[  329.535567]  evict+0xa8/0x188
>[  329.538552]  iput+0x278/0x318
>[  329.541537]  dentry_unlink_inode+0x154/0x170
>[  329.545827]  __dentry_kill+0xc4/0x1d8
>[  329.549509]  shrink_dentry_list+0xf4/0x210
>[  329.553625]  shrink_dcache_parent+0x124/0x210
>[  329.558002]  do_one_tree+0x20/0x50
>[  329.561423]  shrink_dcache_for_umount+0x30/0x98
>[  329.565975]  generic_shutdown_super+0x2c/0xf8
>[  329.570354]  kill_anon_super+0x24/0x48
>[  329.574122]  kill_litter_super+0x2c/0x38
>[  329.578065]  binderfs_kill_super+0x24/0x48
>[  329.582182]  deactivate_locked_super+0x74/0xa0
>[  329.586647]  deactivate_super+0x8c/0x98
>[  329.590502]  cleanup_mnt+0xd8/0x130
>[  329.594008]  __cleanup_mnt+0x20/0x30
>[  329.597605]  task_work_run+0x90/0x150
>[  329.601287]  do_notify_resume+0x130/0x498
>[  329.605317]  work_pending+0x8/0x14
>[  329.608736] irq event stamp: 1612
>[  329.612072] hardirqs last  enabled at (1611): [<ffff800010190bf4>]
>console_unlock+0x514/0x5d8
>[  329.620631] hardirqs last disabled at (1612): [<ffff8000100a904c>]
>debug_exception_enter+0xac/0xe8
>[  329.629622] softirqs last  enabled at (1608): [<ffff8000100818bc>]
>__do_softirq+0x4c4/0x578
>[  329.638005] softirqs last disabled at (1561): [<ffff80001010b6ac>]
>irq_exit+0x144/0x150
>[  329.646035] ---[ end trace bac6584738d9306f ]---
>
>Metadata:
>---------------
>  git branch: master
>git repo:
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>  git describe: v5.6-rc5
>  kernel-config:
>http://snapshots.linaro.org/openembedded/lkft/lkft/sumo/intel-corei7-64/lkft/linux-mainline/2518/config
>
>Full test log,
>https://lkft.validation.linaro.org/scheduler/job/1273667#L6591
>https://lkft.validation.linaro.org/scheduler/job/1273569#L6222
>https://lkft.validation.linaro.org/scheduler/job/1273548#L6126
>https://lkft.validation.linaro.org/scheduler/job/1273596#L4687

Thanks, I'll take a look in a little bit.


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

* [PATCH] binderfs: use refcount for binder control devices too
  2020-03-11  7:52 WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free Naresh Kamboju
  2020-03-11  7:52 ` Naresh Kamboju
  2020-03-11  9:13 ` Christian Brauner
@ 2020-03-11 10:53 ` Christian Brauner
  2020-03-11 18:25   ` Todd Kjos
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
  2 siblings, 2 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-11 10:53 UTC (permalink / raw)
  To: naresh.kamboju
  Cc: ard.biesheuvel, ardb, christian.brauner, gregkh, john.stultz,
	keescook, linux-kernel, linux-kselftest, lkft-triage, shuah,
	tkjos, stable

Binderfs binder-control devices are cleaned up via binderfs_evict_inode
too() which will use refcount_dec_and_test(). However, we missed to set
the refcount for binderfs binder-control devices and so we underflowed
when the binderfs instance got unmounted. Pretty obvious oversight and
should have been part of the more general UAF fix. The good news is that
having test cases (suprisingly) helps.

Technically, we could detect that we're about to cleanup the
binder-control dentry in binderfs_evict_inode() and then simply clean it
up. But that makes the assumption that the binder driver itself will
never make use of a binderfs binder-control device after the binderfs
instance it belongs to has been unmounted and the superblock for it been
destroyed. While it is unlikely to ever come to this let's be on the
safe side. Performance-wise this also really doesn't matter since the
binder-control device is only every really when creating the binderfs
filesystem or creating additional binder devices. Both operations are
pretty rare.

Fixes: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
Link: https://lore.kernel.org/r/CA+G9fYusdfg7PMfC9Xce-xLT7NiyKSbgojpK35GOm=Pf9jXXrA@mail.gmail.com
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Cc: stable@vger.kernel.org
Cc: Todd Kjos <tkjos@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 drivers/android/binderfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 110e41f920c2..f303106b3362 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -448,6 +448,7 @@ static int binderfs_binder_ctl_create(struct super_block *sb)
 	inode->i_uid = info->root_uid;
 	inode->i_gid = info->root_gid;
 
+	refcount_set(&device->ref, 1);
 	device->binderfs_inode = inode;
 	device->miscdev.minor = minor;
 

base-commit: 2c523b344dfa65a3738e7039832044aa133c75fb
-- 
2.25.1


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

* Re: [PATCH] binderfs: use refcount for binder control devices too
  2020-03-11 10:53 ` [PATCH] binderfs: use refcount for binder control devices too Christian Brauner
@ 2020-03-11 18:25   ` Todd Kjos
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
  1 sibling, 0 replies; 29+ messages in thread
From: Todd Kjos @ 2020-03-11 18:25 UTC (permalink / raw)
  To: Christian Brauner
  Cc: naresh.kamboju, ard.biesheuvel, ardb, Greg Kroah-Hartman,
	John Stultz, Kees Cook, LKML, linux-kselftest, lkft-triage,
	shuah, stable

On Wed, Mar 11, 2020 at 3:53 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> Binderfs binder-control devices are cleaned up via binderfs_evict_inode
> too() which will use refcount_dec_and_test(). However, we missed to set
> the refcount for binderfs binder-control devices and so we underflowed
> when the binderfs instance got unmounted. Pretty obvious oversight and
> should have been part of the more general UAF fix. The good news is that
> having test cases (suprisingly) helps.
>
> Technically, we could detect that we're about to cleanup the
> binder-control dentry in binderfs_evict_inode() and then simply clean it
> up. But that makes the assumption that the binder driver itself will
> never make use of a binderfs binder-control device after the binderfs
> instance it belongs to has been unmounted and the superblock for it been
> destroyed. While it is unlikely to ever come to this let's be on the
> safe side. Performance-wise this also really doesn't matter since the
> binder-control device is only every really when creating the binderfs
> filesystem or creating additional binder devices. Both operations are
> pretty rare.
>
> Fixes: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> Link: https://lore.kernel.org/r/CA+G9fYusdfg7PMfC9Xce-xLT7NiyKSbgojpK35GOm=Pf9jXXrA@mail.gmail.com
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: stable@vger.kernel.org
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

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

> ---
>  drivers/android/binderfs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 110e41f920c2..f303106b3362 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -448,6 +448,7 @@ static int binderfs_binder_ctl_create(struct super_block *sb)
>         inode->i_uid = info->root_uid;
>         inode->i_gid = info->root_gid;
>
> +       refcount_set(&device->ref, 1);
>         device->binderfs_inode = inode;
>         device->miscdev.minor = minor;
>
>
> base-commit: 2c523b344dfa65a3738e7039832044aa133c75fb
> --
> 2.25.1
>

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

* [PATCH 1/3] binderfs: port tests to test harness infrastructure
  2020-03-11 10:53 ` [PATCH] binderfs: use refcount for binder control devices too Christian Brauner
  2020-03-11 18:25   ` Todd Kjos
@ 2020-03-12 13:15   ` Christian Brauner
  2020-03-12 13:15     ` [PATCH 2/3] binderfs: add stress test for binderfs binder devices Christian Brauner
                       ` (5 more replies)
  1 sibling, 6 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-12 13:15 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Todd Kjos
  Cc: ard.biesheuvel, ardb, john.stultz, keescook, linux-kernel,
	linux-kselftest, naresh.kamboju, shuah, Arve Hjønnevåg,
	Martijn Coenen, Joel Fernandes, hridya, kernel-team,
	Christian Brauner

Makes for nicer output and prepares for additional tests.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 .../selftests/filesystems/binderfs/Makefile      |  2 ++
 .../filesystems/binderfs/binderfs_test.c         | 16 ++++++----------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
index 58cb659b56b4..75315d9ba7a9 100644
--- a/tools/testing/selftests/filesystems/binderfs/Makefile
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -3,4 +3,6 @@
 CFLAGS += -I../../../../../usr/include/
 TEST_GEN_PROGS := binderfs_test
 
+binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
+
 include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 8c2ed962e1c7..d03ed8eed5eb 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -15,7 +15,9 @@
 #include <unistd.h>
 #include <linux/android/binder.h>
 #include <linux/android/binderfs.h>
+
 #include "../../kselftest.h"
+#include "../../kselftest_harness.h"
 
 static ssize_t write_nointr(int fd, const void *buf, size_t count)
 {
@@ -252,24 +254,18 @@ static void __do_binderfs_test(void)
 	ksft_inc_pass_cnt();
 }
 
-static void binderfs_test_privileged()
+TEST(binderfs_test_privileged)
 {
 	if (geteuid() != 0)
-		ksft_print_msg(
-			"Tests are not run as root. Skipping privileged tests\n");
+		ksft_print_msg("Tests are not run as root. Skipping privileged tests\n");
 	else
 		__do_binderfs_test();
 }
 
-static void binderfs_test_unprivileged()
+TEST(binderfs_test_unprivileged)
 {
 	change_to_userns();
 	__do_binderfs_test();
 }
 
-int main(int argc, char *argv[])
-{
-	binderfs_test_privileged();
-	binderfs_test_unprivileged();
-	ksft_exit_pass();
-}
+TEST_HARNESS_MAIN

base-commit: 2c523b344dfa65a3738e7039832044aa133c75fb
-- 
2.25.1


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

* [PATCH 2/3] binderfs: add stress test for binderfs binder devices
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
@ 2020-03-12 13:15     ` Christian Brauner
  2020-03-12 23:53       ` Kees Cook
  2020-03-12 13:15     ` [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint Christian Brauner
                       ` (4 subsequent siblings)
  5 siblings, 1 reply; 29+ messages in thread
From: Christian Brauner @ 2020-03-12 13:15 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Todd Kjos
  Cc: ard.biesheuvel, ardb, john.stultz, keescook, linux-kernel,
	linux-kselftest, naresh.kamboju, shuah, Arve Hjønnevåg,
	Martijn Coenen, Joel Fernandes, hridya, kernel-team,
	Christian Brauner

This adds a stress test that should hopefully help us catch regressions
for [1], [2], and [3].

[1]: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
[2]: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
[3]: 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 .../selftests/filesystems/binderfs/Makefile   |   2 +-
 .../filesystems/binderfs/binderfs_test.c      | 416 ++++++++++++++----
 2 files changed, 325 insertions(+), 93 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
index 75315d9ba7a9..8af25ae96049 100644
--- a/tools/testing/selftests/filesystems/binderfs/Makefile
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-CFLAGS += -I../../../../../usr/include/
+CFLAGS += -I../../../../../usr/include/ -pthread
 TEST_GEN_PROGS := binderfs_test
 
 binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index d03ed8eed5eb..313ffad15614 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -3,15 +3,20 @@
 #define _GNU_SOURCE
 #include <errno.h>
 #include <fcntl.h>
+#include <pthread.h>
 #include <sched.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/fsuid.h>
 #include <sys/ioctl.h>
 #include <sys/mount.h>
+#include <sys/socket.h>
 #include <sys/stat.h>
+#include <sys/sysinfo.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <linux/android/binder.h>
 #include <linux/android/binderfs.h>
@@ -19,100 +24,26 @@
 #include "../../kselftest.h"
 #include "../../kselftest_harness.h"
 
-static ssize_t write_nointr(int fd, const void *buf, size_t count)
-{
-	ssize_t ret;
-again:
-	ret = write(fd, buf, count);
-	if (ret < 0 && errno == EINTR)
-		goto again;
+#define DEFAULT_THREADS 4
 
-	return ret;
-}
+#define PTR_TO_INT(p) ((int)((intptr_t)(p)))
+#define INT_TO_PTR(u) ((void *)((intptr_t)(u)))
 
-static void write_to_file(const char *filename, const void *buf, size_t count,
-			  int allowed_errno)
-{
-	int fd, saved_errno;
-	ssize_t ret;
-
-	fd = open(filename, O_WRONLY | O_CLOEXEC);
-	if (fd < 0)
-		ksft_exit_fail_msg("%s - Failed to open file %s\n",
-				   strerror(errno), filename);
-
-	ret = write_nointr(fd, buf, count);
-	if (ret < 0) {
-		if (allowed_errno && (errno == allowed_errno)) {
-			close(fd);
-			return;
-		}
-
-		goto on_error;
+#define close_prot_errno_disarm(fd) \
+	if (fd >= 0) {              \
+		int _e_ = errno;    \
+		close(fd);          \
+		errno = _e_;        \
+		fd = -EBADF;        \
 	}
 
-	if ((size_t)ret != count)
-		goto on_error;
-
-	close(fd);
-	return;
-
-on_error:
-	saved_errno = errno;
-	close(fd);
-	errno = saved_errno;
-
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to write to file %s\n",
-				   strerror(errno), filename);
+#define log_exit(format, ...)                                                  \
+	({                                                                     \
+		fprintf(stderr, format "\n", ##__VA_ARGS__);                   \
+		exit(EXIT_FAILURE);                                            \
+	})
 
-	ksft_exit_fail_msg("Failed to write to file %s\n", filename);
-}
-
-static void change_to_userns(void)
-{
-	int ret;
-	uid_t uid;
-	gid_t gid;
-	/* {g,u}id_map files only allow a max of 4096 bytes written to them */
-	char idmap[4096];
-
-	uid = getuid();
-	gid = getgid();
-
-	ret = unshare(CLONE_NEWUSER);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
-				   strerror(errno));
-
-	write_to_file("/proc/self/setgroups", "deny", strlen("deny"), ENOENT);
-
-	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", uid);
-	if (ret < 0 || (size_t)ret >= sizeof(idmap))
-		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
-				   strerror(errno));
-
-	write_to_file("/proc/self/uid_map", idmap, strlen(idmap), 0);
-
-	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", gid);
-	if (ret < 0 || (size_t)ret >= sizeof(idmap))
-		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
-				   strerror(errno));
-
-	write_to_file("/proc/self/gid_map", idmap, strlen(idmap), 0);
-
-	ret = setgid(0);
-	if (ret)
-		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
-				   strerror(errno));
-
-	ret = setuid(0);
-	if (ret)
-		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
-				   strerror(errno));
-}
-
-static void change_to_mountns(void)
+static void change_mountns(void)
 {
 	int ret;
 
@@ -143,7 +74,7 @@ static void __do_binderfs_test(void)
 	struct binderfs_device device = { 0 };
 	struct binder_version version = { 0 };
 
-	change_to_mountns();
+	change_mountns();
 
 	ret = mkdir("/dev/binderfs", 0755);
 	if (ret < 0) {
@@ -254,6 +185,283 @@ static void __do_binderfs_test(void)
 	ksft_inc_pass_cnt();
 }
 
+static int wait_for_pid(pid_t pid)
+{
+	int status, ret;
+
+again:
+	ret = waitpid(pid, &status, 0);
+	if (ret == -1) {
+		if (errno == EINTR)
+			goto again;
+
+		return -1;
+	}
+
+	if (!WIFEXITED(status))
+		return -1;
+
+	return WEXITSTATUS(status);
+}
+
+static int setid_userns_root(void)
+{
+	if (setuid(0))
+		return -1;
+	if (setgid(0))
+		return -1;
+
+	setfsuid(0);
+	setfsgid(0);
+
+	return 0;
+}
+
+enum idmap_type {
+	UID_MAP,
+	GID_MAP,
+};
+
+static ssize_t read_nointr(int fd, void *buf, size_t count)
+{
+	ssize_t ret;
+again:
+	ret = read(fd, buf, count);
+	if (ret < 0 && errno == EINTR)
+		goto again;
+
+	return ret;
+}
+
+static ssize_t write_nointr(int fd, const void *buf, size_t count)
+{
+	ssize_t ret;
+again:
+	ret = write(fd, buf, count);
+	if (ret < 0 && errno == EINTR)
+		goto again;
+
+	return ret;
+}
+
+static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
+			    size_t buf_size)
+{
+	int fd;
+	int ret;
+	char path[4096];
+
+	if (type == GID_MAP) {
+		int setgroups_fd;
+
+		snprintf(path, sizeof(path), "/proc/%d/setgroups", pid);
+		setgroups_fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
+		if (setgroups_fd < 0 && errno != ENOENT)
+			return -1;
+
+		if (setgroups_fd >= 0) {
+			ret = write_nointr(setgroups_fd, "deny", sizeof("deny") - 1);
+			close_prot_errno_disarm(setgroups_fd);
+			if (ret != sizeof("deny") - 1)
+				return -1;
+		}
+	}
+
+	switch (type) {
+	case UID_MAP:
+		ret = snprintf(path, sizeof(path), "/proc/%d/uid_map", pid);
+		break;
+	case GID_MAP:
+		ret = snprintf(path, sizeof(path), "/proc/%d/gid_map", pid);
+		break;
+	default:
+		return -1;
+	}
+	if (ret < 0 || ret >= sizeof(path))
+		return -E2BIG;
+
+	fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
+	if (fd < 0)
+		return -1;
+
+	ret = write_nointr(fd, buf, buf_size);
+	close_prot_errno_disarm(fd);
+	if (ret != buf_size)
+		return -1;
+
+	return 0;
+}
+
+static void change_userns(int syncfds[2])
+{
+	int ret;
+	char buf;
+
+	close_prot_errno_disarm(syncfds[1]);
+
+	ret = unshare(CLONE_NEWUSER);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
+				   strerror(errno));
+
+	ret = write_nointr(syncfds[0], "1", 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("write_nointr() failed\n");
+
+	ret = read_nointr(syncfds[0], &buf, 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("read_nointr() failed\n");
+
+	close_prot_errno_disarm(syncfds[0]);
+
+	if (setid_userns_root())
+		ksft_exit_fail_msg("setid_userns_root() failed");
+}
+
+static void change_idmaps(int syncfds[2], pid_t pid)
+{
+	int ret;
+	char buf;
+	char id_map[4096];
+
+	close_prot_errno_disarm(syncfds[0]);
+
+	ret = read_nointr(syncfds[1], &buf, 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("read_nointr() failed\n");
+
+	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
+	ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
+	if (ret)
+		ksft_exit_fail_msg("write_id_mapping(UID_MAP) failed");
+
+	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
+	ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
+	if (ret)
+		ksft_exit_fail_msg("write_id_mapping(GID_MAP) failed");
+
+	ret = write_nointr(syncfds[1], "1", 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("write_nointr() failed");
+
+	close_prot_errno_disarm(syncfds[1]);
+}
+
+static void *binder_version_thread(void *data)
+{
+	int fd = PTR_TO_INT(data);
+	struct binder_version version = { 0 };
+	int ret;
+
+	ret = ioctl(fd, BINDER_VERSION, &version);
+	if (ret < 0)
+		ksft_print_msg("%s - Failed to open perform BINDER_VERSION request\n", strerror(errno));
+
+	pthread_exit(data);
+}
+
+/*
+ * Regression test:
+ * 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
+ * f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
+ * 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
+ */
+TEST(binderfs_stress)
+{
+	int fds[1000];
+	int syncfds[2];
+	pid_t pid;
+	int fd, ret;
+	size_t len;
+	struct binderfs_device device = { 0 };
+
+	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
+
+	pid = fork();
+	if (pid < 0) {
+		close_prot_errno_disarm(syncfds[0]);
+		close_prot_errno_disarm(syncfds[1]);
+		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
+	}
+
+	if (pid == 0) {
+		int i, j, k, nthreads;
+		pthread_attr_t attr;
+		pthread_t threads[DEFAULT_THREADS];
+		change_userns(syncfds);
+		change_mountns();
+
+		ret = mkdir("/tmp/binderfs", 0755);
+		if (ret < 0 && errno != EEXIST)
+			log_exit("%s - Failed to create binderfs mountpoint\n", strerror(errno));
+
+		ret = mount(NULL, "/tmp/binderfs", "binder", 0, 0);
+		if (ret < 0)
+			log_exit("%s - Failed to mount binderfs\n", strerror(errno));
+
+		for (int i = 0; i < ARRAY_SIZE(fds); i++) {
+			char path[4096];
+
+			fd = open("/tmp/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
+			if (fd < 0)
+				log_exit("%s - Failed to open binder-control device\n", strerror(errno));
+
+			memset(&device, 0, sizeof(device));
+			snprintf(device.name, sizeof(device.name), "%d", i);
+			ret = ioctl(fd, BINDER_CTL_ADD, &device);
+			close_prot_errno_disarm(fd);
+			if (ret < 0)
+				log_exit("%s - Failed to allocate new binder device\n", strerror(errno));
+
+			snprintf(path, sizeof(path), "/tmp/binderfs/%d", i);
+			fds[i] = open(path, O_RDONLY | O_CLOEXEC);
+			if (fds[i] < 0)
+				log_exit("%s - Failed to open binder device\n", strerror(errno));
+		}
+
+		ret = umount2("/tmp/binderfs", MNT_DETACH);
+		if (ret < 0)
+			log_exit("%s - Failed to unmount binderfs\n", strerror(errno));
+
+		nthreads = get_nprocs_conf();
+		if (nthreads > DEFAULT_THREADS)
+			nthreads = DEFAULT_THREADS;
+
+		pthread_attr_init(&attr);
+		for (k = 0; k < ARRAY_SIZE(fds); k++) {
+			for (i = 0; i < nthreads; i++) {
+				ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
+				if (ret) {
+					ksft_print_msg("%s - Failed to create thread %d\n", strerror(errno), i);
+					break;
+				}
+			}
+
+			for (j = 0; j < i; j++) {
+				void *fdptr = NULL;
+
+				ret = pthread_join(threads[j], &fdptr);
+				if (ret)
+					ksft_print_msg("%s - Failed to join thread %d for fd %d\n", strerror(errno), j, PTR_TO_INT(fdptr));
+			}
+		}
+		pthread_attr_destroy(&attr);
+
+		for (k = 0; k < ARRAY_SIZE(fds); k++)
+			close(fds[k]);
+
+		exit(EXIT_SUCCESS);
+	}
+
+	change_idmaps(syncfds, pid);
+
+	ret = wait_for_pid(pid);
+	if (ret)
+		ksft_exit_fail_msg("wait_for_pid() failed");
+}
+
 TEST(binderfs_test_privileged)
 {
 	if (geteuid() != 0)
@@ -264,8 +472,32 @@ TEST(binderfs_test_privileged)
 
 TEST(binderfs_test_unprivileged)
 {
-	change_to_userns();
-	__do_binderfs_test();
+	int ret;
+	int syncfds[2];
+	pid_t pid;
+
+	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
+
+	pid = fork();
+	if (pid < 0) {
+		close_prot_errno_disarm(syncfds[0]);
+		close_prot_errno_disarm(syncfds[1]);
+		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
+	}
+
+	if (pid == 0) {
+		change_userns(syncfds);
+		__do_binderfs_test();
+		exit(EXIT_SUCCESS);
+	}
+
+	change_idmaps(syncfds, pid);
+
+	ret = wait_for_pid(pid);
+	if (ret)
+		ksft_exit_fail_msg("wait_for_pid() failed");
 }
 
 TEST_HARNESS_MAIN
-- 
2.25.1


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

* [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
  2020-03-12 13:15     ` [PATCH 2/3] binderfs: add stress test for binderfs binder devices Christian Brauner
@ 2020-03-12 13:15     ` Christian Brauner
  2020-03-12 23:54       ` Kees Cook
  2020-03-12 21:24     ` [PATCH] binderfs: port to new mount api Christian Brauner
                       ` (3 subsequent siblings)
  5 siblings, 1 reply; 29+ messages in thread
From: Christian Brauner @ 2020-03-12 13:15 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Todd Kjos
  Cc: ard.biesheuvel, ardb, john.stultz, keescook, linux-kernel,
	linux-kselftest, naresh.kamboju, shuah, Arve Hjønnevåg,
	Martijn Coenen, Joel Fernandes, hridya, kernel-team,
	Christian Brauner

Unprivileged users will be able to create directories in there. The
unprivileged test for /dev wouldn't have worked on most systems.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 .../filesystems/binderfs/binderfs_test.c      | 30 +++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 313ffad15614..d6e61998d20a 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -76,7 +76,7 @@ static void __do_binderfs_test(void)
 
 	change_mountns();
 
-	ret = mkdir("/dev/binderfs", 0755);
+	ret = mkdir("/tmp/binderfs", 0755);
 	if (ret < 0) {
 		if (errno != EEXIST)
 			ksft_exit_fail_msg(
@@ -86,13 +86,13 @@ static void __do_binderfs_test(void)
 		keep = true;
 	}
 
-	ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
+	ret = mount(NULL, "/tmp/binderfs", "binder", 0, 0);
 	if (ret < 0) {
 		if (errno != ENODEV)
 			ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
 					   strerror(errno));
 
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		keep ? : rmdir_protect_errno("/tmp/binderfs");
 		ksft_exit_skip(
 			"The Android binderfs filesystem is not available\n");
 	}
@@ -102,7 +102,7 @@ static void __do_binderfs_test(void)
 
 	memcpy(device.name, "my-binder", strlen("my-binder"));
 
-	fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
+	fd = open("/tmp/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
 		ksft_exit_fail_msg(
 			"%s - Failed to open binder-control device\n",
@@ -113,7 +113,7 @@ static void __do_binderfs_test(void)
 	close(fd);
 	errno = saved_errno;
 	if (ret < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		keep ? : rmdir_protect_errno("/tmp/binderfs");
 		ksft_exit_fail_msg(
 			"%s - Failed to allocate new binder device\n",
 			strerror(errno));
@@ -126,9 +126,9 @@ static void __do_binderfs_test(void)
 	/* binder device allocation test passed */
 	ksft_inc_pass_cnt();
 
-	fd = open("/dev/binderfs/my-binder", O_CLOEXEC | O_RDONLY);
+	fd = open("/tmp/binderfs/my-binder", O_CLOEXEC | O_RDONLY);
 	if (fd < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		keep ? : rmdir_protect_errno("/tmp/binderfs");
 		ksft_exit_fail_msg("%s - Failed to open my-binder device\n",
 				   strerror(errno));
 	}
@@ -138,7 +138,7 @@ static void __do_binderfs_test(void)
 	close(fd);
 	errno = saved_errno;
 	if (ret < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		keep ? : rmdir_protect_errno("/tmp/binderfs");
 		ksft_exit_fail_msg(
 			"%s - Failed to open perform BINDER_VERSION request\n",
 			strerror(errno));
@@ -150,9 +150,9 @@ static void __do_binderfs_test(void)
 	/* binder transaction with binderfs binder device passed */
 	ksft_inc_pass_cnt();
 
-	ret = unlink("/dev/binderfs/my-binder");
+	ret = unlink("/tmp/binderfs/my-binder");
 	if (ret < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		keep ? : rmdir_protect_errno("/tmp/binderfs");
 		ksft_exit_fail_msg("%s - Failed to delete binder device\n",
 				   strerror(errno));
 	}
@@ -160,12 +160,12 @@ static void __do_binderfs_test(void)
 	/* binder device removal passed */
 	ksft_inc_pass_cnt();
 
-	ret = unlink("/dev/binderfs/binder-control");
+	ret = unlink("/tmp/binderfs/binder-control");
 	if (!ret) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		keep ? : rmdir_protect_errno("/tmp/binderfs");
 		ksft_exit_fail_msg("Managed to delete binder-control device\n");
 	} else if (errno != EPERM) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		keep ? : rmdir_protect_errno("/tmp/binderfs");
 		ksft_exit_fail_msg(
 			"%s - Failed to delete binder-control device but exited with unexpected error code\n",
 			strerror(errno));
@@ -175,8 +175,8 @@ static void __do_binderfs_test(void)
 	ksft_inc_xfail_cnt();
 
 on_error:
-	ret = umount2("/dev/binderfs", MNT_DETACH);
-	keep ?: rmdir_protect_errno("/dev/binderfs");
+	ret = umount2("/tmp/binderfs", MNT_DETACH);
+	keep ?: rmdir_protect_errno("/tmp/binderfs");
 	if (ret < 0)
 		ksft_exit_fail_msg("%s - Failed to unmount binderfs\n",
 				   strerror(errno));
-- 
2.25.1


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

* [PATCH] binderfs: port to new mount api
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
  2020-03-12 13:15     ` [PATCH 2/3] binderfs: add stress test for binderfs binder devices Christian Brauner
  2020-03-12 13:15     ` [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint Christian Brauner
@ 2020-03-12 21:24     ` Christian Brauner
  2020-03-12 23:56       ` Kees Cook
  2020-03-12 23:51     ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Kees Cook
                       ` (2 subsequent siblings)
  5 siblings, 1 reply; 29+ messages in thread
From: Christian Brauner @ 2020-03-12 21:24 UTC (permalink / raw)
  To: christian.brauner
  Cc: ard.biesheuvel, ardb, arve, gregkh, hridya, joel, john.stultz,
	keescook, kernel-team, linux-kernel, linux-kselftest, maco,
	naresh.kamboju, shuah, tkjos, Todd Kjos

It's time we port binderfs to the new mount api. We can make use of the
new option parser, get nicer infrastructure and it will be easiert if we
ever add any new mount options.

This survives testing with the binderfs selftests:

for i in `seq 1 1000`; do ./binderfs_test; done

including the new stress tests I sent out for review today:

 [==========] Running 3 tests from 1 test cases.
 [ RUN      ] global.binderfs_stress
 [       OK ] global.binderfs_stress
 [ RUN      ] global.binderfs_test_privileged
 # Tests are not run as root. Skipping privileged tests
 [       OK ] global.binderfs_test_privileged
 [ RUN      ] global.binderfs_test_unprivileged
 # Allocated new binder device with major 243, minor 4, and name my-binder
 # Detected binder version: 8
 [       OK ] global.binderfs_test_unprivileged
 [==========] 3 / 3 tests passed.
 [  PASSED  ]

Cc: Todd Kjos <tkjos@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 drivers/android/binderfs.c | 200 +++++++++++++++++++------------------
 1 file changed, 105 insertions(+), 95 deletions(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index f303106b3362..2c89e0b5a82d 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -18,7 +18,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/mount.h>
-#include <linux/parser.h>
+#include <linux/fs_parser.h>
 #include <linux/radix-tree.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
@@ -48,26 +48,30 @@ static dev_t binderfs_dev;
 static DEFINE_MUTEX(binderfs_minors_mutex);
 static DEFINE_IDA(binderfs_minors);
 
-enum {
+enum binderfs_param {
 	Opt_max,
 	Opt_stats_mode,
-	Opt_err
 };
 
 enum binderfs_stats_mode {
-	STATS_NONE,
-	STATS_GLOBAL,
+	binderfs_stats_mode_unset,
+	binderfs_stats_mode_global,
 };
 
-static const match_table_t tokens = {
-	{ Opt_max, "max=%d" },
-	{ Opt_stats_mode, "stats=%s" },
-	{ Opt_err, NULL     }
+static const struct constant_table binderfs_param_stats[] = {
+	{ "global", binderfs_stats_mode_global },
+	{}
 };
 
-static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
+const struct fs_parameter_spec binderfs_fs_parameters[] = {
+	fsparam_u32("max",	Opt_max),
+	fsparam_enum("stats",	Opt_stats_mode, binderfs_param_stats),
+	{}
+};
+
+static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
 {
-	return inode->i_sb->s_fs_info;
+	return sb->s_fs_info;
 }
 
 bool is_binderfs_device(const struct inode *inode)
@@ -246,7 +250,7 @@ static long binder_ctl_ioctl(struct file *file, unsigned int cmd,
 static void binderfs_evict_inode(struct inode *inode)
 {
 	struct binder_device *device = inode->i_private;
-	struct binderfs_info *info = BINDERFS_I(inode);
+	struct binderfs_info *info = BINDERFS_SB(inode->i_sb);
 
 	clear_inode(inode);
 
@@ -264,97 +268,85 @@ static void binderfs_evict_inode(struct inode *inode)
 	}
 }
 
-/**
- * binderfs_parse_mount_opts - parse binderfs mount options
- * @data: options to set (can be NULL in which case defaults are used)
- */
-static int binderfs_parse_mount_opts(char *data,
-				     struct binderfs_mount_opts *opts)
+static int binderfs_fs_context_parse_param(struct fs_context *fc,
+					   struct fs_parameter *param)
 {
-	char *p, *stats;
-	opts->max = BINDERFS_MAX_MINOR;
-	opts->stats_mode = STATS_NONE;
-
-	while ((p = strsep(&data, ",")) != NULL) {
-		substring_t args[MAX_OPT_ARGS];
-		int token;
-		int max_devices;
-
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_max:
-			if (match_int(&args[0], &max_devices) ||
-			    (max_devices < 0 ||
-			     (max_devices > BINDERFS_MAX_MINOR)))
-				return -EINVAL;
-
-			opts->max = max_devices;
-			break;
-		case Opt_stats_mode:
-			if (!capable(CAP_SYS_ADMIN))
-				return -EINVAL;
+	int opt;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
+	struct fs_parse_result result;
 
-			stats = match_strdup(&args[0]);
-			if (!stats)
-				return -ENOMEM;
+	opt = fs_parse(fc, binderfs_fs_parameters, param, &result);
+	if (opt < 0)
+		return opt;
 
-			if (strcmp(stats, "global") != 0) {
-				kfree(stats);
-				return -EINVAL;
-			}
+	switch (opt) {
+	case Opt_max:
+		if (result.uint_32 > BINDERFS_MAX_MINOR)
+			return invalfc(fc, "Bad value for '%s'", param->key);
 
-			opts->stats_mode = STATS_GLOBAL;
-			kfree(stats);
-			break;
-		default:
-			pr_err("Invalid mount options\n");
-			return -EINVAL;
-		}
+		ctx->max = result.uint_32;
+		break;
+	case Opt_stats_mode:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		ctx->stats_mode = result.uint_32;
+		break;
+	default:
+		return invalfc(fc, "Unsupported parameter '%s'", param->key);
 	}
 
 	return 0;
 }
 
-static int binderfs_remount(struct super_block *sb, int *flags, char *data)
+static int binderfs_fs_context_reconfigure(struct fs_context *fc)
 {
-	int prev_stats_mode, ret;
-	struct binderfs_info *info = sb->s_fs_info;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
+	struct binderfs_info *info = BINDERFS_SB(fc->root->d_sb);
 
-	prev_stats_mode = info->mount_opts.stats_mode;
-	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
-	if (ret)
-		return ret;
+	if (info->mount_opts.stats_mode != ctx->stats_mode)
+		return invalfc(fc, "Binderfs stats mode cannot be changed during a remount");
 
-	if (prev_stats_mode != info->mount_opts.stats_mode) {
-		pr_err("Binderfs stats mode cannot be changed during a remount\n");
-		info->mount_opts.stats_mode = prev_stats_mode;
-		return -EINVAL;
-	}
+	info->mount_opts.stats_mode = ctx->stats_mode;
+	info->mount_opts.max = ctx->max;
 
 	return 0;
 }
 
-static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
+static int binderfs_show_options(struct seq_file *seq, struct dentry *root)
 {
-	struct binderfs_info *info;
+	struct binderfs_info *info = BINDERFS_SB(root->d_sb);
 
-	info = root->d_sb->s_fs_info;
 	if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
 		seq_printf(seq, ",max=%d", info->mount_opts.max);
-	if (info->mount_opts.stats_mode == STATS_GLOBAL)
+
+	switch (info->mount_opts.stats_mode) {
+	case binderfs_stats_mode_unset:
+		break;
+	case binderfs_stats_mode_global:
 		seq_printf(seq, ",stats=global");
+		break;
+	}
 
 	return 0;
 }
 
+static void binderfs_put_super(struct super_block *sb)
+{
+	struct binderfs_info *info = sb->s_fs_info;
+
+	if (info && info->ipc_ns)
+		put_ipc_ns(info->ipc_ns);
+
+	kfree(info);
+	sb->s_fs_info = NULL;
+}
+
 static const struct super_operations binderfs_super_ops = {
 	.evict_inode    = binderfs_evict_inode,
-	.remount_fs	= binderfs_remount,
-	.show_options	= binderfs_show_mount_opts,
+	.show_options	= binderfs_show_options,
 	.statfs         = simple_statfs,
+	.put_super	= binderfs_put_super,
 };
 
 static inline bool is_binderfs_control_device(const struct dentry *dentry)
@@ -653,10 +645,11 @@ static int init_binder_logs(struct super_block *sb)
 	return ret;
 }
 
-static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
+static int binderfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	int ret;
 	struct binderfs_info *info;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
 	struct inode *inode = NULL;
 	struct binderfs_device device_info = { 0 };
 	const char *name;
@@ -689,16 +682,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	info->ipc_ns = get_ipc_ns(current->nsproxy->ipc_ns);
 
-	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
-	if (ret)
-		return ret;
-
 	info->root_gid = make_kgid(sb->s_user_ns, 0);
 	if (!gid_valid(info->root_gid))
 		info->root_gid = GLOBAL_ROOT_GID;
 	info->root_uid = make_kuid(sb->s_user_ns, 0);
 	if (!uid_valid(info->root_uid))
 		info->root_uid = GLOBAL_ROOT_UID;
+	info->mount_opts.max = ctx->max;
+	info->mount_opts.stats_mode = ctx->stats_mode;
 
 	inode = new_inode(sb);
 	if (!inode)
@@ -730,36 +721,55 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 			name++;
 	}
 
-	if (info->mount_opts.stats_mode == STATS_GLOBAL)
+	if (info->mount_opts.stats_mode == binderfs_stats_mode_global)
 		return init_binder_logs(sb);
 
 	return 0;
 }
 
-static struct dentry *binderfs_mount(struct file_system_type *fs_type,
-				     int flags, const char *dev_name,
-				     void *data)
+static int binderfs_fs_context_get_tree(struct fs_context *fc)
 {
-	return mount_nodev(fs_type, flags, data, binderfs_fill_super);
+	return get_tree_nodev(fc, binderfs_fill_super);
 }
 
-static void binderfs_kill_super(struct super_block *sb)
+static void binderfs_fs_context_free(struct fs_context *fc)
 {
-	struct binderfs_info *info = sb->s_fs_info;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
+
+	fc->fs_private = NULL;
+	kfree(ctx);
+}
 
-	kill_litter_super(sb);
+static const struct fs_context_operations binderfs_fs_context_ops = {
+	.free		= binderfs_fs_context_free,
+	.get_tree	= binderfs_fs_context_get_tree,
+	.parse_param	= binderfs_fs_context_parse_param,
+	.reconfigure	= binderfs_fs_context_reconfigure,
+};
 
-	if (info && info->ipc_ns)
-		put_ipc_ns(info->ipc_ns);
+static int binderfs_init_fs_context(struct fs_context *fc)
+{
+	struct binderfs_mount_opts *ctx;
 
-	kfree(info);
+	ctx = kzalloc(sizeof(struct binderfs_mount_opts), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->max = BINDERFS_MAX_MINOR;
+	ctx->stats_mode = binderfs_stats_mode_unset;
+
+	fc->fs_private = ctx;
+	fc->ops = &binderfs_fs_context_ops;
+
+	return 0;
 }
 
 static struct file_system_type binder_fs_type = {
-	.name		= "binder",
-	.mount		= binderfs_mount,
-	.kill_sb	= binderfs_kill_super,
-	.fs_flags	= FS_USERNS_MOUNT,
+	.name			= "binder",
+	.init_fs_context	= binderfs_init_fs_context,
+	.parameters		= binderfs_fs_parameters,
+	.kill_sb		= kill_litter_super,
+	.fs_flags		= FS_USERNS_MOUNT,
 };
 
 int __init init_binderfs(void)

base-commit: f17f06a0c7794d3a7c2425663738823354447472
-- 
2.25.1


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

* Re: [PATCH 1/3] binderfs: port tests to test harness infrastructure
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
                       ` (2 preceding siblings ...)
  2020-03-12 21:24     ` [PATCH] binderfs: port to new mount api Christian Brauner
@ 2020-03-12 23:51     ` Kees Cook
  2020-03-13 15:24     ` [PATCH v2 " Christian Brauner
  2020-03-13 15:34     ` [PATCH v2] binderfs: port to new mount api Christian Brauner
  5 siblings, 0 replies; 29+ messages in thread
From: Kees Cook @ 2020-03-12 23:51 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Greg Kroah-Hartman, Todd Kjos, ard.biesheuvel, ardb, john.stultz,
	linux-kernel, linux-kselftest, naresh.kamboju, shuah,
	Arve Hjønnevåg, Martijn Coenen, Joel Fernandes, hridya,
	kernel-team

On Thu, Mar 12, 2020 at 02:15:29PM +0100, Christian Brauner wrote:
> Makes for nicer output and prepares for additional tests.
> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Yay harness! :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  .../selftests/filesystems/binderfs/Makefile      |  2 ++
>  .../filesystems/binderfs/binderfs_test.c         | 16 ++++++----------
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> index 58cb659b56b4..75315d9ba7a9 100644
> --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> @@ -3,4 +3,6 @@
>  CFLAGS += -I../../../../../usr/include/
>  TEST_GEN_PROGS := binderfs_test
>  
> +binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> +
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 8c2ed962e1c7..d03ed8eed5eb 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -15,7 +15,9 @@
>  #include <unistd.h>
>  #include <linux/android/binder.h>
>  #include <linux/android/binderfs.h>
> +
>  #include "../../kselftest.h"
> +#include "../../kselftest_harness.h"
>  
>  static ssize_t write_nointr(int fd, const void *buf, size_t count)
>  {
> @@ -252,24 +254,18 @@ static void __do_binderfs_test(void)
>  	ksft_inc_pass_cnt();
>  }
>  
> -static void binderfs_test_privileged()
> +TEST(binderfs_test_privileged)
>  {
>  	if (geteuid() != 0)
> -		ksft_print_msg(
> -			"Tests are not run as root. Skipping privileged tests\n");
> +		ksft_print_msg("Tests are not run as root. Skipping privileged tests\n");
>  	else
>  		__do_binderfs_test();
>  }
>  
> -static void binderfs_test_unprivileged()
> +TEST(binderfs_test_unprivileged)
>  {
>  	change_to_userns();
>  	__do_binderfs_test();
>  }
>  
> -int main(int argc, char *argv[])
> -{
> -	binderfs_test_privileged();
> -	binderfs_test_unprivileged();
> -	ksft_exit_pass();
> -}
> +TEST_HARNESS_MAIN
> 
> base-commit: 2c523b344dfa65a3738e7039832044aa133c75fb
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH 2/3] binderfs: add stress test for binderfs binder devices
  2020-03-12 13:15     ` [PATCH 2/3] binderfs: add stress test for binderfs binder devices Christian Brauner
@ 2020-03-12 23:53       ` Kees Cook
  2020-03-13 12:54         ` Christian Brauner
  0 siblings, 1 reply; 29+ messages in thread
From: Kees Cook @ 2020-03-12 23:53 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Greg Kroah-Hartman, Todd Kjos, ard.biesheuvel, ardb, john.stultz,
	linux-kernel, linux-kselftest, naresh.kamboju, shuah,
	Arve Hjønnevåg, Martijn Coenen, Joel Fernandes, hridya,
	kernel-team

On Thu, Mar 12, 2020 at 02:15:30PM +0100, Christian Brauner wrote:
> This adds a stress test that should hopefully help us catch regressions
> for [1], [2], and [3].
> 
> [1]: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> [2]: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> [3]: 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Do you care about 80-char line limits? (Or does the selftest tree? There
are a few in here...)

Otherwise, seems reasonable:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  .../selftests/filesystems/binderfs/Makefile   |   2 +-
>  .../filesystems/binderfs/binderfs_test.c      | 416 ++++++++++++++----
>  2 files changed, 325 insertions(+), 93 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> index 75315d9ba7a9..8af25ae96049 100644
> --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -CFLAGS += -I../../../../../usr/include/
> +CFLAGS += -I../../../../../usr/include/ -pthread
>  TEST_GEN_PROGS := binderfs_test
>  
>  binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index d03ed8eed5eb..313ffad15614 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -3,15 +3,20 @@
>  #define _GNU_SOURCE
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <pthread.h>
>  #include <sched.h>
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <sys/fsuid.h>
>  #include <sys/ioctl.h>
>  #include <sys/mount.h>
> +#include <sys/socket.h>
>  #include <sys/stat.h>
> +#include <sys/sysinfo.h>
>  #include <sys/types.h>
> +#include <sys/wait.h>
>  #include <unistd.h>
>  #include <linux/android/binder.h>
>  #include <linux/android/binderfs.h>
> @@ -19,100 +24,26 @@
>  #include "../../kselftest.h"
>  #include "../../kselftest_harness.h"
>  
> -static ssize_t write_nointr(int fd, const void *buf, size_t count)
> -{
> -	ssize_t ret;
> -again:
> -	ret = write(fd, buf, count);
> -	if (ret < 0 && errno == EINTR)
> -		goto again;
> +#define DEFAULT_THREADS 4
>  
> -	return ret;
> -}
> +#define PTR_TO_INT(p) ((int)((intptr_t)(p)))
> +#define INT_TO_PTR(u) ((void *)((intptr_t)(u)))
>  
> -static void write_to_file(const char *filename, const void *buf, size_t count,
> -			  int allowed_errno)
> -{
> -	int fd, saved_errno;
> -	ssize_t ret;
> -
> -	fd = open(filename, O_WRONLY | O_CLOEXEC);
> -	if (fd < 0)
> -		ksft_exit_fail_msg("%s - Failed to open file %s\n",
> -				   strerror(errno), filename);
> -
> -	ret = write_nointr(fd, buf, count);
> -	if (ret < 0) {
> -		if (allowed_errno && (errno == allowed_errno)) {
> -			close(fd);
> -			return;
> -		}
> -
> -		goto on_error;
> +#define close_prot_errno_disarm(fd) \
> +	if (fd >= 0) {              \
> +		int _e_ = errno;    \
> +		close(fd);          \
> +		errno = _e_;        \
> +		fd = -EBADF;        \
>  	}
>  
> -	if ((size_t)ret != count)
> -		goto on_error;
> -
> -	close(fd);
> -	return;
> -
> -on_error:
> -	saved_errno = errno;
> -	close(fd);
> -	errno = saved_errno;
> -
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to write to file %s\n",
> -				   strerror(errno), filename);
> +#define log_exit(format, ...)                                                  \
> +	({                                                                     \
> +		fprintf(stderr, format "\n", ##__VA_ARGS__);                   \
> +		exit(EXIT_FAILURE);                                            \
> +	})
>  
> -	ksft_exit_fail_msg("Failed to write to file %s\n", filename);
> -}
> -
> -static void change_to_userns(void)
> -{
> -	int ret;
> -	uid_t uid;
> -	gid_t gid;
> -	/* {g,u}id_map files only allow a max of 4096 bytes written to them */
> -	char idmap[4096];
> -
> -	uid = getuid();
> -	gid = getgid();
> -
> -	ret = unshare(CLONE_NEWUSER);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
> -				   strerror(errno));
> -
> -	write_to_file("/proc/self/setgroups", "deny", strlen("deny"), ENOENT);
> -
> -	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", uid);
> -	if (ret < 0 || (size_t)ret >= sizeof(idmap))
> -		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
> -				   strerror(errno));
> -
> -	write_to_file("/proc/self/uid_map", idmap, strlen(idmap), 0);
> -
> -	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", gid);
> -	if (ret < 0 || (size_t)ret >= sizeof(idmap))
> -		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
> -				   strerror(errno));
> -
> -	write_to_file("/proc/self/gid_map", idmap, strlen(idmap), 0);
> -
> -	ret = setgid(0);
> -	if (ret)
> -		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
> -				   strerror(errno));
> -
> -	ret = setuid(0);
> -	if (ret)
> -		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
> -				   strerror(errno));
> -}
> -
> -static void change_to_mountns(void)
> +static void change_mountns(void)
>  {
>  	int ret;
>  
> @@ -143,7 +74,7 @@ static void __do_binderfs_test(void)
>  	struct binderfs_device device = { 0 };
>  	struct binder_version version = { 0 };
>  
> -	change_to_mountns();
> +	change_mountns();
>  
>  	ret = mkdir("/dev/binderfs", 0755);
>  	if (ret < 0) {
> @@ -254,6 +185,283 @@ static void __do_binderfs_test(void)
>  	ksft_inc_pass_cnt();
>  }
>  
> +static int wait_for_pid(pid_t pid)
> +{
> +	int status, ret;
> +
> +again:
> +	ret = waitpid(pid, &status, 0);
> +	if (ret == -1) {
> +		if (errno == EINTR)
> +			goto again;
> +
> +		return -1;
> +	}
> +
> +	if (!WIFEXITED(status))
> +		return -1;
> +
> +	return WEXITSTATUS(status);
> +}
> +
> +static int setid_userns_root(void)
> +{
> +	if (setuid(0))
> +		return -1;
> +	if (setgid(0))
> +		return -1;
> +
> +	setfsuid(0);
> +	setfsgid(0);
> +
> +	return 0;
> +}
> +
> +enum idmap_type {
> +	UID_MAP,
> +	GID_MAP,
> +};
> +
> +static ssize_t read_nointr(int fd, void *buf, size_t count)
> +{
> +	ssize_t ret;
> +again:
> +	ret = read(fd, buf, count);
> +	if (ret < 0 && errno == EINTR)
> +		goto again;
> +
> +	return ret;
> +}
> +
> +static ssize_t write_nointr(int fd, const void *buf, size_t count)
> +{
> +	ssize_t ret;
> +again:
> +	ret = write(fd, buf, count);
> +	if (ret < 0 && errno == EINTR)
> +		goto again;
> +
> +	return ret;
> +}
> +
> +static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
> +			    size_t buf_size)
> +{
> +	int fd;
> +	int ret;
> +	char path[4096];
> +
> +	if (type == GID_MAP) {
> +		int setgroups_fd;
> +
> +		snprintf(path, sizeof(path), "/proc/%d/setgroups", pid);
> +		setgroups_fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
> +		if (setgroups_fd < 0 && errno != ENOENT)
> +			return -1;
> +
> +		if (setgroups_fd >= 0) {
> +			ret = write_nointr(setgroups_fd, "deny", sizeof("deny") - 1);
> +			close_prot_errno_disarm(setgroups_fd);
> +			if (ret != sizeof("deny") - 1)
> +				return -1;
> +		}
> +	}
> +
> +	switch (type) {
> +	case UID_MAP:
> +		ret = snprintf(path, sizeof(path), "/proc/%d/uid_map", pid);
> +		break;
> +	case GID_MAP:
> +		ret = snprintf(path, sizeof(path), "/proc/%d/gid_map", pid);
> +		break;
> +	default:
> +		return -1;
> +	}
> +	if (ret < 0 || ret >= sizeof(path))
> +		return -E2BIG;
> +
> +	fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
> +	if (fd < 0)
> +		return -1;
> +
> +	ret = write_nointr(fd, buf, buf_size);
> +	close_prot_errno_disarm(fd);
> +	if (ret != buf_size)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static void change_userns(int syncfds[2])
> +{
> +	int ret;
> +	char buf;
> +
> +	close_prot_errno_disarm(syncfds[1]);
> +
> +	ret = unshare(CLONE_NEWUSER);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
> +				   strerror(errno));
> +
> +	ret = write_nointr(syncfds[0], "1", 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("write_nointr() failed\n");
> +
> +	ret = read_nointr(syncfds[0], &buf, 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("read_nointr() failed\n");
> +
> +	close_prot_errno_disarm(syncfds[0]);
> +
> +	if (setid_userns_root())
> +		ksft_exit_fail_msg("setid_userns_root() failed");
> +}
> +
> +static void change_idmaps(int syncfds[2], pid_t pid)
> +{
> +	int ret;
> +	char buf;
> +	char id_map[4096];
> +
> +	close_prot_errno_disarm(syncfds[0]);
> +
> +	ret = read_nointr(syncfds[1], &buf, 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("read_nointr() failed\n");
> +
> +	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
> +	ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
> +	if (ret)
> +		ksft_exit_fail_msg("write_id_mapping(UID_MAP) failed");
> +
> +	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
> +	ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
> +	if (ret)
> +		ksft_exit_fail_msg("write_id_mapping(GID_MAP) failed");
> +
> +	ret = write_nointr(syncfds[1], "1", 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("write_nointr() failed");
> +
> +	close_prot_errno_disarm(syncfds[1]);
> +}
> +
> +static void *binder_version_thread(void *data)
> +{
> +	int fd = PTR_TO_INT(data);
> +	struct binder_version version = { 0 };
> +	int ret;
> +
> +	ret = ioctl(fd, BINDER_VERSION, &version);
> +	if (ret < 0)
> +		ksft_print_msg("%s - Failed to open perform BINDER_VERSION request\n", strerror(errno));
> +
> +	pthread_exit(data);
> +}
> +
> +/*
> + * Regression test:
> + * 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> + * f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> + * 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> + */
> +TEST(binderfs_stress)
> +{
> +	int fds[1000];
> +	int syncfds[2];
> +	pid_t pid;
> +	int fd, ret;
> +	size_t len;
> +	struct binderfs_device device = { 0 };
> +
> +	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		close_prot_errno_disarm(syncfds[0]);
> +		close_prot_errno_disarm(syncfds[1]);
> +		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
> +	}
> +
> +	if (pid == 0) {
> +		int i, j, k, nthreads;
> +		pthread_attr_t attr;
> +		pthread_t threads[DEFAULT_THREADS];
> +		change_userns(syncfds);
> +		change_mountns();
> +
> +		ret = mkdir("/tmp/binderfs", 0755);
> +		if (ret < 0 && errno != EEXIST)
> +			log_exit("%s - Failed to create binderfs mountpoint\n", strerror(errno));
> +
> +		ret = mount(NULL, "/tmp/binderfs", "binder", 0, 0);
> +		if (ret < 0)
> +			log_exit("%s - Failed to mount binderfs\n", strerror(errno));
> +
> +		for (int i = 0; i < ARRAY_SIZE(fds); i++) {
> +			char path[4096];
> +
> +			fd = open("/tmp/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
> +			if (fd < 0)
> +				log_exit("%s - Failed to open binder-control device\n", strerror(errno));
> +
> +			memset(&device, 0, sizeof(device));
> +			snprintf(device.name, sizeof(device.name), "%d", i);
> +			ret = ioctl(fd, BINDER_CTL_ADD, &device);
> +			close_prot_errno_disarm(fd);
> +			if (ret < 0)
> +				log_exit("%s - Failed to allocate new binder device\n", strerror(errno));
> +
> +			snprintf(path, sizeof(path), "/tmp/binderfs/%d", i);
> +			fds[i] = open(path, O_RDONLY | O_CLOEXEC);
> +			if (fds[i] < 0)
> +				log_exit("%s - Failed to open binder device\n", strerror(errno));
> +		}
> +
> +		ret = umount2("/tmp/binderfs", MNT_DETACH);
> +		if (ret < 0)
> +			log_exit("%s - Failed to unmount binderfs\n", strerror(errno));
> +
> +		nthreads = get_nprocs_conf();
> +		if (nthreads > DEFAULT_THREADS)
> +			nthreads = DEFAULT_THREADS;
> +
> +		pthread_attr_init(&attr);
> +		for (k = 0; k < ARRAY_SIZE(fds); k++) {
> +			for (i = 0; i < nthreads; i++) {
> +				ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
> +				if (ret) {
> +					ksft_print_msg("%s - Failed to create thread %d\n", strerror(errno), i);
> +					break;
> +				}
> +			}
> +
> +			for (j = 0; j < i; j++) {
> +				void *fdptr = NULL;
> +
> +				ret = pthread_join(threads[j], &fdptr);
> +				if (ret)
> +					ksft_print_msg("%s - Failed to join thread %d for fd %d\n", strerror(errno), j, PTR_TO_INT(fdptr));
> +			}
> +		}
> +		pthread_attr_destroy(&attr);
> +
> +		for (k = 0; k < ARRAY_SIZE(fds); k++)
> +			close(fds[k]);
> +
> +		exit(EXIT_SUCCESS);
> +	}
> +
> +	change_idmaps(syncfds, pid);
> +
> +	ret = wait_for_pid(pid);
> +	if (ret)
> +		ksft_exit_fail_msg("wait_for_pid() failed");
> +}
> +
>  TEST(binderfs_test_privileged)
>  {
>  	if (geteuid() != 0)
> @@ -264,8 +472,32 @@ TEST(binderfs_test_privileged)
>  
>  TEST(binderfs_test_unprivileged)
>  {
> -	change_to_userns();
> -	__do_binderfs_test();
> +	int ret;
> +	int syncfds[2];
> +	pid_t pid;
> +
> +	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		close_prot_errno_disarm(syncfds[0]);
> +		close_prot_errno_disarm(syncfds[1]);
> +		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
> +	}
> +
> +	if (pid == 0) {
> +		change_userns(syncfds);
> +		__do_binderfs_test();
> +		exit(EXIT_SUCCESS);
> +	}
> +
> +	change_idmaps(syncfds, pid);
> +
> +	ret = wait_for_pid(pid);
> +	if (ret)
> +		ksft_exit_fail_msg("wait_for_pid() failed");
>  }
>  
>  TEST_HARNESS_MAIN
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint
  2020-03-12 13:15     ` [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint Christian Brauner
@ 2020-03-12 23:54       ` Kees Cook
  2020-03-13 12:55         ` Christian Brauner
  0 siblings, 1 reply; 29+ messages in thread
From: Kees Cook @ 2020-03-12 23:54 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Greg Kroah-Hartman, Todd Kjos, ard.biesheuvel, ardb, john.stultz,
	linux-kernel, linux-kselftest, naresh.kamboju, shuah,
	Arve Hjønnevåg, Martijn Coenen, Joel Fernandes, hridya,
	kernel-team

On Thu, Mar 12, 2020 at 02:15:31PM +0100, Christian Brauner wrote:
> Unprivileged users will be able to create directories in there. The
> unprivileged test for /dev wouldn't have worked on most systems.
> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Seems good. (Though would a dynamic location be better?
mkstemp()-style?)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  .../filesystems/binderfs/binderfs_test.c      | 30 +++++++++----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 313ffad15614..d6e61998d20a 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -76,7 +76,7 @@ static void __do_binderfs_test(void)
>  
>  	change_mountns();
>  
> -	ret = mkdir("/dev/binderfs", 0755);
> +	ret = mkdir("/tmp/binderfs", 0755);
>  	if (ret < 0) {
>  		if (errno != EEXIST)
>  			ksft_exit_fail_msg(
> @@ -86,13 +86,13 @@ static void __do_binderfs_test(void)
>  		keep = true;
>  	}
>  
> -	ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
> +	ret = mount(NULL, "/tmp/binderfs", "binder", 0, 0);
>  	if (ret < 0) {
>  		if (errno != ENODEV)
>  			ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
>  					   strerror(errno));
>  
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		keep ? : rmdir_protect_errno("/tmp/binderfs");
>  		ksft_exit_skip(
>  			"The Android binderfs filesystem is not available\n");
>  	}
> @@ -102,7 +102,7 @@ static void __do_binderfs_test(void)
>  
>  	memcpy(device.name, "my-binder", strlen("my-binder"));
>  
> -	fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
> +	fd = open("/tmp/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
>  	if (fd < 0)
>  		ksft_exit_fail_msg(
>  			"%s - Failed to open binder-control device\n",
> @@ -113,7 +113,7 @@ static void __do_binderfs_test(void)
>  	close(fd);
>  	errno = saved_errno;
>  	if (ret < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		keep ? : rmdir_protect_errno("/tmp/binderfs");
>  		ksft_exit_fail_msg(
>  			"%s - Failed to allocate new binder device\n",
>  			strerror(errno));
> @@ -126,9 +126,9 @@ static void __do_binderfs_test(void)
>  	/* binder device allocation test passed */
>  	ksft_inc_pass_cnt();
>  
> -	fd = open("/dev/binderfs/my-binder", O_CLOEXEC | O_RDONLY);
> +	fd = open("/tmp/binderfs/my-binder", O_CLOEXEC | O_RDONLY);
>  	if (fd < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		keep ? : rmdir_protect_errno("/tmp/binderfs");
>  		ksft_exit_fail_msg("%s - Failed to open my-binder device\n",
>  				   strerror(errno));
>  	}
> @@ -138,7 +138,7 @@ static void __do_binderfs_test(void)
>  	close(fd);
>  	errno = saved_errno;
>  	if (ret < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		keep ? : rmdir_protect_errno("/tmp/binderfs");
>  		ksft_exit_fail_msg(
>  			"%s - Failed to open perform BINDER_VERSION request\n",
>  			strerror(errno));
> @@ -150,9 +150,9 @@ static void __do_binderfs_test(void)
>  	/* binder transaction with binderfs binder device passed */
>  	ksft_inc_pass_cnt();
>  
> -	ret = unlink("/dev/binderfs/my-binder");
> +	ret = unlink("/tmp/binderfs/my-binder");
>  	if (ret < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		keep ? : rmdir_protect_errno("/tmp/binderfs");
>  		ksft_exit_fail_msg("%s - Failed to delete binder device\n",
>  				   strerror(errno));
>  	}
> @@ -160,12 +160,12 @@ static void __do_binderfs_test(void)
>  	/* binder device removal passed */
>  	ksft_inc_pass_cnt();
>  
> -	ret = unlink("/dev/binderfs/binder-control");
> +	ret = unlink("/tmp/binderfs/binder-control");
>  	if (!ret) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		keep ? : rmdir_protect_errno("/tmp/binderfs");
>  		ksft_exit_fail_msg("Managed to delete binder-control device\n");
>  	} else if (errno != EPERM) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		keep ? : rmdir_protect_errno("/tmp/binderfs");
>  		ksft_exit_fail_msg(
>  			"%s - Failed to delete binder-control device but exited with unexpected error code\n",
>  			strerror(errno));
> @@ -175,8 +175,8 @@ static void __do_binderfs_test(void)
>  	ksft_inc_xfail_cnt();
>  
>  on_error:
> -	ret = umount2("/dev/binderfs", MNT_DETACH);
> -	keep ?: rmdir_protect_errno("/dev/binderfs");
> +	ret = umount2("/tmp/binderfs", MNT_DETACH);
> +	keep ?: rmdir_protect_errno("/tmp/binderfs");
>  	if (ret < 0)
>  		ksft_exit_fail_msg("%s - Failed to unmount binderfs\n",
>  				   strerror(errno));
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH] binderfs: port to new mount api
  2020-03-12 21:24     ` [PATCH] binderfs: port to new mount api Christian Brauner
@ 2020-03-12 23:56       ` Kees Cook
  2020-03-13 12:55         ` Christian Brauner
  0 siblings, 1 reply; 29+ messages in thread
From: Kees Cook @ 2020-03-12 23:56 UTC (permalink / raw)
  To: Christian Brauner
  Cc: ard.biesheuvel, ardb, arve, gregkh, hridya, joel, john.stultz,
	kernel-team, linux-kernel, linux-kselftest, maco, naresh.kamboju,
	shuah, tkjos, Todd Kjos

On Thu, Mar 12, 2020 at 10:24:20PM +0100, Christian Brauner wrote:
> It's time we port binderfs to the new mount api. We can make use of the
> new option parser, get nicer infrastructure and it will be easiert if we
> ever add any new mount options.
> 
> This survives testing with the binderfs selftests:
> 
> for i in `seq 1 1000`; do ./binderfs_test; done
> 
> including the new stress tests I sent out for review today:
> 
>  [==========] Running 3 tests from 1 test cases.
>  [ RUN      ] global.binderfs_stress
>  [       OK ] global.binderfs_stress
>  [ RUN      ] global.binderfs_test_privileged
>  # Tests are not run as root. Skipping privileged tests
>  [       OK ] global.binderfs_test_privileged

I would use the XFAIL harness infrastructure for these kinds of skips.

-Kees

>  [ RUN      ] global.binderfs_test_unprivileged
>  # Allocated new binder device with major 243, minor 4, and name my-binder
>  # Detected binder version: 8
>  [       OK ] global.binderfs_test_unprivileged
>  [==========] 3 / 3 tests passed.
>  [  PASSED  ]
> 
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  drivers/android/binderfs.c | 200 +++++++++++++++++++------------------
>  1 file changed, 105 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index f303106b3362..2c89e0b5a82d 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -18,7 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mount.h>
> -#include <linux/parser.h>
> +#include <linux/fs_parser.h>
>  #include <linux/radix-tree.h>
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
> @@ -48,26 +48,30 @@ static dev_t binderfs_dev;
>  static DEFINE_MUTEX(binderfs_minors_mutex);
>  static DEFINE_IDA(binderfs_minors);
>  
> -enum {
> +enum binderfs_param {
>  	Opt_max,
>  	Opt_stats_mode,
> -	Opt_err
>  };
>  
>  enum binderfs_stats_mode {
> -	STATS_NONE,
> -	STATS_GLOBAL,
> +	binderfs_stats_mode_unset,
> +	binderfs_stats_mode_global,
>  };
>  
> -static const match_table_t tokens = {
> -	{ Opt_max, "max=%d" },
> -	{ Opt_stats_mode, "stats=%s" },
> -	{ Opt_err, NULL     }
> +static const struct constant_table binderfs_param_stats[] = {
> +	{ "global", binderfs_stats_mode_global },
> +	{}
>  };
>  
> -static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> +const struct fs_parameter_spec binderfs_fs_parameters[] = {
> +	fsparam_u32("max",	Opt_max),
> +	fsparam_enum("stats",	Opt_stats_mode, binderfs_param_stats),
> +	{}
> +};
> +
> +static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
>  {
> -	return inode->i_sb->s_fs_info;
> +	return sb->s_fs_info;
>  }
>  
>  bool is_binderfs_device(const struct inode *inode)
> @@ -246,7 +250,7 @@ static long binder_ctl_ioctl(struct file *file, unsigned int cmd,
>  static void binderfs_evict_inode(struct inode *inode)
>  {
>  	struct binder_device *device = inode->i_private;
> -	struct binderfs_info *info = BINDERFS_I(inode);
> +	struct binderfs_info *info = BINDERFS_SB(inode->i_sb);
>  
>  	clear_inode(inode);
>  
> @@ -264,97 +268,85 @@ static void binderfs_evict_inode(struct inode *inode)
>  	}
>  }
>  
> -/**
> - * binderfs_parse_mount_opts - parse binderfs mount options
> - * @data: options to set (can be NULL in which case defaults are used)
> - */
> -static int binderfs_parse_mount_opts(char *data,
> -				     struct binderfs_mount_opts *opts)
> +static int binderfs_fs_context_parse_param(struct fs_context *fc,
> +					   struct fs_parameter *param)
>  {
> -	char *p, *stats;
> -	opts->max = BINDERFS_MAX_MINOR;
> -	opts->stats_mode = STATS_NONE;
> -
> -	while ((p = strsep(&data, ",")) != NULL) {
> -		substring_t args[MAX_OPT_ARGS];
> -		int token;
> -		int max_devices;
> -
> -		if (!*p)
> -			continue;
> -
> -		token = match_token(p, tokens, args);
> -		switch (token) {
> -		case Opt_max:
> -			if (match_int(&args[0], &max_devices) ||
> -			    (max_devices < 0 ||
> -			     (max_devices > BINDERFS_MAX_MINOR)))
> -				return -EINVAL;
> -
> -			opts->max = max_devices;
> -			break;
> -		case Opt_stats_mode:
> -			if (!capable(CAP_SYS_ADMIN))
> -				return -EINVAL;
> +	int opt;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
> +	struct fs_parse_result result;
>  
> -			stats = match_strdup(&args[0]);
> -			if (!stats)
> -				return -ENOMEM;
> +	opt = fs_parse(fc, binderfs_fs_parameters, param, &result);
> +	if (opt < 0)
> +		return opt;
>  
> -			if (strcmp(stats, "global") != 0) {
> -				kfree(stats);
> -				return -EINVAL;
> -			}
> +	switch (opt) {
> +	case Opt_max:
> +		if (result.uint_32 > BINDERFS_MAX_MINOR)
> +			return invalfc(fc, "Bad value for '%s'", param->key);
>  
> -			opts->stats_mode = STATS_GLOBAL;
> -			kfree(stats);
> -			break;
> -		default:
> -			pr_err("Invalid mount options\n");
> -			return -EINVAL;
> -		}
> +		ctx->max = result.uint_32;
> +		break;
> +	case Opt_stats_mode:
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
> +
> +		ctx->stats_mode = result.uint_32;
> +		break;
> +	default:
> +		return invalfc(fc, "Unsupported parameter '%s'", param->key);
>  	}
>  
>  	return 0;
>  }
>  
> -static int binderfs_remount(struct super_block *sb, int *flags, char *data)
> +static int binderfs_fs_context_reconfigure(struct fs_context *fc)
>  {
> -	int prev_stats_mode, ret;
> -	struct binderfs_info *info = sb->s_fs_info;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
> +	struct binderfs_info *info = BINDERFS_SB(fc->root->d_sb);
>  
> -	prev_stats_mode = info->mount_opts.stats_mode;
> -	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> -	if (ret)
> -		return ret;
> +	if (info->mount_opts.stats_mode != ctx->stats_mode)
> +		return invalfc(fc, "Binderfs stats mode cannot be changed during a remount");
>  
> -	if (prev_stats_mode != info->mount_opts.stats_mode) {
> -		pr_err("Binderfs stats mode cannot be changed during a remount\n");
> -		info->mount_opts.stats_mode = prev_stats_mode;
> -		return -EINVAL;
> -	}
> +	info->mount_opts.stats_mode = ctx->stats_mode;
> +	info->mount_opts.max = ctx->max;
>  
>  	return 0;
>  }
>  
> -static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
> +static int binderfs_show_options(struct seq_file *seq, struct dentry *root)
>  {
> -	struct binderfs_info *info;
> +	struct binderfs_info *info = BINDERFS_SB(root->d_sb);
>  
> -	info = root->d_sb->s_fs_info;
>  	if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
>  		seq_printf(seq, ",max=%d", info->mount_opts.max);
> -	if (info->mount_opts.stats_mode == STATS_GLOBAL)
> +
> +	switch (info->mount_opts.stats_mode) {
> +	case binderfs_stats_mode_unset:
> +		break;
> +	case binderfs_stats_mode_global:
>  		seq_printf(seq, ",stats=global");
> +		break;
> +	}
>  
>  	return 0;
>  }
>  
> +static void binderfs_put_super(struct super_block *sb)
> +{
> +	struct binderfs_info *info = sb->s_fs_info;
> +
> +	if (info && info->ipc_ns)
> +		put_ipc_ns(info->ipc_ns);
> +
> +	kfree(info);
> +	sb->s_fs_info = NULL;
> +}
> +
>  static const struct super_operations binderfs_super_ops = {
>  	.evict_inode    = binderfs_evict_inode,
> -	.remount_fs	= binderfs_remount,
> -	.show_options	= binderfs_show_mount_opts,
> +	.show_options	= binderfs_show_options,
>  	.statfs         = simple_statfs,
> +	.put_super	= binderfs_put_super,
>  };
>  
>  static inline bool is_binderfs_control_device(const struct dentry *dentry)
> @@ -653,10 +645,11 @@ static int init_binder_logs(struct super_block *sb)
>  	return ret;
>  }
>  
> -static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> +static int binderfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	int ret;
>  	struct binderfs_info *info;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
>  	struct inode *inode = NULL;
>  	struct binderfs_device device_info = { 0 };
>  	const char *name;
> @@ -689,16 +682,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  
>  	info->ipc_ns = get_ipc_ns(current->nsproxy->ipc_ns);
>  
> -	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> -	if (ret)
> -		return ret;
> -
>  	info->root_gid = make_kgid(sb->s_user_ns, 0);
>  	if (!gid_valid(info->root_gid))
>  		info->root_gid = GLOBAL_ROOT_GID;
>  	info->root_uid = make_kuid(sb->s_user_ns, 0);
>  	if (!uid_valid(info->root_uid))
>  		info->root_uid = GLOBAL_ROOT_UID;
> +	info->mount_opts.max = ctx->max;
> +	info->mount_opts.stats_mode = ctx->stats_mode;
>  
>  	inode = new_inode(sb);
>  	if (!inode)
> @@ -730,36 +721,55 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  			name++;
>  	}
>  
> -	if (info->mount_opts.stats_mode == STATS_GLOBAL)
> +	if (info->mount_opts.stats_mode == binderfs_stats_mode_global)
>  		return init_binder_logs(sb);
>  
>  	return 0;
>  }
>  
> -static struct dentry *binderfs_mount(struct file_system_type *fs_type,
> -				     int flags, const char *dev_name,
> -				     void *data)
> +static int binderfs_fs_context_get_tree(struct fs_context *fc)
>  {
> -	return mount_nodev(fs_type, flags, data, binderfs_fill_super);
> +	return get_tree_nodev(fc, binderfs_fill_super);
>  }
>  
> -static void binderfs_kill_super(struct super_block *sb)
> +static void binderfs_fs_context_free(struct fs_context *fc)
>  {
> -	struct binderfs_info *info = sb->s_fs_info;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
> +
> +	fc->fs_private = NULL;
> +	kfree(ctx);
> +}
>  
> -	kill_litter_super(sb);
> +static const struct fs_context_operations binderfs_fs_context_ops = {
> +	.free		= binderfs_fs_context_free,
> +	.get_tree	= binderfs_fs_context_get_tree,
> +	.parse_param	= binderfs_fs_context_parse_param,
> +	.reconfigure	= binderfs_fs_context_reconfigure,
> +};
>  
> -	if (info && info->ipc_ns)
> -		put_ipc_ns(info->ipc_ns);
> +static int binderfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct binderfs_mount_opts *ctx;
>  
> -	kfree(info);
> +	ctx = kzalloc(sizeof(struct binderfs_mount_opts), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->max = BINDERFS_MAX_MINOR;
> +	ctx->stats_mode = binderfs_stats_mode_unset;
> +
> +	fc->fs_private = ctx;
> +	fc->ops = &binderfs_fs_context_ops;
> +
> +	return 0;
>  }
>  
>  static struct file_system_type binder_fs_type = {
> -	.name		= "binder",
> -	.mount		= binderfs_mount,
> -	.kill_sb	= binderfs_kill_super,
> -	.fs_flags	= FS_USERNS_MOUNT,
> +	.name			= "binder",
> +	.init_fs_context	= binderfs_init_fs_context,
> +	.parameters		= binderfs_fs_parameters,
> +	.kill_sb		= kill_litter_super,
> +	.fs_flags		= FS_USERNS_MOUNT,
>  };
>  
>  int __init init_binderfs(void)
> 
> base-commit: f17f06a0c7794d3a7c2425663738823354447472
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH 2/3] binderfs: add stress test for binderfs binder devices
  2020-03-12 23:53       ` Kees Cook
@ 2020-03-13 12:54         ` Christian Brauner
  0 siblings, 0 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 12:54 UTC (permalink / raw)
  To: Kees Cook
  Cc: Greg Kroah-Hartman, Todd Kjos, ard.biesheuvel, ardb, john.stultz,
	linux-kernel, linux-kselftest, naresh.kamboju, shuah,
	Arve Hjønnevåg, Martijn Coenen, Joel Fernandes, hridya,
	kernel-team

On Thu, Mar 12, 2020 at 04:53:17PM -0700, Kees Cook wrote:
> On Thu, Mar 12, 2020 at 02:15:30PM +0100, Christian Brauner wrote:
> > This adds a stress test that should hopefully help us catch regressions
> > for [1], [2], and [3].
> > 
> > [1]: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> > [2]: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> > [3]: 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Do you care about 80-char line limits? (Or does the selftest tree? There
> are a few in here...)

They should mostly all be calls to kselftest print functions. I usually
never wrap them in tests so it's easy to do:

git grep "Find me this error string"

which becames annoying when you wrap them. :)

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

* Re: [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint
  2020-03-12 23:54       ` Kees Cook
@ 2020-03-13 12:55         ` Christian Brauner
  0 siblings, 0 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 12:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Greg Kroah-Hartman, Todd Kjos, ard.biesheuvel, ardb, john.stultz,
	linux-kernel, linux-kselftest, naresh.kamboju, shuah,
	Arve Hjønnevåg, Martijn Coenen, Joel Fernandes, hridya,
	kernel-team

On Thu, Mar 12, 2020 at 04:54:25PM -0700, Kees Cook wrote:
> On Thu, Mar 12, 2020 at 02:15:31PM +0100, Christian Brauner wrote:
> > Unprivileged users will be able to create directories in there. The
> > unprivileged test for /dev wouldn't have worked on most systems.
> > 
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Seems good. (Though would a dynamic location be better?
> mkstemp()-style?)

Yeah, when I originally wrote binderfs that was really more of a
test-stub than anything else. I'll see if I can switch to something less
hard-coded. :)

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

* Re: [PATCH] binderfs: port to new mount api
  2020-03-12 23:56       ` Kees Cook
@ 2020-03-13 12:55         ` Christian Brauner
  2020-03-13 12:56           ` Christian Brauner
  0 siblings, 1 reply; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 12:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: ard.biesheuvel, ardb, arve, gregkh, hridya, joel, john.stultz,
	kernel-team, linux-kernel, linux-kselftest, maco, naresh.kamboju,
	shuah, tkjos, Todd Kjos

On Thu, Mar 12, 2020 at 04:56:11PM -0700, Kees Cook wrote:
> On Thu, Mar 12, 2020 at 10:24:20PM +0100, Christian Brauner wrote:
> > It's time we port binderfs to the new mount api. We can make use of the
> > new option parser, get nicer infrastructure and it will be easiert if we
> > ever add any new mount options.
> > 
> > This survives testing with the binderfs selftests:
> > 
> > for i in `seq 1 1000`; do ./binderfs_test; done
> > 
> > including the new stress tests I sent out for review today:
> > 
> >  [==========] Running 3 tests from 1 test cases.
> >  [ RUN      ] global.binderfs_stress
> >  [       OK ] global.binderfs_stress
> >  [ RUN      ] global.binderfs_test_privileged
> >  # Tests are not run as root. Skipping privileged tests
> >  [       OK ] global.binderfs_test_privileged
> 
> I would use the XFAIL harness infrastructure for these kinds of skips.

Hmyeah, will do.

Christian

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

* Re: [PATCH] binderfs: port to new mount api
  2020-03-13 12:55         ` Christian Brauner
@ 2020-03-13 12:56           ` Christian Brauner
  0 siblings, 0 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 12:56 UTC (permalink / raw)
  To: Kees Cook
  Cc: ard.biesheuvel, ardb, arve, gregkh, hridya, joel, john.stultz,
	kernel-team, linux-kernel, linux-kselftest, maco, naresh.kamboju,
	shuah, tkjos, Todd Kjos

On Fri, Mar 13, 2020 at 01:55:53PM +0100, Christian Brauner wrote:
> On Thu, Mar 12, 2020 at 04:56:11PM -0700, Kees Cook wrote:
> > On Thu, Mar 12, 2020 at 10:24:20PM +0100, Christian Brauner wrote:
> > > It's time we port binderfs to the new mount api. We can make use of the
> > > new option parser, get nicer infrastructure and it will be easiert if we
> > > ever add any new mount options.
> > > 
> > > This survives testing with the binderfs selftests:
> > > 
> > > for i in `seq 1 1000`; do ./binderfs_test; done
> > > 
> > > including the new stress tests I sent out for review today:
> > > 
> > >  [==========] Running 3 tests from 1 test cases.
> > >  [ RUN      ] global.binderfs_stress
> > >  [       OK ] global.binderfs_stress
> > >  [ RUN      ] global.binderfs_test_privileged
> > >  # Tests are not run as root. Skipping privileged tests
> > >  [       OK ] global.binderfs_test_privileged
> > 
> > I would use the XFAIL harness infrastructure for these kinds of skips.
> 
> Hmyeah, will do.

For the selftests I sent out earlier that is.
This patch doesn't touch them. I just ran this patch on top of the
new stress test to show it passes.

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

* [PATCH v2 1/3] binderfs: port tests to test harness infrastructure
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
                       ` (3 preceding siblings ...)
  2020-03-12 23:51     ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Kees Cook
@ 2020-03-13 15:24     ` Christian Brauner
  2020-03-13 15:24       ` [PATCH v2 2/3] binderfs_test: switch from /dev to a unique per-test mountpoint Christian Brauner
                         ` (2 more replies)
  2020-03-13 15:34     ` [PATCH v2] binderfs: port to new mount api Christian Brauner
  5 siblings, 3 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 15:24 UTC (permalink / raw)
  To: gregkh, tkjos, keescook, linux-kernel
  Cc: christian.brauner, ard.biesheuvel, ardb, arve, hridya, joel,
	john.stultz, kernel-team, linux-kselftest, maco, naresh.kamboju,
	shuah

Makes for nicer output and prepares for additional tests.

Cc: Kees Cook <keescook@chromium.org>:
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Kees Cook <keescook@chromium.org>:
  - Switch to XFAIL() to skip tests.
---
 .../selftests/filesystems/binderfs/Makefile   |  2 ++
 .../filesystems/binderfs/binderfs_test.c      | 31 +++++++++----------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
index 58cb659b56b4..75315d9ba7a9 100644
--- a/tools/testing/selftests/filesystems/binderfs/Makefile
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -3,4 +3,6 @@
 CFLAGS += -I../../../../../usr/include/
 TEST_GEN_PROGS := binderfs_test
 
+binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
+
 include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 8c2ed962e1c7..0cfca65e095a 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -15,7 +15,9 @@
 #include <unistd.h>
 #include <linux/android/binder.h>
 #include <linux/android/binderfs.h>
+
 #include "../../kselftest.h"
+#include "../../kselftest_harness.h"
 
 static ssize_t write_nointr(int fd, const void *buf, size_t count)
 {
@@ -132,7 +134,7 @@ static void rmdir_protect_errno(const char *dir)
 	errno = saved_errno;
 }
 
-static void __do_binderfs_test(void)
+static int __do_binderfs_test(void)
 {
 	int fd, ret, saved_errno;
 	size_t len;
@@ -160,8 +162,7 @@ static void __do_binderfs_test(void)
 					   strerror(errno));
 
 		keep ? : rmdir_protect_errno("/dev/binderfs");
-		ksft_exit_skip(
-			"The Android binderfs filesystem is not available\n");
+		return 1;
 	}
 
 	/* binderfs mount test passed */
@@ -250,26 +251,24 @@ static void __do_binderfs_test(void)
 
 	/* binderfs unmount test passed */
 	ksft_inc_pass_cnt();
+	return 0;
 }
 
-static void binderfs_test_privileged()
+TEST(binderfs_test_privileged)
 {
 	if (geteuid() != 0)
-		ksft_print_msg(
-			"Tests are not run as root. Skipping privileged tests\n");
-	else
-		__do_binderfs_test();
+		XFAIL(return, "Tests are not run as root. Skipping privileged tests");
+
+	if (__do_binderfs_test() == 1)
+		XFAIL(return, "The Android binderfs filesystem is not available");
 }
 
-static void binderfs_test_unprivileged()
+TEST(binderfs_test_unprivileged)
 {
 	change_to_userns();
-	__do_binderfs_test();
-}
 
-int main(int argc, char *argv[])
-{
-	binderfs_test_privileged();
-	binderfs_test_unprivileged();
-	ksft_exit_pass();
+	if (__do_binderfs_test() == 1)
+		XFAIL(return, "The Android binderfs filesystem is not available");
 }
+
+TEST_HARNESS_MAIN

base-commit: 2c523b344dfa65a3738e7039832044aa133c75fb
-- 
2.25.1


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

* [PATCH v2 2/3] binderfs_test: switch from /dev to a unique per-test mountpoint
  2020-03-13 15:24     ` [PATCH v2 " Christian Brauner
@ 2020-03-13 15:24       ` Christian Brauner
  2020-03-13 23:07         ` Kees Cook
  2020-03-13 15:24       ` [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices Christian Brauner
  2020-03-13 23:07       ` [PATCH v2 1/3] binderfs: port tests to test harness infrastructure Kees Cook
  2 siblings, 1 reply; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 15:24 UTC (permalink / raw)
  To: gregkh, tkjos, keescook, linux-kernel
  Cc: christian.brauner, ard.biesheuvel, ardb, arve, hridya, joel,
	john.stultz, kernel-team, linux-kselftest, maco, naresh.kamboju,
	shuah

Unprivileged users will be able to create directories in there. The
unprivileged test for /dev wouldn't have worked on most systems.

Cc: Kees Cook <keescook@chromium.org>:
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Kees Cook <keescook@chromium.org>:
  - Switch to unique mountpoint through mkdtemp().
---
 .../filesystems/binderfs/binderfs_test.c      | 47 +++++++++----------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 0cfca65e095a..818eb49f8125 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -139,29 +139,25 @@ static int __do_binderfs_test(void)
 	int fd, ret, saved_errno;
 	size_t len;
 	ssize_t wret;
-	bool keep = false;
 	struct binderfs_device device = { 0 };
 	struct binder_version version = { 0 };
+	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
+		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
 
 	change_to_mountns();
 
-	ret = mkdir("/dev/binderfs", 0755);
-	if (ret < 0) {
-		if (errno != EEXIST)
-			ksft_exit_fail_msg(
-				"%s - Failed to create binderfs mountpoint\n",
-				strerror(errno));
-
-		keep = true;
-	}
+	if (!mkdtemp(binderfs_mntpt))
+		ksft_exit_fail_msg(
+			"%s - Failed to create binderfs mountpoint\n",
+			strerror(errno));
 
-	ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
+	ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
 	if (ret < 0) {
 		if (errno != ENODEV)
 			ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
 					   strerror(errno));
 
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		rmdir_protect_errno(binderfs_mntpt);
 		return 1;
 	}
 
@@ -170,7 +166,8 @@ static int __do_binderfs_test(void)
 
 	memcpy(device.name, "my-binder", strlen("my-binder"));
 
-	fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
+	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
+	fd = open(device_path, O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
 		ksft_exit_fail_msg(
 			"%s - Failed to open binder-control device\n",
@@ -181,7 +178,7 @@ static int __do_binderfs_test(void)
 	close(fd);
 	errno = saved_errno;
 	if (ret < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		rmdir_protect_errno(binderfs_mntpt);
 		ksft_exit_fail_msg(
 			"%s - Failed to allocate new binder device\n",
 			strerror(errno));
@@ -194,9 +191,10 @@ static int __do_binderfs_test(void)
 	/* binder device allocation test passed */
 	ksft_inc_pass_cnt();
 
-	fd = open("/dev/binderfs/my-binder", O_CLOEXEC | O_RDONLY);
+	snprintf(device_path, sizeof(device_path), "%s/my-binder", binderfs_mntpt);
+	fd = open(device_path, O_CLOEXEC | O_RDONLY);
 	if (fd < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		rmdir_protect_errno(binderfs_mntpt);
 		ksft_exit_fail_msg("%s - Failed to open my-binder device\n",
 				   strerror(errno));
 	}
@@ -206,7 +204,7 @@ static int __do_binderfs_test(void)
 	close(fd);
 	errno = saved_errno;
 	if (ret < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		rmdir_protect_errno(binderfs_mntpt);
 		ksft_exit_fail_msg(
 			"%s - Failed to open perform BINDER_VERSION request\n",
 			strerror(errno));
@@ -218,9 +216,9 @@ static int __do_binderfs_test(void)
 	/* binder transaction with binderfs binder device passed */
 	ksft_inc_pass_cnt();
 
-	ret = unlink("/dev/binderfs/my-binder");
+	ret = unlink(device_path);
 	if (ret < 0) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		rmdir_protect_errno(binderfs_mntpt);
 		ksft_exit_fail_msg("%s - Failed to delete binder device\n",
 				   strerror(errno));
 	}
@@ -228,12 +226,13 @@ static int __do_binderfs_test(void)
 	/* binder device removal passed */
 	ksft_inc_pass_cnt();
 
-	ret = unlink("/dev/binderfs/binder-control");
+	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
+	ret = unlink(device_path);
 	if (!ret) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		rmdir_protect_errno(binderfs_mntpt);
 		ksft_exit_fail_msg("Managed to delete binder-control device\n");
 	} else if (errno != EPERM) {
-		keep ? : rmdir_protect_errno("/dev/binderfs");
+		rmdir_protect_errno(binderfs_mntpt);
 		ksft_exit_fail_msg(
 			"%s - Failed to delete binder-control device but exited with unexpected error code\n",
 			strerror(errno));
@@ -243,8 +242,8 @@ static int __do_binderfs_test(void)
 	ksft_inc_xfail_cnt();
 
 on_error:
-	ret = umount2("/dev/binderfs", MNT_DETACH);
-	keep ?: rmdir_protect_errno("/dev/binderfs");
+	ret = umount2(binderfs_mntpt, MNT_DETACH);
+	rmdir_protect_errno(binderfs_mntpt);
 	if (ret < 0)
 		ksft_exit_fail_msg("%s - Failed to unmount binderfs\n",
 				   strerror(errno));
-- 
2.25.1


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

* [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices
  2020-03-13 15:24     ` [PATCH v2 " Christian Brauner
  2020-03-13 15:24       ` [PATCH v2 2/3] binderfs_test: switch from /dev to a unique per-test mountpoint Christian Brauner
@ 2020-03-13 15:24       ` Christian Brauner
  2020-03-13 23:08         ` Kees Cook
  2020-03-13 23:07       ` [PATCH v2 1/3] binderfs: port tests to test harness infrastructure Kees Cook
  2 siblings, 1 reply; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 15:24 UTC (permalink / raw)
  To: gregkh, tkjos, keescook, linux-kernel
  Cc: christian.brauner, ard.biesheuvel, ardb, arve, hridya, joel,
	john.stultz, kernel-team, linux-kselftest, maco, naresh.kamboju,
	shuah

This adds a stress test that should hopefully help us catch regressions
for [1], [2], and [3].

[1]: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
[2]: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
[3]: 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
Cc: Kees Cook <keescook@chromium.org>:
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Kees Cook <keescook@chromium.org>:
  - Switch to unique mountpoint through mkdtemp().
---
 .../selftests/filesystems/binderfs/Makefile   |   2 +-
 .../filesystems/binderfs/binderfs_test.c      | 426 ++++++++++++++----
 2 files changed, 334 insertions(+), 94 deletions(-)

diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
index 75315d9ba7a9..8af25ae96049 100644
--- a/tools/testing/selftests/filesystems/binderfs/Makefile
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-CFLAGS += -I../../../../../usr/include/
+CFLAGS += -I../../../../../usr/include/ -pthread
 TEST_GEN_PROGS := binderfs_test
 
 binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
index 818eb49f8125..8a6b507e34a8 100644
--- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -3,15 +3,20 @@
 #define _GNU_SOURCE
 #include <errno.h>
 #include <fcntl.h>
+#include <pthread.h>
 #include <sched.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/fsuid.h>
 #include <sys/ioctl.h>
 #include <sys/mount.h>
+#include <sys/socket.h>
 #include <sys/stat.h>
+#include <sys/sysinfo.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <unistd.h>
 #include <linux/android/binder.h>
 #include <linux/android/binderfs.h>
@@ -19,100 +24,26 @@
 #include "../../kselftest.h"
 #include "../../kselftest_harness.h"
 
-static ssize_t write_nointr(int fd, const void *buf, size_t count)
-{
-	ssize_t ret;
-again:
-	ret = write(fd, buf, count);
-	if (ret < 0 && errno == EINTR)
-		goto again;
-
-	return ret;
-}
-
-static void write_to_file(const char *filename, const void *buf, size_t count,
-			  int allowed_errno)
-{
-	int fd, saved_errno;
-	ssize_t ret;
-
-	fd = open(filename, O_WRONLY | O_CLOEXEC);
-	if (fd < 0)
-		ksft_exit_fail_msg("%s - Failed to open file %s\n",
-				   strerror(errno), filename);
+#define DEFAULT_THREADS 4
 
-	ret = write_nointr(fd, buf, count);
-	if (ret < 0) {
-		if (allowed_errno && (errno == allowed_errno)) {
-			close(fd);
-			return;
-		}
+#define PTR_TO_INT(p) ((int)((intptr_t)(p)))
+#define INT_TO_PTR(u) ((void *)((intptr_t)(u)))
 
-		goto on_error;
+#define close_prot_errno_disarm(fd) \
+	if (fd >= 0) {              \
+		int _e_ = errno;    \
+		close(fd);          \
+		errno = _e_;        \
+		fd = -EBADF;        \
 	}
 
-	if ((size_t)ret != count)
-		goto on_error;
-
-	close(fd);
-	return;
-
-on_error:
-	saved_errno = errno;
-	close(fd);
-	errno = saved_errno;
-
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to write to file %s\n",
-				   strerror(errno), filename);
-
-	ksft_exit_fail_msg("Failed to write to file %s\n", filename);
-}
-
-static void change_to_userns(void)
-{
-	int ret;
-	uid_t uid;
-	gid_t gid;
-	/* {g,u}id_map files only allow a max of 4096 bytes written to them */
-	char idmap[4096];
-
-	uid = getuid();
-	gid = getgid();
-
-	ret = unshare(CLONE_NEWUSER);
-	if (ret < 0)
-		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
-				   strerror(errno));
-
-	write_to_file("/proc/self/setgroups", "deny", strlen("deny"), ENOENT);
-
-	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", uid);
-	if (ret < 0 || (size_t)ret >= sizeof(idmap))
-		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
-				   strerror(errno));
-
-	write_to_file("/proc/self/uid_map", idmap, strlen(idmap), 0);
-
-	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", gid);
-	if (ret < 0 || (size_t)ret >= sizeof(idmap))
-		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
-				   strerror(errno));
-
-	write_to_file("/proc/self/gid_map", idmap, strlen(idmap), 0);
-
-	ret = setgid(0);
-	if (ret)
-		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
-				   strerror(errno));
+#define log_exit(format, ...)                                                  \
+	({                                                                     \
+		fprintf(stderr, format "\n", ##__VA_ARGS__);                   \
+		exit(EXIT_FAILURE);                                            \
+	})
 
-	ret = setuid(0);
-	if (ret)
-		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
-				   strerror(errno));
-}
-
-static void change_to_mountns(void)
+static void change_mountns(void)
 {
 	int ret;
 
@@ -144,7 +75,7 @@ static int __do_binderfs_test(void)
 	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
 		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
 
-	change_to_mountns();
+	change_mountns();
 
 	if (!mkdtemp(binderfs_mntpt))
 		ksft_exit_fail_msg(
@@ -253,6 +184,288 @@ static int __do_binderfs_test(void)
 	return 0;
 }
 
+static int wait_for_pid(pid_t pid)
+{
+	int status, ret;
+
+again:
+	ret = waitpid(pid, &status, 0);
+	if (ret == -1) {
+		if (errno == EINTR)
+			goto again;
+
+		return -1;
+	}
+
+	if (!WIFEXITED(status))
+		return -1;
+
+	return WEXITSTATUS(status);
+}
+
+static int setid_userns_root(void)
+{
+	if (setuid(0))
+		return -1;
+	if (setgid(0))
+		return -1;
+
+	setfsuid(0);
+	setfsgid(0);
+
+	return 0;
+}
+
+enum idmap_type {
+	UID_MAP,
+	GID_MAP,
+};
+
+static ssize_t read_nointr(int fd, void *buf, size_t count)
+{
+	ssize_t ret;
+again:
+	ret = read(fd, buf, count);
+	if (ret < 0 && errno == EINTR)
+		goto again;
+
+	return ret;
+}
+
+static ssize_t write_nointr(int fd, const void *buf, size_t count)
+{
+	ssize_t ret;
+again:
+	ret = write(fd, buf, count);
+	if (ret < 0 && errno == EINTR)
+		goto again;
+
+	return ret;
+}
+
+static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
+			    size_t buf_size)
+{
+	int fd;
+	int ret;
+	char path[4096];
+
+	if (type == GID_MAP) {
+		int setgroups_fd;
+
+		snprintf(path, sizeof(path), "/proc/%d/setgroups", pid);
+		setgroups_fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
+		if (setgroups_fd < 0 && errno != ENOENT)
+			return -1;
+
+		if (setgroups_fd >= 0) {
+			ret = write_nointr(setgroups_fd, "deny", sizeof("deny") - 1);
+			close_prot_errno_disarm(setgroups_fd);
+			if (ret != sizeof("deny") - 1)
+				return -1;
+		}
+	}
+
+	switch (type) {
+	case UID_MAP:
+		ret = snprintf(path, sizeof(path), "/proc/%d/uid_map", pid);
+		break;
+	case GID_MAP:
+		ret = snprintf(path, sizeof(path), "/proc/%d/gid_map", pid);
+		break;
+	default:
+		return -1;
+	}
+	if (ret < 0 || ret >= sizeof(path))
+		return -E2BIG;
+
+	fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
+	if (fd < 0)
+		return -1;
+
+	ret = write_nointr(fd, buf, buf_size);
+	close_prot_errno_disarm(fd);
+	if (ret != buf_size)
+		return -1;
+
+	return 0;
+}
+
+static void change_userns(int syncfds[2])
+{
+	int ret;
+	char buf;
+
+	close_prot_errno_disarm(syncfds[1]);
+
+	ret = unshare(CLONE_NEWUSER);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
+				   strerror(errno));
+
+	ret = write_nointr(syncfds[0], "1", 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("write_nointr() failed\n");
+
+	ret = read_nointr(syncfds[0], &buf, 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("read_nointr() failed\n");
+
+	close_prot_errno_disarm(syncfds[0]);
+
+	if (setid_userns_root())
+		ksft_exit_fail_msg("setid_userns_root() failed");
+}
+
+static void change_idmaps(int syncfds[2], pid_t pid)
+{
+	int ret;
+	char buf;
+	char id_map[4096];
+
+	close_prot_errno_disarm(syncfds[0]);
+
+	ret = read_nointr(syncfds[1], &buf, 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("read_nointr() failed\n");
+
+	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
+	ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
+	if (ret)
+		ksft_exit_fail_msg("write_id_mapping(UID_MAP) failed");
+
+	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
+	ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
+	if (ret)
+		ksft_exit_fail_msg("write_id_mapping(GID_MAP) failed");
+
+	ret = write_nointr(syncfds[1], "1", 1);
+	if (ret != 1)
+		ksft_exit_fail_msg("write_nointr() failed");
+
+	close_prot_errno_disarm(syncfds[1]);
+}
+
+static void *binder_version_thread(void *data)
+{
+	int fd = PTR_TO_INT(data);
+	struct binder_version version = { 0 };
+	int ret;
+
+	ret = ioctl(fd, BINDER_VERSION, &version);
+	if (ret < 0)
+		ksft_print_msg("%s - Failed to open perform BINDER_VERSION request\n", strerror(errno));
+
+	pthread_exit(data);
+}
+
+/*
+ * Regression test:
+ * 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
+ * f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
+ * 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
+ */
+TEST(binderfs_stress)
+{
+	int fds[1000];
+	int syncfds[2];
+	pid_t pid;
+	int fd, ret;
+	size_t len;
+	struct binderfs_device device = { 0 };
+	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
+		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
+
+	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
+
+	pid = fork();
+	if (pid < 0) {
+		close_prot_errno_disarm(syncfds[0]);
+		close_prot_errno_disarm(syncfds[1]);
+		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
+	}
+
+	if (pid == 0) {
+		int i, j, k, nthreads;
+		pthread_attr_t attr;
+		pthread_t threads[DEFAULT_THREADS];
+		change_userns(syncfds);
+		change_mountns();
+
+		if (!mkdtemp(binderfs_mntpt))
+			log_exit("%s - Failed to create binderfs mountpoint\n",
+				 strerror(errno));
+
+		ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
+		if (ret < 0)
+			log_exit("%s - Failed to mount binderfs\n", strerror(errno));
+
+		for (int i = 0; i < ARRAY_SIZE(fds); i++) {
+
+			snprintf(device_path, sizeof(device_path),
+				 "%s/binder-control", binderfs_mntpt);
+			fd = open(device_path, O_RDONLY | O_CLOEXEC);
+			if (fd < 0)
+				log_exit("%s - Failed to open binder-control device\n", strerror(errno));
+
+			memset(&device, 0, sizeof(device));
+			snprintf(device.name, sizeof(device.name), "%d", i);
+			ret = ioctl(fd, BINDER_CTL_ADD, &device);
+			close_prot_errno_disarm(fd);
+			if (ret < 0)
+				log_exit("%s - Failed to allocate new binder device\n", strerror(errno));
+
+			snprintf(device_path, sizeof(device_path), "%s/%d",
+				 binderfs_mntpt, i);
+			fds[i] = open(device_path, O_RDONLY | O_CLOEXEC);
+			if (fds[i] < 0)
+				log_exit("%s - Failed to open binder device\n", strerror(errno));
+		}
+
+		ret = umount2(binderfs_mntpt, MNT_DETACH);
+		rmdir_protect_errno(binderfs_mntpt);
+		if (ret < 0)
+			log_exit("%s - Failed to unmount binderfs\n", strerror(errno));
+
+		nthreads = get_nprocs_conf();
+		if (nthreads > DEFAULT_THREADS)
+			nthreads = DEFAULT_THREADS;
+
+		pthread_attr_init(&attr);
+		for (k = 0; k < ARRAY_SIZE(fds); k++) {
+			for (i = 0; i < nthreads; i++) {
+				ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
+				if (ret) {
+					ksft_print_msg("%s - Failed to create thread %d\n", strerror(errno), i);
+					break;
+				}
+			}
+
+			for (j = 0; j < i; j++) {
+				void *fdptr = NULL;
+
+				ret = pthread_join(threads[j], &fdptr);
+				if (ret)
+					ksft_print_msg("%s - Failed to join thread %d for fd %d\n", strerror(errno), j, PTR_TO_INT(fdptr));
+			}
+		}
+		pthread_attr_destroy(&attr);
+
+		for (k = 0; k < ARRAY_SIZE(fds); k++)
+			close(fds[k]);
+
+		exit(EXIT_SUCCESS);
+	}
+
+	change_idmaps(syncfds, pid);
+
+	ret = wait_for_pid(pid);
+	if (ret)
+		ksft_exit_fail_msg("wait_for_pid() failed");
+}
+
 TEST(binderfs_test_privileged)
 {
 	if (geteuid() != 0)
@@ -264,10 +477,37 @@ TEST(binderfs_test_privileged)
 
 TEST(binderfs_test_unprivileged)
 {
-	change_to_userns();
+	int ret;
+	int syncfds[2];
+	pid_t pid;
 
-	if (__do_binderfs_test() == 1)
-		XFAIL(return, "The Android binderfs filesystem is not available");
+	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
+
+	pid = fork();
+	if (pid < 0) {
+		close_prot_errno_disarm(syncfds[0]);
+		close_prot_errno_disarm(syncfds[1]);
+		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
+	}
+
+	if (pid == 0) {
+		change_userns(syncfds);
+		if (__do_binderfs_test() == 1)
+			exit(2);
+		exit(EXIT_SUCCESS);
+	}
+
+	change_idmaps(syncfds, pid);
+
+	ret = wait_for_pid(pid);
+	if (ret) {
+		if (ret == 2)
+			XFAIL(return, "The Android binderfs filesystem is not available");
+		else
+			ksft_exit_fail_msg("wait_for_pid() failed");
+	}
 }
 
 TEST_HARNESS_MAIN
-- 
2.25.1


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

* [PATCH v2] binderfs: port to new mount api
  2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
                       ` (4 preceding siblings ...)
  2020-03-13 15:24     ` [PATCH v2 " Christian Brauner
@ 2020-03-13 15:34     ` Christian Brauner
  2020-03-13 23:08       ` Kees Cook
  2020-03-18 12:29       ` Greg KH
  5 siblings, 2 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-13 15:34 UTC (permalink / raw)
  To: gregkh, tkjos, keescook, linux-kernel
  Cc: christian.brauner, ard.biesheuvel, ardb, arve, hridya, joel,
	john.stultz, kernel-team, linux-kselftest, maco, naresh.kamboju,
	shuah, Todd Kjos

When I first wrote binderfs the new mount api had not yet landed. Now
that it has been around for a little while and a bunch of filesystems
have already been ported we should do so too. When Al sent his
mount-api-conversion pr he requested that binderfs (and a few others) be
ported separately. It's time we port binderfs. We can make use of the
new option parser, get nicer infrastructure and it will be easier if we
ever add any new mount options.

This survives testing with the binderfs selftests:

for i in `seq 1 1000`; do ./binderfs_test; done

including the new stress tests I sent out for review today:

 TAP version 13
 1..1
 # selftests: filesystems/binderfs: binderfs_test
 # [==========] Running 3 tests from 1 test cases.
 # [ RUN      ] global.binderfs_stress
 # [  XFAIL!  ] Tests are not run as root. Skipping privileged tests
 # [==========] Running 3 tests from 1 test cases.
 # [ RUN      ] global.binderfs_stress
 # [       OK ] global.binderfs_stress
 # [ RUN      ] global.binderfs_test_privileged
 # [       OK ] global.binderfs_test_privileged
 # [ RUN      ] global.binderfs_test_unprivileged
 # # Allocated new binder device with major 243, minor 4, and name my-binder
 # # Detected binder version: 8
 # [==========] Running 3 tests from 1 test cases.
 # [ RUN      ] global.binderfs_stress
 # [       OK ] global.binderfs_stress
 # [ RUN      ] global.binderfs_test_privileged
 # [       OK ] global.binderfs_test_privileged
 # [ RUN      ] global.binderfs_test_unprivileged
 # [       OK ] global.binderfs_test_unprivileged
 # [==========] 3 / 3 tests passed.
 # [  PASSED  ]
 ok 1 selftests: filesystems/binderfs: binderfs_test

Cc: Todd Kjos <tkjos@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v2 */
- Christian Brauner <christian.brauner@ubuntu.com>:
  - Commit message adapted to new stresstest output after porting to
    XFAIL infrastructure.
    For the stresstest patchset see:
    https://lore.kernel.org/r/20200313152420.138777-1-christian.brauner@ubuntu.com
---
 drivers/android/binderfs.c | 200 +++++++++++++++++++------------------
 1 file changed, 104 insertions(+), 96 deletions(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index f303106b3362..9ecad74183a3 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -18,7 +18,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/mount.h>
-#include <linux/parser.h>
+#include <linux/fs_parser.h>
 #include <linux/radix-tree.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
@@ -48,26 +48,30 @@ static dev_t binderfs_dev;
 static DEFINE_MUTEX(binderfs_minors_mutex);
 static DEFINE_IDA(binderfs_minors);
 
-enum {
+enum binderfs_param {
 	Opt_max,
 	Opt_stats_mode,
-	Opt_err
 };
 
 enum binderfs_stats_mode {
-	STATS_NONE,
-	STATS_GLOBAL,
+	binderfs_stats_mode_unset,
+	binderfs_stats_mode_global,
 };
 
-static const match_table_t tokens = {
-	{ Opt_max, "max=%d" },
-	{ Opt_stats_mode, "stats=%s" },
-	{ Opt_err, NULL     }
+static const struct constant_table binderfs_param_stats[] = {
+	{ "global", binderfs_stats_mode_global },
+	{}
 };
 
-static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
+const struct fs_parameter_spec binderfs_fs_parameters[] = {
+	fsparam_u32("max",	Opt_max),
+	fsparam_enum("stats",	Opt_stats_mode, binderfs_param_stats),
+	{}
+};
+
+static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
 {
-	return inode->i_sb->s_fs_info;
+	return sb->s_fs_info;
 }
 
 bool is_binderfs_device(const struct inode *inode)
@@ -246,7 +250,7 @@ static long binder_ctl_ioctl(struct file *file, unsigned int cmd,
 static void binderfs_evict_inode(struct inode *inode)
 {
 	struct binder_device *device = inode->i_private;
-	struct binderfs_info *info = BINDERFS_I(inode);
+	struct binderfs_info *info = BINDERFS_SB(inode->i_sb);
 
 	clear_inode(inode);
 
@@ -264,97 +268,84 @@ static void binderfs_evict_inode(struct inode *inode)
 	}
 }
 
-/**
- * binderfs_parse_mount_opts - parse binderfs mount options
- * @data: options to set (can be NULL in which case defaults are used)
- */
-static int binderfs_parse_mount_opts(char *data,
-				     struct binderfs_mount_opts *opts)
+static int binderfs_fs_context_parse_param(struct fs_context *fc,
+					   struct fs_parameter *param)
 {
-	char *p, *stats;
-	opts->max = BINDERFS_MAX_MINOR;
-	opts->stats_mode = STATS_NONE;
-
-	while ((p = strsep(&data, ",")) != NULL) {
-		substring_t args[MAX_OPT_ARGS];
-		int token;
-		int max_devices;
-
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_max:
-			if (match_int(&args[0], &max_devices) ||
-			    (max_devices < 0 ||
-			     (max_devices > BINDERFS_MAX_MINOR)))
-				return -EINVAL;
-
-			opts->max = max_devices;
-			break;
-		case Opt_stats_mode:
-			if (!capable(CAP_SYS_ADMIN))
-				return -EINVAL;
+	int opt;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
+	struct fs_parse_result result;
 
-			stats = match_strdup(&args[0]);
-			if (!stats)
-				return -ENOMEM;
+	opt = fs_parse(fc, binderfs_fs_parameters, param, &result);
+	if (opt < 0)
+		return opt;
 
-			if (strcmp(stats, "global") != 0) {
-				kfree(stats);
-				return -EINVAL;
-			}
+	switch (opt) {
+	case Opt_max:
+		if (result.uint_32 > BINDERFS_MAX_MINOR)
+			return invalfc(fc, "Bad value for '%s'", param->key);
 
-			opts->stats_mode = STATS_GLOBAL;
-			kfree(stats);
-			break;
-		default:
-			pr_err("Invalid mount options\n");
-			return -EINVAL;
-		}
+		ctx->max = result.uint_32;
+		break;
+	case Opt_stats_mode:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		ctx->stats_mode = result.uint_32;
+		break;
+	default:
+		return invalfc(fc, "Unsupported parameter '%s'", param->key);
 	}
 
 	return 0;
 }
 
-static int binderfs_remount(struct super_block *sb, int *flags, char *data)
+static int binderfs_fs_context_reconfigure(struct fs_context *fc)
 {
-	int prev_stats_mode, ret;
-	struct binderfs_info *info = sb->s_fs_info;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
+	struct binderfs_info *info = BINDERFS_SB(fc->root->d_sb);
 
-	prev_stats_mode = info->mount_opts.stats_mode;
-	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
-	if (ret)
-		return ret;
-
-	if (prev_stats_mode != info->mount_opts.stats_mode) {
-		pr_err("Binderfs stats mode cannot be changed during a remount\n");
-		info->mount_opts.stats_mode = prev_stats_mode;
-		return -EINVAL;
-	}
+	if (info->mount_opts.stats_mode != ctx->stats_mode)
+		return invalfc(fc, "Binderfs stats mode cannot be changed during a remount");
 
+	info->mount_opts.stats_mode = ctx->stats_mode;
+	info->mount_opts.max = ctx->max;
 	return 0;
 }
 
-static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
+static int binderfs_show_options(struct seq_file *seq, struct dentry *root)
 {
-	struct binderfs_info *info;
+	struct binderfs_info *info = BINDERFS_SB(root->d_sb);
 
-	info = root->d_sb->s_fs_info;
 	if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
 		seq_printf(seq, ",max=%d", info->mount_opts.max);
-	if (info->mount_opts.stats_mode == STATS_GLOBAL)
+
+	switch (info->mount_opts.stats_mode) {
+	case binderfs_stats_mode_unset:
+		break;
+	case binderfs_stats_mode_global:
 		seq_printf(seq, ",stats=global");
+		break;
+	}
 
 	return 0;
 }
 
+static void binderfs_put_super(struct super_block *sb)
+{
+	struct binderfs_info *info = sb->s_fs_info;
+
+	if (info && info->ipc_ns)
+		put_ipc_ns(info->ipc_ns);
+
+	kfree(info);
+	sb->s_fs_info = NULL;
+}
+
 static const struct super_operations binderfs_super_ops = {
 	.evict_inode    = binderfs_evict_inode,
-	.remount_fs	= binderfs_remount,
-	.show_options	= binderfs_show_mount_opts,
+	.show_options	= binderfs_show_options,
 	.statfs         = simple_statfs,
+	.put_super	= binderfs_put_super,
 };
 
 static inline bool is_binderfs_control_device(const struct dentry *dentry)
@@ -653,10 +644,11 @@ static int init_binder_logs(struct super_block *sb)
 	return ret;
 }
 
-static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
+static int binderfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	int ret;
 	struct binderfs_info *info;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
 	struct inode *inode = NULL;
 	struct binderfs_device device_info = { 0 };
 	const char *name;
@@ -689,16 +681,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 
 	info->ipc_ns = get_ipc_ns(current->nsproxy->ipc_ns);
 
-	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
-	if (ret)
-		return ret;
-
 	info->root_gid = make_kgid(sb->s_user_ns, 0);
 	if (!gid_valid(info->root_gid))
 		info->root_gid = GLOBAL_ROOT_GID;
 	info->root_uid = make_kuid(sb->s_user_ns, 0);
 	if (!uid_valid(info->root_uid))
 		info->root_uid = GLOBAL_ROOT_UID;
+	info->mount_opts.max = ctx->max;
+	info->mount_opts.stats_mode = ctx->stats_mode;
 
 	inode = new_inode(sb);
 	if (!inode)
@@ -730,36 +720,54 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 			name++;
 	}
 
-	if (info->mount_opts.stats_mode == STATS_GLOBAL)
+	if (info->mount_opts.stats_mode == binderfs_stats_mode_global)
 		return init_binder_logs(sb);
 
 	return 0;
 }
 
-static struct dentry *binderfs_mount(struct file_system_type *fs_type,
-				     int flags, const char *dev_name,
-				     void *data)
+static int binderfs_fs_context_get_tree(struct fs_context *fc)
 {
-	return mount_nodev(fs_type, flags, data, binderfs_fill_super);
+	return get_tree_nodev(fc, binderfs_fill_super);
 }
 
-static void binderfs_kill_super(struct super_block *sb)
+static void binderfs_fs_context_free(struct fs_context *fc)
 {
-	struct binderfs_info *info = sb->s_fs_info;
+	struct binderfs_mount_opts *ctx = fc->fs_private;
 
-	kill_litter_super(sb);
+	kfree(ctx);
+}
 
-	if (info && info->ipc_ns)
-		put_ipc_ns(info->ipc_ns);
+static const struct fs_context_operations binderfs_fs_context_ops = {
+	.free		= binderfs_fs_context_free,
+	.get_tree	= binderfs_fs_context_get_tree,
+	.parse_param	= binderfs_fs_context_parse_param,
+	.reconfigure	= binderfs_fs_context_reconfigure,
+};
 
-	kfree(info);
+static int binderfs_init_fs_context(struct fs_context *fc)
+{
+	struct binderfs_mount_opts *ctx = fc->fs_private;
+
+	ctx = kzalloc(sizeof(struct binderfs_mount_opts), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->max = BINDERFS_MAX_MINOR;
+	ctx->stats_mode = binderfs_stats_mode_unset;
+
+	fc->fs_private = ctx;
+	fc->ops = &binderfs_fs_context_ops;
+
+	return 0;
 }
 
 static struct file_system_type binder_fs_type = {
-	.name		= "binder",
-	.mount		= binderfs_mount,
-	.kill_sb	= binderfs_kill_super,
-	.fs_flags	= FS_USERNS_MOUNT,
+	.name			= "binder",
+	.init_fs_context	= binderfs_init_fs_context,
+	.parameters		= binderfs_fs_parameters,
+	.kill_sb		= kill_litter_super,
+	.fs_flags		= FS_USERNS_MOUNT,
 };
 
 int __init init_binderfs(void)

base-commit: f17f06a0c7794d3a7c2425663738823354447472
-- 
2.25.1


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

* Re: [PATCH v2 1/3] binderfs: port tests to test harness infrastructure
  2020-03-13 15:24     ` [PATCH v2 " Christian Brauner
  2020-03-13 15:24       ` [PATCH v2 2/3] binderfs_test: switch from /dev to a unique per-test mountpoint Christian Brauner
  2020-03-13 15:24       ` [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices Christian Brauner
@ 2020-03-13 23:07       ` Kees Cook
  2 siblings, 0 replies; 29+ messages in thread
From: Kees Cook @ 2020-03-13 23:07 UTC (permalink / raw)
  To: Christian Brauner
  Cc: gregkh, tkjos, linux-kernel, ard.biesheuvel, ardb, arve, hridya,
	joel, john.stultz, kernel-team, linux-kselftest, maco,
	naresh.kamboju, shuah

On Fri, Mar 13, 2020 at 04:24:18PM +0100, Christian Brauner wrote:
> Makes for nicer output and prepares for additional tests.
> 
> Cc: Kees Cook <keescook@chromium.org>:
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> /* v2 */
> - Kees Cook <keescook@chromium.org>:
>   - Switch to XFAIL() to skip tests.
> ---
>  .../selftests/filesystems/binderfs/Makefile   |  2 ++
>  .../filesystems/binderfs/binderfs_test.c      | 31 +++++++++----------
>  2 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> index 58cb659b56b4..75315d9ba7a9 100644
> --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> @@ -3,4 +3,6 @@
>  CFLAGS += -I../../../../../usr/include/
>  TEST_GEN_PROGS := binderfs_test
>  
> +binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> +
>  include ../../lib.mk
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 8c2ed962e1c7..0cfca65e095a 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -15,7 +15,9 @@
>  #include <unistd.h>
>  #include <linux/android/binder.h>
>  #include <linux/android/binderfs.h>
> +
>  #include "../../kselftest.h"
> +#include "../../kselftest_harness.h"
>  
>  static ssize_t write_nointr(int fd, const void *buf, size_t count)
>  {
> @@ -132,7 +134,7 @@ static void rmdir_protect_errno(const char *dir)
>  	errno = saved_errno;
>  }
>  
> -static void __do_binderfs_test(void)
> +static int __do_binderfs_test(void)
>  {
>  	int fd, ret, saved_errno;
>  	size_t len;
> @@ -160,8 +162,7 @@ static void __do_binderfs_test(void)
>  					   strerror(errno));
>  
>  		keep ? : rmdir_protect_errno("/dev/binderfs");
> -		ksft_exit_skip(
> -			"The Android binderfs filesystem is not available\n");
> +		return 1;
>  	}
>  
>  	/* binderfs mount test passed */
> @@ -250,26 +251,24 @@ static void __do_binderfs_test(void)
>  
>  	/* binderfs unmount test passed */
>  	ksft_inc_pass_cnt();
> +	return 0;
>  }
>  
> -static void binderfs_test_privileged()
> +TEST(binderfs_test_privileged)
>  {
>  	if (geteuid() != 0)
> -		ksft_print_msg(
> -			"Tests are not run as root. Skipping privileged tests\n");
> -	else
> -		__do_binderfs_test();
> +		XFAIL(return, "Tests are not run as root. Skipping privileged tests");
> +
> +	if (__do_binderfs_test() == 1)
> +		XFAIL(return, "The Android binderfs filesystem is not available");
>  }
>  
> -static void binderfs_test_unprivileged()
> +TEST(binderfs_test_unprivileged)
>  {
>  	change_to_userns();
> -	__do_binderfs_test();
> -}
>  
> -int main(int argc, char *argv[])
> -{
> -	binderfs_test_privileged();
> -	binderfs_test_unprivileged();
> -	ksft_exit_pass();
> +	if (__do_binderfs_test() == 1)
> +		XFAIL(return, "The Android binderfs filesystem is not available");
>  }
> +
> +TEST_HARNESS_MAIN
> 
> base-commit: 2c523b344dfa65a3738e7039832044aa133c75fb
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH v2 2/3] binderfs_test: switch from /dev to a unique per-test mountpoint
  2020-03-13 15:24       ` [PATCH v2 2/3] binderfs_test: switch from /dev to a unique per-test mountpoint Christian Brauner
@ 2020-03-13 23:07         ` Kees Cook
  0 siblings, 0 replies; 29+ messages in thread
From: Kees Cook @ 2020-03-13 23:07 UTC (permalink / raw)
  To: Christian Brauner
  Cc: gregkh, tkjos, linux-kernel, ard.biesheuvel, ardb, arve, hridya,
	joel, john.stultz, kernel-team, linux-kselftest, maco,
	naresh.kamboju, shuah

On Fri, Mar 13, 2020 at 04:24:19PM +0100, Christian Brauner wrote:
> Unprivileged users will be able to create directories in there. The
> unprivileged test for /dev wouldn't have worked on most systems.
> 
> Cc: Kees Cook <keescook@chromium.org>:
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> /* v2 */
> - Kees Cook <keescook@chromium.org>:
>   - Switch to unique mountpoint through mkdtemp().
> ---
>  .../filesystems/binderfs/binderfs_test.c      | 47 +++++++++----------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 0cfca65e095a..818eb49f8125 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -139,29 +139,25 @@ static int __do_binderfs_test(void)
>  	int fd, ret, saved_errno;
>  	size_t len;
>  	ssize_t wret;
> -	bool keep = false;
>  	struct binderfs_device device = { 0 };
>  	struct binder_version version = { 0 };
> +	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
> +		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
>  
>  	change_to_mountns();
>  
> -	ret = mkdir("/dev/binderfs", 0755);
> -	if (ret < 0) {
> -		if (errno != EEXIST)
> -			ksft_exit_fail_msg(
> -				"%s - Failed to create binderfs mountpoint\n",
> -				strerror(errno));
> -
> -		keep = true;
> -	}
> +	if (!mkdtemp(binderfs_mntpt))
> +		ksft_exit_fail_msg(
> +			"%s - Failed to create binderfs mountpoint\n",
> +			strerror(errno));
>  
> -	ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
> +	ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
>  	if (ret < 0) {
>  		if (errno != ENODEV)
>  			ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
>  					   strerror(errno));
>  
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		rmdir_protect_errno(binderfs_mntpt);
>  		return 1;
>  	}
>  
> @@ -170,7 +166,8 @@ static int __do_binderfs_test(void)
>  
>  	memcpy(device.name, "my-binder", strlen("my-binder"));
>  
> -	fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
> +	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
> +	fd = open(device_path, O_RDONLY | O_CLOEXEC);
>  	if (fd < 0)
>  		ksft_exit_fail_msg(
>  			"%s - Failed to open binder-control device\n",
> @@ -181,7 +178,7 @@ static int __do_binderfs_test(void)
>  	close(fd);
>  	errno = saved_errno;
>  	if (ret < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		rmdir_protect_errno(binderfs_mntpt);
>  		ksft_exit_fail_msg(
>  			"%s - Failed to allocate new binder device\n",
>  			strerror(errno));
> @@ -194,9 +191,10 @@ static int __do_binderfs_test(void)
>  	/* binder device allocation test passed */
>  	ksft_inc_pass_cnt();
>  
> -	fd = open("/dev/binderfs/my-binder", O_CLOEXEC | O_RDONLY);
> +	snprintf(device_path, sizeof(device_path), "%s/my-binder", binderfs_mntpt);
> +	fd = open(device_path, O_CLOEXEC | O_RDONLY);
>  	if (fd < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		rmdir_protect_errno(binderfs_mntpt);
>  		ksft_exit_fail_msg("%s - Failed to open my-binder device\n",
>  				   strerror(errno));
>  	}
> @@ -206,7 +204,7 @@ static int __do_binderfs_test(void)
>  	close(fd);
>  	errno = saved_errno;
>  	if (ret < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		rmdir_protect_errno(binderfs_mntpt);
>  		ksft_exit_fail_msg(
>  			"%s - Failed to open perform BINDER_VERSION request\n",
>  			strerror(errno));
> @@ -218,9 +216,9 @@ static int __do_binderfs_test(void)
>  	/* binder transaction with binderfs binder device passed */
>  	ksft_inc_pass_cnt();
>  
> -	ret = unlink("/dev/binderfs/my-binder");
> +	ret = unlink(device_path);
>  	if (ret < 0) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		rmdir_protect_errno(binderfs_mntpt);
>  		ksft_exit_fail_msg("%s - Failed to delete binder device\n",
>  				   strerror(errno));
>  	}
> @@ -228,12 +226,13 @@ static int __do_binderfs_test(void)
>  	/* binder device removal passed */
>  	ksft_inc_pass_cnt();
>  
> -	ret = unlink("/dev/binderfs/binder-control");
> +	snprintf(device_path, sizeof(device_path), "%s/binder-control", binderfs_mntpt);
> +	ret = unlink(device_path);
>  	if (!ret) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		rmdir_protect_errno(binderfs_mntpt);
>  		ksft_exit_fail_msg("Managed to delete binder-control device\n");
>  	} else if (errno != EPERM) {
> -		keep ? : rmdir_protect_errno("/dev/binderfs");
> +		rmdir_protect_errno(binderfs_mntpt);
>  		ksft_exit_fail_msg(
>  			"%s - Failed to delete binder-control device but exited with unexpected error code\n",
>  			strerror(errno));
> @@ -243,8 +242,8 @@ static int __do_binderfs_test(void)
>  	ksft_inc_xfail_cnt();
>  
>  on_error:
> -	ret = umount2("/dev/binderfs", MNT_DETACH);
> -	keep ?: rmdir_protect_errno("/dev/binderfs");
> +	ret = umount2(binderfs_mntpt, MNT_DETACH);
> +	rmdir_protect_errno(binderfs_mntpt);
>  	if (ret < 0)
>  		ksft_exit_fail_msg("%s - Failed to unmount binderfs\n",
>  				   strerror(errno));
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices
  2020-03-13 15:24       ` [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices Christian Brauner
@ 2020-03-13 23:08         ` Kees Cook
  2020-03-16 22:44           ` Hridya Valsaraju
  0 siblings, 1 reply; 29+ messages in thread
From: Kees Cook @ 2020-03-13 23:08 UTC (permalink / raw)
  To: Christian Brauner
  Cc: gregkh, tkjos, linux-kernel, ard.biesheuvel, ardb, arve, hridya,
	joel, john.stultz, kernel-team, linux-kselftest, maco,
	naresh.kamboju, shuah

On Fri, Mar 13, 2020 at 04:24:20PM +0100, Christian Brauner wrote:
> This adds a stress test that should hopefully help us catch regressions
> for [1], [2], and [3].
> 
> [1]: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> [2]: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> [3]: 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> Cc: Kees Cook <keescook@chromium.org>:
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> /* v2 */
> - Kees Cook <keescook@chromium.org>:
>   - Switch to unique mountpoint through mkdtemp().
> ---
>  .../selftests/filesystems/binderfs/Makefile   |   2 +-
>  .../filesystems/binderfs/binderfs_test.c      | 426 ++++++++++++++----
>  2 files changed, 334 insertions(+), 94 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> index 75315d9ba7a9..8af25ae96049 100644
> --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -CFLAGS += -I../../../../../usr/include/
> +CFLAGS += -I../../../../../usr/include/ -pthread
>  TEST_GEN_PROGS := binderfs_test
>  
>  binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> index 818eb49f8125..8a6b507e34a8 100644
> --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -3,15 +3,20 @@
>  #define _GNU_SOURCE
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <pthread.h>
>  #include <sched.h>
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <sys/fsuid.h>
>  #include <sys/ioctl.h>
>  #include <sys/mount.h>
> +#include <sys/socket.h>
>  #include <sys/stat.h>
> +#include <sys/sysinfo.h>
>  #include <sys/types.h>
> +#include <sys/wait.h>
>  #include <unistd.h>
>  #include <linux/android/binder.h>
>  #include <linux/android/binderfs.h>
> @@ -19,100 +24,26 @@
>  #include "../../kselftest.h"
>  #include "../../kselftest_harness.h"
>  
> -static ssize_t write_nointr(int fd, const void *buf, size_t count)
> -{
> -	ssize_t ret;
> -again:
> -	ret = write(fd, buf, count);
> -	if (ret < 0 && errno == EINTR)
> -		goto again;
> -
> -	return ret;
> -}
> -
> -static void write_to_file(const char *filename, const void *buf, size_t count,
> -			  int allowed_errno)
> -{
> -	int fd, saved_errno;
> -	ssize_t ret;
> -
> -	fd = open(filename, O_WRONLY | O_CLOEXEC);
> -	if (fd < 0)
> -		ksft_exit_fail_msg("%s - Failed to open file %s\n",
> -				   strerror(errno), filename);
> +#define DEFAULT_THREADS 4
>  
> -	ret = write_nointr(fd, buf, count);
> -	if (ret < 0) {
> -		if (allowed_errno && (errno == allowed_errno)) {
> -			close(fd);
> -			return;
> -		}
> +#define PTR_TO_INT(p) ((int)((intptr_t)(p)))
> +#define INT_TO_PTR(u) ((void *)((intptr_t)(u)))
>  
> -		goto on_error;
> +#define close_prot_errno_disarm(fd) \
> +	if (fd >= 0) {              \
> +		int _e_ = errno;    \
> +		close(fd);          \
> +		errno = _e_;        \
> +		fd = -EBADF;        \
>  	}
>  
> -	if ((size_t)ret != count)
> -		goto on_error;
> -
> -	close(fd);
> -	return;
> -
> -on_error:
> -	saved_errno = errno;
> -	close(fd);
> -	errno = saved_errno;
> -
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to write to file %s\n",
> -				   strerror(errno), filename);
> -
> -	ksft_exit_fail_msg("Failed to write to file %s\n", filename);
> -}
> -
> -static void change_to_userns(void)
> -{
> -	int ret;
> -	uid_t uid;
> -	gid_t gid;
> -	/* {g,u}id_map files only allow a max of 4096 bytes written to them */
> -	char idmap[4096];
> -
> -	uid = getuid();
> -	gid = getgid();
> -
> -	ret = unshare(CLONE_NEWUSER);
> -	if (ret < 0)
> -		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
> -				   strerror(errno));
> -
> -	write_to_file("/proc/self/setgroups", "deny", strlen("deny"), ENOENT);
> -
> -	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", uid);
> -	if (ret < 0 || (size_t)ret >= sizeof(idmap))
> -		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
> -				   strerror(errno));
> -
> -	write_to_file("/proc/self/uid_map", idmap, strlen(idmap), 0);
> -
> -	ret = snprintf(idmap, sizeof(idmap), "0 %d 1", gid);
> -	if (ret < 0 || (size_t)ret >= sizeof(idmap))
> -		ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
> -				   strerror(errno));
> -
> -	write_to_file("/proc/self/gid_map", idmap, strlen(idmap), 0);
> -
> -	ret = setgid(0);
> -	if (ret)
> -		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
> -				   strerror(errno));
> +#define log_exit(format, ...)                                                  \
> +	({                                                                     \
> +		fprintf(stderr, format "\n", ##__VA_ARGS__);                   \
> +		exit(EXIT_FAILURE);                                            \
> +	})
>  
> -	ret = setuid(0);
> -	if (ret)
> -		ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
> -				   strerror(errno));
> -}
> -
> -static void change_to_mountns(void)
> +static void change_mountns(void)
>  {
>  	int ret;
>  
> @@ -144,7 +75,7 @@ static int __do_binderfs_test(void)
>  	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
>  		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
>  
> -	change_to_mountns();
> +	change_mountns();
>  
>  	if (!mkdtemp(binderfs_mntpt))
>  		ksft_exit_fail_msg(
> @@ -253,6 +184,288 @@ static int __do_binderfs_test(void)
>  	return 0;
>  }
>  
> +static int wait_for_pid(pid_t pid)
> +{
> +	int status, ret;
> +
> +again:
> +	ret = waitpid(pid, &status, 0);
> +	if (ret == -1) {
> +		if (errno == EINTR)
> +			goto again;
> +
> +		return -1;
> +	}
> +
> +	if (!WIFEXITED(status))
> +		return -1;
> +
> +	return WEXITSTATUS(status);
> +}
> +
> +static int setid_userns_root(void)
> +{
> +	if (setuid(0))
> +		return -1;
> +	if (setgid(0))
> +		return -1;
> +
> +	setfsuid(0);
> +	setfsgid(0);
> +
> +	return 0;
> +}
> +
> +enum idmap_type {
> +	UID_MAP,
> +	GID_MAP,
> +};
> +
> +static ssize_t read_nointr(int fd, void *buf, size_t count)
> +{
> +	ssize_t ret;
> +again:
> +	ret = read(fd, buf, count);
> +	if (ret < 0 && errno == EINTR)
> +		goto again;
> +
> +	return ret;
> +}
> +
> +static ssize_t write_nointr(int fd, const void *buf, size_t count)
> +{
> +	ssize_t ret;
> +again:
> +	ret = write(fd, buf, count);
> +	if (ret < 0 && errno == EINTR)
> +		goto again;
> +
> +	return ret;
> +}
> +
> +static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
> +			    size_t buf_size)
> +{
> +	int fd;
> +	int ret;
> +	char path[4096];
> +
> +	if (type == GID_MAP) {
> +		int setgroups_fd;
> +
> +		snprintf(path, sizeof(path), "/proc/%d/setgroups", pid);
> +		setgroups_fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
> +		if (setgroups_fd < 0 && errno != ENOENT)
> +			return -1;
> +
> +		if (setgroups_fd >= 0) {
> +			ret = write_nointr(setgroups_fd, "deny", sizeof("deny") - 1);
> +			close_prot_errno_disarm(setgroups_fd);
> +			if (ret != sizeof("deny") - 1)
> +				return -1;
> +		}
> +	}
> +
> +	switch (type) {
> +	case UID_MAP:
> +		ret = snprintf(path, sizeof(path), "/proc/%d/uid_map", pid);
> +		break;
> +	case GID_MAP:
> +		ret = snprintf(path, sizeof(path), "/proc/%d/gid_map", pid);
> +		break;
> +	default:
> +		return -1;
> +	}
> +	if (ret < 0 || ret >= sizeof(path))
> +		return -E2BIG;
> +
> +	fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
> +	if (fd < 0)
> +		return -1;
> +
> +	ret = write_nointr(fd, buf, buf_size);
> +	close_prot_errno_disarm(fd);
> +	if (ret != buf_size)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static void change_userns(int syncfds[2])
> +{
> +	int ret;
> +	char buf;
> +
> +	close_prot_errno_disarm(syncfds[1]);
> +
> +	ret = unshare(CLONE_NEWUSER);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
> +				   strerror(errno));
> +
> +	ret = write_nointr(syncfds[0], "1", 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("write_nointr() failed\n");
> +
> +	ret = read_nointr(syncfds[0], &buf, 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("read_nointr() failed\n");
> +
> +	close_prot_errno_disarm(syncfds[0]);
> +
> +	if (setid_userns_root())
> +		ksft_exit_fail_msg("setid_userns_root() failed");
> +}
> +
> +static void change_idmaps(int syncfds[2], pid_t pid)
> +{
> +	int ret;
> +	char buf;
> +	char id_map[4096];
> +
> +	close_prot_errno_disarm(syncfds[0]);
> +
> +	ret = read_nointr(syncfds[1], &buf, 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("read_nointr() failed\n");
> +
> +	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
> +	ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
> +	if (ret)
> +		ksft_exit_fail_msg("write_id_mapping(UID_MAP) failed");
> +
> +	snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
> +	ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
> +	if (ret)
> +		ksft_exit_fail_msg("write_id_mapping(GID_MAP) failed");
> +
> +	ret = write_nointr(syncfds[1], "1", 1);
> +	if (ret != 1)
> +		ksft_exit_fail_msg("write_nointr() failed");
> +
> +	close_prot_errno_disarm(syncfds[1]);
> +}
> +
> +static void *binder_version_thread(void *data)
> +{
> +	int fd = PTR_TO_INT(data);
> +	struct binder_version version = { 0 };
> +	int ret;
> +
> +	ret = ioctl(fd, BINDER_VERSION, &version);
> +	if (ret < 0)
> +		ksft_print_msg("%s - Failed to open perform BINDER_VERSION request\n", strerror(errno));
> +
> +	pthread_exit(data);
> +}
> +
> +/*
> + * Regression test:
> + * 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> + * f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> + * 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> + */
> +TEST(binderfs_stress)
> +{
> +	int fds[1000];
> +	int syncfds[2];
> +	pid_t pid;
> +	int fd, ret;
> +	size_t len;
> +	struct binderfs_device device = { 0 };
> +	char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
> +		device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
> +
> +	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		close_prot_errno_disarm(syncfds[0]);
> +		close_prot_errno_disarm(syncfds[1]);
> +		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
> +	}
> +
> +	if (pid == 0) {
> +		int i, j, k, nthreads;
> +		pthread_attr_t attr;
> +		pthread_t threads[DEFAULT_THREADS];
> +		change_userns(syncfds);
> +		change_mountns();
> +
> +		if (!mkdtemp(binderfs_mntpt))
> +			log_exit("%s - Failed to create binderfs mountpoint\n",
> +				 strerror(errno));
> +
> +		ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
> +		if (ret < 0)
> +			log_exit("%s - Failed to mount binderfs\n", strerror(errno));
> +
> +		for (int i = 0; i < ARRAY_SIZE(fds); i++) {
> +
> +			snprintf(device_path, sizeof(device_path),
> +				 "%s/binder-control", binderfs_mntpt);
> +			fd = open(device_path, O_RDONLY | O_CLOEXEC);
> +			if (fd < 0)
> +				log_exit("%s - Failed to open binder-control device\n", strerror(errno));
> +
> +			memset(&device, 0, sizeof(device));
> +			snprintf(device.name, sizeof(device.name), "%d", i);
> +			ret = ioctl(fd, BINDER_CTL_ADD, &device);
> +			close_prot_errno_disarm(fd);
> +			if (ret < 0)
> +				log_exit("%s - Failed to allocate new binder device\n", strerror(errno));
> +
> +			snprintf(device_path, sizeof(device_path), "%s/%d",
> +				 binderfs_mntpt, i);
> +			fds[i] = open(device_path, O_RDONLY | O_CLOEXEC);
> +			if (fds[i] < 0)
> +				log_exit("%s - Failed to open binder device\n", strerror(errno));
> +		}
> +
> +		ret = umount2(binderfs_mntpt, MNT_DETACH);
> +		rmdir_protect_errno(binderfs_mntpt);
> +		if (ret < 0)
> +			log_exit("%s - Failed to unmount binderfs\n", strerror(errno));
> +
> +		nthreads = get_nprocs_conf();
> +		if (nthreads > DEFAULT_THREADS)
> +			nthreads = DEFAULT_THREADS;
> +
> +		pthread_attr_init(&attr);
> +		for (k = 0; k < ARRAY_SIZE(fds); k++) {
> +			for (i = 0; i < nthreads; i++) {
> +				ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
> +				if (ret) {
> +					ksft_print_msg("%s - Failed to create thread %d\n", strerror(errno), i);
> +					break;
> +				}
> +			}
> +
> +			for (j = 0; j < i; j++) {
> +				void *fdptr = NULL;
> +
> +				ret = pthread_join(threads[j], &fdptr);
> +				if (ret)
> +					ksft_print_msg("%s - Failed to join thread %d for fd %d\n", strerror(errno), j, PTR_TO_INT(fdptr));
> +			}
> +		}
> +		pthread_attr_destroy(&attr);
> +
> +		for (k = 0; k < ARRAY_SIZE(fds); k++)
> +			close(fds[k]);
> +
> +		exit(EXIT_SUCCESS);
> +	}
> +
> +	change_idmaps(syncfds, pid);
> +
> +	ret = wait_for_pid(pid);
> +	if (ret)
> +		ksft_exit_fail_msg("wait_for_pid() failed");
> +}
> +
>  TEST(binderfs_test_privileged)
>  {
>  	if (geteuid() != 0)
> @@ -264,10 +477,37 @@ TEST(binderfs_test_privileged)
>  
>  TEST(binderfs_test_unprivileged)
>  {
> -	change_to_userns();
> +	int ret;
> +	int syncfds[2];
> +	pid_t pid;
>  
> -	if (__do_binderfs_test() == 1)
> -		XFAIL(return, "The Android binderfs filesystem is not available");
> +	ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> +	if (ret < 0)
> +		ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> +
> +	pid = fork();
> +	if (pid < 0) {
> +		close_prot_errno_disarm(syncfds[0]);
> +		close_prot_errno_disarm(syncfds[1]);
> +		ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
> +	}
> +
> +	if (pid == 0) {
> +		change_userns(syncfds);
> +		if (__do_binderfs_test() == 1)
> +			exit(2);
> +		exit(EXIT_SUCCESS);
> +	}
> +
> +	change_idmaps(syncfds, pid);
> +
> +	ret = wait_for_pid(pid);
> +	if (ret) {
> +		if (ret == 2)
> +			XFAIL(return, "The Android binderfs filesystem is not available");
> +		else
> +			ksft_exit_fail_msg("wait_for_pid() failed");
> +	}
>  }
>  
>  TEST_HARNESS_MAIN
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH v2] binderfs: port to new mount api
  2020-03-13 15:34     ` [PATCH v2] binderfs: port to new mount api Christian Brauner
@ 2020-03-13 23:08       ` Kees Cook
  2020-03-18 12:29       ` Greg KH
  1 sibling, 0 replies; 29+ messages in thread
From: Kees Cook @ 2020-03-13 23:08 UTC (permalink / raw)
  To: Christian Brauner
  Cc: gregkh, tkjos, linux-kernel, ard.biesheuvel, ardb, arve, hridya,
	joel, john.stultz, kernel-team, linux-kselftest, maco,
	naresh.kamboju, shuah, Todd Kjos

On Fri, Mar 13, 2020 at 04:34:27PM +0100, Christian Brauner wrote:
> When I first wrote binderfs the new mount api had not yet landed. Now
> that it has been around for a little while and a bunch of filesystems
> have already been ported we should do so too. When Al sent his
> mount-api-conversion pr he requested that binderfs (and a few others) be
> ported separately. It's time we port binderfs. We can make use of the
> new option parser, get nicer infrastructure and it will be easier if we
> ever add any new mount options.
> 
> This survives testing with the binderfs selftests:
> 
> for i in `seq 1 1000`; do ./binderfs_test; done
> 
> including the new stress tests I sent out for review today:
> 
>  TAP version 13
>  1..1
>  # selftests: filesystems/binderfs: binderfs_test
>  # [==========] Running 3 tests from 1 test cases.
>  # [ RUN      ] global.binderfs_stress
>  # [  XFAIL!  ] Tests are not run as root. Skipping privileged tests
>  # [==========] Running 3 tests from 1 test cases.
>  # [ RUN      ] global.binderfs_stress
>  # [       OK ] global.binderfs_stress
>  # [ RUN      ] global.binderfs_test_privileged
>  # [       OK ] global.binderfs_test_privileged
>  # [ RUN      ] global.binderfs_test_unprivileged
>  # # Allocated new binder device with major 243, minor 4, and name my-binder
>  # # Detected binder version: 8
>  # [==========] Running 3 tests from 1 test cases.
>  # [ RUN      ] global.binderfs_stress
>  # [       OK ] global.binderfs_stress
>  # [ RUN      ] global.binderfs_test_privileged
>  # [       OK ] global.binderfs_test_privileged
>  # [ RUN      ] global.binderfs_test_unprivileged
>  # [       OK ] global.binderfs_test_unprivileged
>  # [==========] 3 / 3 tests passed.
>  # [  PASSED  ]
>  ok 1 selftests: filesystems/binderfs: binderfs_test
> 
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> /* v2 */
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - Commit message adapted to new stresstest output after porting to
>     XFAIL infrastructure.
>     For the stresstest patchset see:
>     https://lore.kernel.org/r/20200313152420.138777-1-christian.brauner@ubuntu.com
> ---
>  drivers/android/binderfs.c | 200 +++++++++++++++++++------------------
>  1 file changed, 104 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index f303106b3362..9ecad74183a3 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -18,7 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/mount.h>
> -#include <linux/parser.h>
> +#include <linux/fs_parser.h>
>  #include <linux/radix-tree.h>
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
> @@ -48,26 +48,30 @@ static dev_t binderfs_dev;
>  static DEFINE_MUTEX(binderfs_minors_mutex);
>  static DEFINE_IDA(binderfs_minors);
>  
> -enum {
> +enum binderfs_param {
>  	Opt_max,
>  	Opt_stats_mode,
> -	Opt_err
>  };
>  
>  enum binderfs_stats_mode {
> -	STATS_NONE,
> -	STATS_GLOBAL,
> +	binderfs_stats_mode_unset,
> +	binderfs_stats_mode_global,
>  };
>  
> -static const match_table_t tokens = {
> -	{ Opt_max, "max=%d" },
> -	{ Opt_stats_mode, "stats=%s" },
> -	{ Opt_err, NULL     }
> +static const struct constant_table binderfs_param_stats[] = {
> +	{ "global", binderfs_stats_mode_global },
> +	{}
>  };
>  
> -static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> +const struct fs_parameter_spec binderfs_fs_parameters[] = {
> +	fsparam_u32("max",	Opt_max),
> +	fsparam_enum("stats",	Opt_stats_mode, binderfs_param_stats),
> +	{}
> +};
> +
> +static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
>  {
> -	return inode->i_sb->s_fs_info;
> +	return sb->s_fs_info;
>  }
>  
>  bool is_binderfs_device(const struct inode *inode)
> @@ -246,7 +250,7 @@ static long binder_ctl_ioctl(struct file *file, unsigned int cmd,
>  static void binderfs_evict_inode(struct inode *inode)
>  {
>  	struct binder_device *device = inode->i_private;
> -	struct binderfs_info *info = BINDERFS_I(inode);
> +	struct binderfs_info *info = BINDERFS_SB(inode->i_sb);
>  
>  	clear_inode(inode);
>  
> @@ -264,97 +268,84 @@ static void binderfs_evict_inode(struct inode *inode)
>  	}
>  }
>  
> -/**
> - * binderfs_parse_mount_opts - parse binderfs mount options
> - * @data: options to set (can be NULL in which case defaults are used)
> - */
> -static int binderfs_parse_mount_opts(char *data,
> -				     struct binderfs_mount_opts *opts)
> +static int binderfs_fs_context_parse_param(struct fs_context *fc,
> +					   struct fs_parameter *param)
>  {
> -	char *p, *stats;
> -	opts->max = BINDERFS_MAX_MINOR;
> -	opts->stats_mode = STATS_NONE;
> -
> -	while ((p = strsep(&data, ",")) != NULL) {
> -		substring_t args[MAX_OPT_ARGS];
> -		int token;
> -		int max_devices;
> -
> -		if (!*p)
> -			continue;
> -
> -		token = match_token(p, tokens, args);
> -		switch (token) {
> -		case Opt_max:
> -			if (match_int(&args[0], &max_devices) ||
> -			    (max_devices < 0 ||
> -			     (max_devices > BINDERFS_MAX_MINOR)))
> -				return -EINVAL;
> -
> -			opts->max = max_devices;
> -			break;
> -		case Opt_stats_mode:
> -			if (!capable(CAP_SYS_ADMIN))
> -				return -EINVAL;
> +	int opt;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
> +	struct fs_parse_result result;
>  
> -			stats = match_strdup(&args[0]);
> -			if (!stats)
> -				return -ENOMEM;
> +	opt = fs_parse(fc, binderfs_fs_parameters, param, &result);
> +	if (opt < 0)
> +		return opt;
>  
> -			if (strcmp(stats, "global") != 0) {
> -				kfree(stats);
> -				return -EINVAL;
> -			}
> +	switch (opt) {
> +	case Opt_max:
> +		if (result.uint_32 > BINDERFS_MAX_MINOR)
> +			return invalfc(fc, "Bad value for '%s'", param->key);
>  
> -			opts->stats_mode = STATS_GLOBAL;
> -			kfree(stats);
> -			break;
> -		default:
> -			pr_err("Invalid mount options\n");
> -			return -EINVAL;
> -		}
> +		ctx->max = result.uint_32;
> +		break;
> +	case Opt_stats_mode:
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
> +
> +		ctx->stats_mode = result.uint_32;
> +		break;
> +	default:
> +		return invalfc(fc, "Unsupported parameter '%s'", param->key);
>  	}
>  
>  	return 0;
>  }
>  
> -static int binderfs_remount(struct super_block *sb, int *flags, char *data)
> +static int binderfs_fs_context_reconfigure(struct fs_context *fc)
>  {
> -	int prev_stats_mode, ret;
> -	struct binderfs_info *info = sb->s_fs_info;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
> +	struct binderfs_info *info = BINDERFS_SB(fc->root->d_sb);
>  
> -	prev_stats_mode = info->mount_opts.stats_mode;
> -	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> -	if (ret)
> -		return ret;
> -
> -	if (prev_stats_mode != info->mount_opts.stats_mode) {
> -		pr_err("Binderfs stats mode cannot be changed during a remount\n");
> -		info->mount_opts.stats_mode = prev_stats_mode;
> -		return -EINVAL;
> -	}
> +	if (info->mount_opts.stats_mode != ctx->stats_mode)
> +		return invalfc(fc, "Binderfs stats mode cannot be changed during a remount");
>  
> +	info->mount_opts.stats_mode = ctx->stats_mode;
> +	info->mount_opts.max = ctx->max;
>  	return 0;
>  }
>  
> -static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
> +static int binderfs_show_options(struct seq_file *seq, struct dentry *root)
>  {
> -	struct binderfs_info *info;
> +	struct binderfs_info *info = BINDERFS_SB(root->d_sb);
>  
> -	info = root->d_sb->s_fs_info;
>  	if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
>  		seq_printf(seq, ",max=%d", info->mount_opts.max);
> -	if (info->mount_opts.stats_mode == STATS_GLOBAL)
> +
> +	switch (info->mount_opts.stats_mode) {
> +	case binderfs_stats_mode_unset:
> +		break;
> +	case binderfs_stats_mode_global:
>  		seq_printf(seq, ",stats=global");
> +		break;
> +	}
>  
>  	return 0;
>  }
>  
> +static void binderfs_put_super(struct super_block *sb)
> +{
> +	struct binderfs_info *info = sb->s_fs_info;
> +
> +	if (info && info->ipc_ns)
> +		put_ipc_ns(info->ipc_ns);
> +
> +	kfree(info);
> +	sb->s_fs_info = NULL;
> +}
> +
>  static const struct super_operations binderfs_super_ops = {
>  	.evict_inode    = binderfs_evict_inode,
> -	.remount_fs	= binderfs_remount,
> -	.show_options	= binderfs_show_mount_opts,
> +	.show_options	= binderfs_show_options,
>  	.statfs         = simple_statfs,
> +	.put_super	= binderfs_put_super,
>  };
>  
>  static inline bool is_binderfs_control_device(const struct dentry *dentry)
> @@ -653,10 +644,11 @@ static int init_binder_logs(struct super_block *sb)
>  	return ret;
>  }
>  
> -static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> +static int binderfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	int ret;
>  	struct binderfs_info *info;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
>  	struct inode *inode = NULL;
>  	struct binderfs_device device_info = { 0 };
>  	const char *name;
> @@ -689,16 +681,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  
>  	info->ipc_ns = get_ipc_ns(current->nsproxy->ipc_ns);
>  
> -	ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> -	if (ret)
> -		return ret;
> -
>  	info->root_gid = make_kgid(sb->s_user_ns, 0);
>  	if (!gid_valid(info->root_gid))
>  		info->root_gid = GLOBAL_ROOT_GID;
>  	info->root_uid = make_kuid(sb->s_user_ns, 0);
>  	if (!uid_valid(info->root_uid))
>  		info->root_uid = GLOBAL_ROOT_UID;
> +	info->mount_opts.max = ctx->max;
> +	info->mount_opts.stats_mode = ctx->stats_mode;
>  
>  	inode = new_inode(sb);
>  	if (!inode)
> @@ -730,36 +720,54 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  			name++;
>  	}
>  
> -	if (info->mount_opts.stats_mode == STATS_GLOBAL)
> +	if (info->mount_opts.stats_mode == binderfs_stats_mode_global)
>  		return init_binder_logs(sb);
>  
>  	return 0;
>  }
>  
> -static struct dentry *binderfs_mount(struct file_system_type *fs_type,
> -				     int flags, const char *dev_name,
> -				     void *data)
> +static int binderfs_fs_context_get_tree(struct fs_context *fc)
>  {
> -	return mount_nodev(fs_type, flags, data, binderfs_fill_super);
> +	return get_tree_nodev(fc, binderfs_fill_super);
>  }
>  
> -static void binderfs_kill_super(struct super_block *sb)
> +static void binderfs_fs_context_free(struct fs_context *fc)
>  {
> -	struct binderfs_info *info = sb->s_fs_info;
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
>  
> -	kill_litter_super(sb);
> +	kfree(ctx);
> +}
>  
> -	if (info && info->ipc_ns)
> -		put_ipc_ns(info->ipc_ns);
> +static const struct fs_context_operations binderfs_fs_context_ops = {
> +	.free		= binderfs_fs_context_free,
> +	.get_tree	= binderfs_fs_context_get_tree,
> +	.parse_param	= binderfs_fs_context_parse_param,
> +	.reconfigure	= binderfs_fs_context_reconfigure,
> +};
>  
> -	kfree(info);
> +static int binderfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct binderfs_mount_opts *ctx = fc->fs_private;
> +
> +	ctx = kzalloc(sizeof(struct binderfs_mount_opts), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->max = BINDERFS_MAX_MINOR;
> +	ctx->stats_mode = binderfs_stats_mode_unset;
> +
> +	fc->fs_private = ctx;
> +	fc->ops = &binderfs_fs_context_ops;
> +
> +	return 0;
>  }
>  
>  static struct file_system_type binder_fs_type = {
> -	.name		= "binder",
> -	.mount		= binderfs_mount,
> -	.kill_sb	= binderfs_kill_super,
> -	.fs_flags	= FS_USERNS_MOUNT,
> +	.name			= "binder",
> +	.init_fs_context	= binderfs_init_fs_context,
> +	.parameters		= binderfs_fs_parameters,
> +	.kill_sb		= kill_litter_super,
> +	.fs_flags		= FS_USERNS_MOUNT,
>  };
>  
>  int __init init_binderfs(void)
> 
> base-commit: f17f06a0c7794d3a7c2425663738823354447472
> -- 
> 2.25.1
> 

-- 
Kees Cook

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

* Re: [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices
  2020-03-13 23:08         ` Kees Cook
@ 2020-03-16 22:44           ` Hridya Valsaraju
  2020-03-17  8:27             ` Christian Brauner
  0 siblings, 1 reply; 29+ messages in thread
From: Hridya Valsaraju @ 2020-03-16 22:44 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christian Brauner, Greg Kroah-Hartman, Todd Kjos, LKML,
	ard.biesheuvel, ardb, Arve Hjønnevåg, Joel Fernandes,
	John Stultz, Android Kernel Team, linux-kselftest,
	Martijn Coenen, naresh.kamboju, shuah

On Fri, Mar 13, 2020 at 4:08 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Mar 13, 2020 at 04:24:20PM +0100, Christian Brauner wrote:
> > This adds a stress test that should hopefully help us catch regressions
> > for [1], [2], and [3].
> >
> > [1]: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> > [2]: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> > [3]: 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> > Cc: Kees Cook <keescook@chromium.org>:
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>

Thank you for creating these tests Christian! I was able to get them
running on the cuttlefish platform with the ACK android-mainline
branch.

Regards,
Hridya

> > ---
> > /* v2 */
> > - Kees Cook <keescook@chromium.org>:
> >   - Switch to unique mountpoint through mkdtemp().
> > ---
> >  .../selftests/filesystems/binderfs/Makefile   |   2 +-
> >  .../filesystems/binderfs/binderfs_test.c      | 426 ++++++++++++++----
> >  2 files changed, 334 insertions(+), 94 deletions(-)
> >
> > diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile b/tools/testing/selftests/filesystems/binderfs/Makefile
> > index 75315d9ba7a9..8af25ae96049 100644
> > --- a/tools/testing/selftests/filesystems/binderfs/Makefile
> > +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> > @@ -1,6 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >
> > -CFLAGS += -I../../../../../usr/include/
> > +CFLAGS += -I../../../../../usr/include/ -pthread
> >  TEST_GEN_PROGS := binderfs_test
> >
> >  binderfs_test: binderfs_test.c ../../kselftest.h ../../kselftest_harness.h
> > diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > index 818eb49f8125..8a6b507e34a8 100644
> > --- a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > @@ -3,15 +3,20 @@
> >  #define _GNU_SOURCE
> >  #include <errno.h>
> >  #include <fcntl.h>
> > +#include <pthread.h>
> >  #include <sched.h>
> >  #include <stdbool.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > +#include <sys/fsuid.h>
> >  #include <sys/ioctl.h>
> >  #include <sys/mount.h>
> > +#include <sys/socket.h>
> >  #include <sys/stat.h>
> > +#include <sys/sysinfo.h>
> >  #include <sys/types.h>
> > +#include <sys/wait.h>
> >  #include <unistd.h>
> >  #include <linux/android/binder.h>
> >  #include <linux/android/binderfs.h>
> > @@ -19,100 +24,26 @@
> >  #include "../../kselftest.h"
> >  #include "../../kselftest_harness.h"
> >
> > -static ssize_t write_nointr(int fd, const void *buf, size_t count)
> > -{
> > -     ssize_t ret;
> > -again:
> > -     ret = write(fd, buf, count);
> > -     if (ret < 0 && errno == EINTR)
> > -             goto again;
> > -
> > -     return ret;
> > -}
> > -
> > -static void write_to_file(const char *filename, const void *buf, size_t count,
> > -                       int allowed_errno)
> > -{
> > -     int fd, saved_errno;
> > -     ssize_t ret;
> > -
> > -     fd = open(filename, O_WRONLY | O_CLOEXEC);
> > -     if (fd < 0)
> > -             ksft_exit_fail_msg("%s - Failed to open file %s\n",
> > -                                strerror(errno), filename);
> > +#define DEFAULT_THREADS 4
> >
> > -     ret = write_nointr(fd, buf, count);
> > -     if (ret < 0) {
> > -             if (allowed_errno && (errno == allowed_errno)) {
> > -                     close(fd);
> > -                     return;
> > -             }
> > +#define PTR_TO_INT(p) ((int)((intptr_t)(p)))
> > +#define INT_TO_PTR(u) ((void *)((intptr_t)(u)))
> >
> > -             goto on_error;
> > +#define close_prot_errno_disarm(fd) \
> > +     if (fd >= 0) {              \
> > +             int _e_ = errno;    \
> > +             close(fd);          \
> > +             errno = _e_;        \
> > +             fd = -EBADF;        \
> >       }
> >
> > -     if ((size_t)ret != count)
> > -             goto on_error;
> > -
> > -     close(fd);
> > -     return;
> > -
> > -on_error:
> > -     saved_errno = errno;
> > -     close(fd);
> > -     errno = saved_errno;
> > -
> > -     if (ret < 0)
> > -             ksft_exit_fail_msg("%s - Failed to write to file %s\n",
> > -                                strerror(errno), filename);
> > -
> > -     ksft_exit_fail_msg("Failed to write to file %s\n", filename);
> > -}
> > -
> > -static void change_to_userns(void)
> > -{
> > -     int ret;
> > -     uid_t uid;
> > -     gid_t gid;
> > -     /* {g,u}id_map files only allow a max of 4096 bytes written to them */
> > -     char idmap[4096];
> > -
> > -     uid = getuid();
> > -     gid = getgid();
> > -
> > -     ret = unshare(CLONE_NEWUSER);
> > -     if (ret < 0)
> > -             ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
> > -                                strerror(errno));
> > -
> > -     write_to_file("/proc/self/setgroups", "deny", strlen("deny"), ENOENT);
> > -
> > -     ret = snprintf(idmap, sizeof(idmap), "0 %d 1", uid);
> > -     if (ret < 0 || (size_t)ret >= sizeof(idmap))
> > -             ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
> > -                                strerror(errno));
> > -
> > -     write_to_file("/proc/self/uid_map", idmap, strlen(idmap), 0);
> > -
> > -     ret = snprintf(idmap, sizeof(idmap), "0 %d 1", gid);
> > -     if (ret < 0 || (size_t)ret >= sizeof(idmap))
> > -             ksft_exit_fail_msg("%s - Failed to prepare uid mapping\n",
> > -                                strerror(errno));
> > -
> > -     write_to_file("/proc/self/gid_map", idmap, strlen(idmap), 0);
> > -
> > -     ret = setgid(0);
> > -     if (ret)
> > -             ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
> > -                                strerror(errno));
> > +#define log_exit(format, ...)                                                  \
> > +     ({                                                                     \
> > +             fprintf(stderr, format "\n", ##__VA_ARGS__);                   \
> > +             exit(EXIT_FAILURE);                                            \
> > +     })
> >
> > -     ret = setuid(0);
> > -     if (ret)
> > -             ksft_exit_fail_msg("%s - Failed to setgid(0)\n",
> > -                                strerror(errno));
> > -}
> > -
> > -static void change_to_mountns(void)
> > +static void change_mountns(void)
> >  {
> >       int ret;
> >
> > @@ -144,7 +75,7 @@ static int __do_binderfs_test(void)
> >       char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
> >               device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
> >
> > -     change_to_mountns();
> > +     change_mountns();
> >
> >       if (!mkdtemp(binderfs_mntpt))
> >               ksft_exit_fail_msg(
> > @@ -253,6 +184,288 @@ static int __do_binderfs_test(void)
> >       return 0;
> >  }
> >
> > +static int wait_for_pid(pid_t pid)
> > +{
> > +     int status, ret;
> > +
> > +again:
> > +     ret = waitpid(pid, &status, 0);
> > +     if (ret == -1) {
> > +             if (errno == EINTR)
> > +                     goto again;
> > +
> > +             return -1;
> > +     }
> > +
> > +     if (!WIFEXITED(status))
> > +             return -1;
> > +
> > +     return WEXITSTATUS(status);
> > +}
> > +
> > +static int setid_userns_root(void)
> > +{
> > +     if (setuid(0))
> > +             return -1;
> > +     if (setgid(0))
> > +             return -1;
> > +
> > +     setfsuid(0);
> > +     setfsgid(0);
> > +
> > +     return 0;
> > +}
> > +
> > +enum idmap_type {
> > +     UID_MAP,
> > +     GID_MAP,
> > +};
> > +
> > +static ssize_t read_nointr(int fd, void *buf, size_t count)
> > +{
> > +     ssize_t ret;
> > +again:
> > +     ret = read(fd, buf, count);
> > +     if (ret < 0 && errno == EINTR)
> > +             goto again;
> > +
> > +     return ret;
> > +}
> > +
> > +static ssize_t write_nointr(int fd, const void *buf, size_t count)
> > +{
> > +     ssize_t ret;
> > +again:
> > +     ret = write(fd, buf, count);
> > +     if (ret < 0 && errno == EINTR)
> > +             goto again;
> > +
> > +     return ret;
> > +}
> > +
> > +static int write_id_mapping(enum idmap_type type, pid_t pid, const char *buf,
> > +                         size_t buf_size)
> > +{
> > +     int fd;
> > +     int ret;
> > +     char path[4096];
> > +
> > +     if (type == GID_MAP) {
> > +             int setgroups_fd;
> > +
> > +             snprintf(path, sizeof(path), "/proc/%d/setgroups", pid);
> > +             setgroups_fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
> > +             if (setgroups_fd < 0 && errno != ENOENT)
> > +                     return -1;
> > +
> > +             if (setgroups_fd >= 0) {
> > +                     ret = write_nointr(setgroups_fd, "deny", sizeof("deny") - 1);
> > +                     close_prot_errno_disarm(setgroups_fd);
> > +                     if (ret != sizeof("deny") - 1)
> > +                             return -1;
> > +             }
> > +     }
> > +
> > +     switch (type) {
> > +     case UID_MAP:
> > +             ret = snprintf(path, sizeof(path), "/proc/%d/uid_map", pid);
> > +             break;
> > +     case GID_MAP:
> > +             ret = snprintf(path, sizeof(path), "/proc/%d/gid_map", pid);
> > +             break;
> > +     default:
> > +             return -1;
> > +     }
> > +     if (ret < 0 || ret >= sizeof(path))
> > +             return -E2BIG;
> > +
> > +     fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW);
> > +     if (fd < 0)
> > +             return -1;
> > +
> > +     ret = write_nointr(fd, buf, buf_size);
> > +     close_prot_errno_disarm(fd);
> > +     if (ret != buf_size)
> > +             return -1;
> > +
> > +     return 0;
> > +}
> > +
> > +static void change_userns(int syncfds[2])
> > +{
> > +     int ret;
> > +     char buf;
> > +
> > +     close_prot_errno_disarm(syncfds[1]);
> > +
> > +     ret = unshare(CLONE_NEWUSER);
> > +     if (ret < 0)
> > +             ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
> > +                                strerror(errno));
> > +
> > +     ret = write_nointr(syncfds[0], "1", 1);
> > +     if (ret != 1)
> > +             ksft_exit_fail_msg("write_nointr() failed\n");
> > +
> > +     ret = read_nointr(syncfds[0], &buf, 1);
> > +     if (ret != 1)
> > +             ksft_exit_fail_msg("read_nointr() failed\n");
> > +
> > +     close_prot_errno_disarm(syncfds[0]);
> > +
> > +     if (setid_userns_root())
> > +             ksft_exit_fail_msg("setid_userns_root() failed");
> > +}
> > +
> > +static void change_idmaps(int syncfds[2], pid_t pid)
> > +{
> > +     int ret;
> > +     char buf;
> > +     char id_map[4096];
> > +
> > +     close_prot_errno_disarm(syncfds[0]);
> > +
> > +     ret = read_nointr(syncfds[1], &buf, 1);
> > +     if (ret != 1)
> > +             ksft_exit_fail_msg("read_nointr() failed\n");
> > +
> > +     snprintf(id_map, sizeof(id_map), "0 %d 1\n", getuid());
> > +     ret = write_id_mapping(UID_MAP, pid, id_map, strlen(id_map));
> > +     if (ret)
> > +             ksft_exit_fail_msg("write_id_mapping(UID_MAP) failed");
> > +
> > +     snprintf(id_map, sizeof(id_map), "0 %d 1\n", getgid());
> > +     ret = write_id_mapping(GID_MAP, pid, id_map, strlen(id_map));
> > +     if (ret)
> > +             ksft_exit_fail_msg("write_id_mapping(GID_MAP) failed");
> > +
> > +     ret = write_nointr(syncfds[1], "1", 1);
> > +     if (ret != 1)
> > +             ksft_exit_fail_msg("write_nointr() failed");
> > +
> > +     close_prot_errno_disarm(syncfds[1]);
> > +}
> > +
> > +static void *binder_version_thread(void *data)
> > +{
> > +     int fd = PTR_TO_INT(data);
> > +     struct binder_version version = { 0 };
> > +     int ret;
> > +
> > +     ret = ioctl(fd, BINDER_VERSION, &version);
> > +     if (ret < 0)
> > +             ksft_print_msg("%s - Failed to open perform BINDER_VERSION request\n", strerror(errno));
> > +
> > +     pthread_exit(data);
> > +}
> > +
> > +/*
> > + * Regression test:
> > + * 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> > + * f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> > + * 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> > + */
> > +TEST(binderfs_stress)
> > +{
> > +     int fds[1000];
> > +     int syncfds[2];
> > +     pid_t pid;
> > +     int fd, ret;
> > +     size_t len;
> > +     struct binderfs_device device = { 0 };
> > +     char binderfs_mntpt[] = P_tmpdir "/binderfs_XXXXXX",
> > +             device_path[sizeof(P_tmpdir "/binderfs_XXXXXX/") + BINDERFS_MAX_NAME];
> > +
> > +     ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> > +     if (ret < 0)
> > +             ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> > +
> > +     pid = fork();
> > +     if (pid < 0) {
> > +             close_prot_errno_disarm(syncfds[0]);
> > +             close_prot_errno_disarm(syncfds[1]);
> > +             ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
> > +     }
> > +
> > +     if (pid == 0) {
> > +             int i, j, k, nthreads;
> > +             pthread_attr_t attr;
> > +             pthread_t threads[DEFAULT_THREADS];
> > +             change_userns(syncfds);
> > +             change_mountns();
> > +
> > +             if (!mkdtemp(binderfs_mntpt))
> > +                     log_exit("%s - Failed to create binderfs mountpoint\n",
> > +                              strerror(errno));
> > +
> > +             ret = mount(NULL, binderfs_mntpt, "binder", 0, 0);
> > +             if (ret < 0)
> > +                     log_exit("%s - Failed to mount binderfs\n", strerror(errno));
> > +
> > +             for (int i = 0; i < ARRAY_SIZE(fds); i++) {
> > +
> > +                     snprintf(device_path, sizeof(device_path),
> > +                              "%s/binder-control", binderfs_mntpt);
> > +                     fd = open(device_path, O_RDONLY | O_CLOEXEC);
> > +                     if (fd < 0)
> > +                             log_exit("%s - Failed to open binder-control device\n", strerror(errno));
> > +
> > +                     memset(&device, 0, sizeof(device));
> > +                     snprintf(device.name, sizeof(device.name), "%d", i);
> > +                     ret = ioctl(fd, BINDER_CTL_ADD, &device);
> > +                     close_prot_errno_disarm(fd);
> > +                     if (ret < 0)
> > +                             log_exit("%s - Failed to allocate new binder device\n", strerror(errno));
> > +
> > +                     snprintf(device_path, sizeof(device_path), "%s/%d",
> > +                              binderfs_mntpt, i);
> > +                     fds[i] = open(device_path, O_RDONLY | O_CLOEXEC);
> > +                     if (fds[i] < 0)
> > +                             log_exit("%s - Failed to open binder device\n", strerror(errno));
> > +             }
> > +
> > +             ret = umount2(binderfs_mntpt, MNT_DETACH);
> > +             rmdir_protect_errno(binderfs_mntpt);
> > +             if (ret < 0)
> > +                     log_exit("%s - Failed to unmount binderfs\n", strerror(errno));
> > +
> > +             nthreads = get_nprocs_conf();
> > +             if (nthreads > DEFAULT_THREADS)
> > +                     nthreads = DEFAULT_THREADS;
> > +
> > +             pthread_attr_init(&attr);
> > +             for (k = 0; k < ARRAY_SIZE(fds); k++) {
> > +                     for (i = 0; i < nthreads; i++) {
> > +                             ret = pthread_create(&threads[i], &attr, binder_version_thread, INT_TO_PTR(fds[k]));
> > +                             if (ret) {
> > +                                     ksft_print_msg("%s - Failed to create thread %d\n", strerror(errno), i);
> > +                                     break;
> > +                             }
> > +                     }
> > +
> > +                     for (j = 0; j < i; j++) {
> > +                             void *fdptr = NULL;
> > +
> > +                             ret = pthread_join(threads[j], &fdptr);
> > +                             if (ret)
> > +                                     ksft_print_msg("%s - Failed to join thread %d for fd %d\n", strerror(errno), j, PTR_TO_INT(fdptr));
> > +                     }
> > +             }
> > +             pthread_attr_destroy(&attr);
> > +
> > +             for (k = 0; k < ARRAY_SIZE(fds); k++)
> > +                     close(fds[k]);
> > +
> > +             exit(EXIT_SUCCESS);
> > +     }
> > +
> > +     change_idmaps(syncfds, pid);
> > +
> > +     ret = wait_for_pid(pid);
> > +     if (ret)
> > +             ksft_exit_fail_msg("wait_for_pid() failed");
> > +}
> > +
> >  TEST(binderfs_test_privileged)
> >  {
> >       if (geteuid() != 0)
> > @@ -264,10 +477,37 @@ TEST(binderfs_test_privileged)
> >
> >  TEST(binderfs_test_unprivileged)
> >  {
> > -     change_to_userns();
> > +     int ret;
> > +     int syncfds[2];
> > +     pid_t pid;
> >
> > -     if (__do_binderfs_test() == 1)
> > -             XFAIL(return, "The Android binderfs filesystem is not available");
> > +     ret = socketpair(PF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0, syncfds);
> > +     if (ret < 0)
> > +             ksft_exit_fail_msg("%s - Failed to create socket pair", strerror(errno));
> > +
> > +     pid = fork();
> > +     if (pid < 0) {
> > +             close_prot_errno_disarm(syncfds[0]);
> > +             close_prot_errno_disarm(syncfds[1]);
> > +             ksft_exit_fail_msg("%s - Failed to fork", strerror(errno));
> > +     }
> > +
> > +     if (pid == 0) {
> > +             change_userns(syncfds);
> > +             if (__do_binderfs_test() == 1)
> > +                     exit(2);
> > +             exit(EXIT_SUCCESS);
> > +     }
> > +
> > +     change_idmaps(syncfds, pid);
> > +
> > +     ret = wait_for_pid(pid);
> > +     if (ret) {
> > +             if (ret == 2)
> > +                     XFAIL(return, "The Android binderfs filesystem is not available");
> > +             else
> > +                     ksft_exit_fail_msg("wait_for_pid() failed");
> > +     }
> >  }
> >
> >  TEST_HARNESS_MAIN
> > --
> > 2.25.1
> >
>
> --
> Kees Cook

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

* Re: [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices
  2020-03-16 22:44           ` Hridya Valsaraju
@ 2020-03-17  8:27             ` Christian Brauner
  0 siblings, 0 replies; 29+ messages in thread
From: Christian Brauner @ 2020-03-17  8:27 UTC (permalink / raw)
  To: Hridya Valsaraju
  Cc: Kees Cook, Greg Kroah-Hartman, Todd Kjos, LKML, ard.biesheuvel,
	ardb, Arve Hjønnevåg, Joel Fernandes, John Stultz,
	Android Kernel Team, linux-kselftest, Martijn Coenen,
	naresh.kamboju, shuah

On Mon, Mar 16, 2020 at 03:44:44PM -0700, Hridya Valsaraju wrote:
> On Fri, Mar 13, 2020 at 4:08 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Mar 13, 2020 at 04:24:20PM +0100, Christian Brauner wrote:
> > > This adds a stress test that should hopefully help us catch regressions
> > > for [1], [2], and [3].
> > >
> > > [1]: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
> > > [2]: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
> > > [3]: 211b64e4b5b6 ("binderfs: use refcount for binder control devices too")
> > > Cc: Kees Cook <keescook@chromium.org>:
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> >
> > -Kees
> >
> 
> Thank you for creating these tests Christian! I was able to get them
> running on the cuttlefish platform with the ACK android-mainline
> branch.

Thank you, Hridya!
Christian

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

* Re: [PATCH v2] binderfs: port to new mount api
  2020-03-13 15:34     ` [PATCH v2] binderfs: port to new mount api Christian Brauner
  2020-03-13 23:08       ` Kees Cook
@ 2020-03-18 12:29       ` Greg KH
  1 sibling, 0 replies; 29+ messages in thread
From: Greg KH @ 2020-03-18 12:29 UTC (permalink / raw)
  To: Christian Brauner
  Cc: tkjos, keescook, linux-kernel, ard.biesheuvel, ardb, arve,
	hridya, joel, john.stultz, kernel-team, linux-kselftest, maco,
	naresh.kamboju, shuah, Todd Kjos

On Fri, Mar 13, 2020 at 04:34:27PM +0100, Christian Brauner wrote:
> When I first wrote binderfs the new mount api had not yet landed. Now
> that it has been around for a little while and a bunch of filesystems
> have already been ported we should do so too. When Al sent his
> mount-api-conversion pr he requested that binderfs (and a few others) be
> ported separately. It's time we port binderfs. We can make use of the
> new option parser, get nicer infrastructure and it will be easier if we
> ever add any new mount options.
> 
> This survives testing with the binderfs selftests:
> 
> for i in `seq 1 1000`; do ./binderfs_test; done
> 
> including the new stress tests I sent out for review today:

<snip>

Thanks for these, all now queued up.

greg k-h

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

end of thread, other threads:[~2020-03-18 12:29 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11  7:52 WARNING: at refcount.c:190 refcount_sub_and_test_checked+0xac/0xc8 - refcount_t: underflow; use-after-free Naresh Kamboju
2020-03-11  7:52 ` Naresh Kamboju
2020-03-11  9:13 ` Christian Brauner
2020-03-11  9:13   ` Christian Brauner
2020-03-11 10:53 ` [PATCH] binderfs: use refcount for binder control devices too Christian Brauner
2020-03-11 18:25   ` Todd Kjos
2020-03-12 13:15   ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Christian Brauner
2020-03-12 13:15     ` [PATCH 2/3] binderfs: add stress test for binderfs binder devices Christian Brauner
2020-03-12 23:53       ` Kees Cook
2020-03-13 12:54         ` Christian Brauner
2020-03-12 13:15     ` [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint Christian Brauner
2020-03-12 23:54       ` Kees Cook
2020-03-13 12:55         ` Christian Brauner
2020-03-12 21:24     ` [PATCH] binderfs: port to new mount api Christian Brauner
2020-03-12 23:56       ` Kees Cook
2020-03-13 12:55         ` Christian Brauner
2020-03-13 12:56           ` Christian Brauner
2020-03-12 23:51     ` [PATCH 1/3] binderfs: port tests to test harness infrastructure Kees Cook
2020-03-13 15:24     ` [PATCH v2 " Christian Brauner
2020-03-13 15:24       ` [PATCH v2 2/3] binderfs_test: switch from /dev to a unique per-test mountpoint Christian Brauner
2020-03-13 23:07         ` Kees Cook
2020-03-13 15:24       ` [PATCH v2 3/3] binderfs: add stress test for binderfs binder devices Christian Brauner
2020-03-13 23:08         ` Kees Cook
2020-03-16 22:44           ` Hridya Valsaraju
2020-03-17  8:27             ` Christian Brauner
2020-03-13 23:07       ` [PATCH v2 1/3] binderfs: port tests to test harness infrastructure Kees Cook
2020-03-13 15:34     ` [PATCH v2] binderfs: port to new mount api Christian Brauner
2020-03-13 23:08       ` Kees Cook
2020-03-18 12:29       ` Greg KH

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