| Message ID | 20210927163805.808907-13-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | syfs: generic deadlock fix with module removal | expand |
On Mon, Sep 27, 2021 at 09:38:05AM -0700, Luis Chamberlain wrote: > The ATTRIBUTE_GROUPS is typically used to avoid boiler plate > code which is used in many drivers. Embracing ATTRIBUTE_GROUPS was > long due on the zram driver, however a recent fix for sysfs allows > users of ATTRIBUTE_GROUPS to also associate a module to the group > attribute. Does this mean that other modules using sysfs but _not_ ATTRIBUTE_GROUPS() are still vulnerable to potential use-after-free of the kernfs fops? -Kees > > In zram's case this also means it allows us to fix a race which triggers > a deadlock on the zram driver. This deadlock happens when a sysfs attribute > use a lock also used on module removal. This happens when for instance a > sysfs file on a driver is used, then at the same time we have module > removal call trigger. The module removal call code holds a lock, and then > the sysfs file entry waits for the same lock. While holding the lock the > module removal tries to remove the sysfs entries, but these cannot be > removed yet as one is waiting for a lock. This won't complete as the lock > is already held. Likewise module removal cannot complete, and so we > deadlock. > > Sysfs fixes this when the group attributes have a module associated to > it, sysfs will *try* to get a refcount to the module when a shared > lock is used, prior to mucking with a sysfs attribute. If this fails we > just give up right away. > > This deadlock was first reported with the zram driver, a sketch of how > this can happen follows: > > CPU A CPU B > whatever_store() > module_unload > mutex_lock(foo) > mutex_lock(foo) > del_gendisk(zram->disk); > device_del() > device_remove_groups() > > In this situation whatever_store() is waiting for the mutex foo to > become unlocked, but that won't happen until module removal is complete. > But module removal won't complete until the sysfs file being poked > completes which is waiting for a lock already held. > > This issue can be reproduced easily on the zram driver as follows: > > Loop 1 on one terminal: > > while true; > do modprobe zram; > modprobe -r zram; > done > > Loop 2 on a second terminal: > while true; do > echo 1024 > /sys/block/zram0/disksize; > echo 1 > /sys/block/zram0/reset; > done > > Without this patch we end up in a deadlock, and the following > stack trace is produced which hints to us what the issue was: > > INFO: task bash:888 blocked for more than 120 seconds. > Tainted: G E 5.12.0-rc1-next-20210304+ #4 > "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > task:bash state:D stack: 0 pid: 888 ppid: 887 flags:<etc> > Call Trace: > __schedule+0x2e4/0x900 > schedule+0x46/0xb0 > schedule_preempt_disabled+0xa/0x10 > __mutex_lock.constprop.0+0x2c3/0x490 > ? _kstrtoull+0x35/0xd0 > reset_store+0x6c/0x160 [zram] > kernfs_fop_write_iter+0x124/0x1b0 > new_sync_write+0x11c/0x1b0 > vfs_write+0x1c2/0x260 > ksys_write+0x5f/0xe0 > do_syscall_64+0x33/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > RIP: 0033:0x7f34f2c3df33 > RSP: 002b:00007ffe751df6e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 > RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f34f2c3df33 > RDX: 0000000000000002 RSI: 0000561ccb06ec10 RDI: 0000000000000001 > RBP: 0000561ccb06ec10 R08: 000000000000000a R09: 0000000000000001 > R10: 0000561ccb157590 R11: 0000000000000246 R12: 0000000000000002 > R13: 00007f34f2d0e6a0 R14: 0000000000000002 R15: 00007f34f2d0e8a0 > INFO: task modprobe:1104 can't die for more than 120 seconds. > task:modprobe state:D stack: 0 pid: 1104 ppid: 916 flags:<etc> > Call Trace: > __schedule+0x2e4/0x900 > schedule+0x46/0xb0 > __kernfs_remove.part.0+0x228/0x2b0 > ? finish_wait+0x80/0x80 > kernfs_remove_by_name_ns+0x50/0x90 > remove_files+0x2b/0x60 > sysfs_remove_group+0x38/0x80 > sysfs_remove_groups+0x29/0x40 > device_remove_attrs+0x4a/0x80 > device_del+0x183/0x3e0 > ? mutex_lock+0xe/0x30 > del_gendisk+0x27a/0x2d0 > zram_remove+0x8a/0xb0 [zram] > ? hot_remove_store+0xf0/0xf0 [zram] > zram_remove_cb+0xd/0x10 [zram] > idr_for_each+0x5e/0xd0 > destroy_devices+0x39/0x6f [zram] > __do_sys_delete_module+0x190/0x2a0 > do_syscall_64+0x33/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > RIP: 0033:0x7f32adf727d7 > RSP: 002b:00007ffc08bb38a8 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0 > RAX: ffffffffffffffda RBX: 000055eea23cbb10 RCX: 00007f32adf727d7 > RDX: 0000000000000000 RSI: 0000000000000800 RDI: 000055eea23cbb78 > RBP: 000055eea23cbb10 R08: 0000000000000000 R09: 0000000000000000 > R10: 00007f32adfe5ac0 R11: 0000000000000206 R12: 000055eea23cbb78 > R13: 0000000000000000 R14: 0000000000000000 R15: 000055eea23cbc20 > > [0] https://lkml.kernel.org/r/20210401235925.GR4332@42.do-not-panic.com > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/block/zram/zram_drv.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index b26abcb955cc..60a55ae8cd91 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -1902,14 +1902,7 @@ static struct attribute *zram_disk_attrs[] = { > NULL, > }; > > -static const struct attribute_group zram_disk_attr_group = { > - .attrs = zram_disk_attrs, > -}; > - > -static const struct attribute_group *zram_disk_attr_groups[] = { > - &zram_disk_attr_group, > - NULL, > -}; > +ATTRIBUTE_GROUPS(zram_disk); > > /* > * Allocate and initialize new zram device. the function returns > @@ -1981,7 +1974,7 @@ static int zram_add(void) > blk_queue_max_write_zeroes_sectors(zram->disk->queue, UINT_MAX); > > blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, zram->disk->queue); > - device_add_disk(NULL, zram->disk, zram_disk_attr_groups); > + device_add_disk(NULL, zram->disk, zram_disk_groups); > > strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor)); > > -- > 2.30.2 >
On Tue, Oct 05, 2021 at 01:57:00PM -0700, Kees Cook wrote: > On Mon, Sep 27, 2021 at 09:38:05AM -0700, Luis Chamberlain wrote: > > The ATTRIBUTE_GROUPS is typically used to avoid boiler plate > > code which is used in many drivers. Embracing ATTRIBUTE_GROUPS was > > long due on the zram driver, however a recent fix for sysfs allows > > users of ATTRIBUTE_GROUPS to also associate a module to the group > > attribute. > > Does this mean that other modules using sysfs but _not_ > ATTRIBUTE_GROUPS() are still vulnerable to potential use-after-free of > the kernfs fops? The issue is not UAF, its the possible deadlock, but in that sense, yes. If they don't use ATTRIBUTE_GROUPS() then there is no information being provided to sysfs about the module owner. Luis
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index b26abcb955cc..60a55ae8cd91 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1902,14 +1902,7 @@ static struct attribute *zram_disk_attrs[] = { NULL, }; -static const struct attribute_group zram_disk_attr_group = { - .attrs = zram_disk_attrs, -}; - -static const struct attribute_group *zram_disk_attr_groups[] = { - &zram_disk_attr_group, - NULL, -}; +ATTRIBUTE_GROUPS(zram_disk); /* * Allocate and initialize new zram device. the function returns @@ -1981,7 +1974,7 @@ static int zram_add(void) blk_queue_max_write_zeroes_sectors(zram->disk->queue, UINT_MAX); blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, zram->disk->queue); - device_add_disk(NULL, zram->disk, zram_disk_attr_groups); + device_add_disk(NULL, zram->disk, zram_disk_groups); strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
The ATTRIBUTE_GROUPS is typically used to avoid boiler plate code which is used in many drivers. Embracing ATTRIBUTE_GROUPS was long due on the zram driver, however a recent fix for sysfs allows users of ATTRIBUTE_GROUPS to also associate a module to the group attribute. In zram's case this also means it allows us to fix a race which triggers a deadlock on the zram driver. This deadlock happens when a sysfs attribute use a lock also used on module removal. This happens when for instance a sysfs file on a driver is used, then at the same time we have module removal call trigger. The module removal call code holds a lock, and then the sysfs file entry waits for the same lock. While holding the lock the module removal tries to remove the sysfs entries, but these cannot be removed yet as one is waiting for a lock. This won't complete as the lock is already held. Likewise module removal cannot complete, and so we deadlock. Sysfs fixes this when the group attributes have a module associated to it, sysfs will *try* to get a refcount to the module when a shared lock is used, prior to mucking with a sysfs attribute. If this fails we just give up right away. This deadlock was first reported with the zram driver, a sketch of how this can happen follows: CPU A CPU B whatever_store() module_unload mutex_lock(foo) mutex_lock(foo) del_gendisk(zram->disk); device_del() device_remove_groups() In this situation whatever_store() is waiting for the mutex foo to become unlocked, but that won't happen until module removal is complete. But module removal won't complete until the sysfs file being poked completes which is waiting for a lock already held. This issue can be reproduced easily on the zram driver as follows: Loop 1 on one terminal: while true; do modprobe zram; modprobe -r zram; done Loop 2 on a second terminal: while true; do echo 1024 > /sys/block/zram0/disksize; echo 1 > /sys/block/zram0/reset; done Without this patch we end up in a deadlock, and the following stack trace is produced which hints to us what the issue was: INFO: task bash:888 blocked for more than 120 seconds. Tainted: G E 5.12.0-rc1-next-20210304+ #4 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. task:bash state:D stack: 0 pid: 888 ppid: 887 flags:<etc> Call Trace: __schedule+0x2e4/0x900 schedule+0x46/0xb0 schedule_preempt_disabled+0xa/0x10 __mutex_lock.constprop.0+0x2c3/0x490 ? _kstrtoull+0x35/0xd0 reset_store+0x6c/0x160 [zram] kernfs_fop_write_iter+0x124/0x1b0 new_sync_write+0x11c/0x1b0 vfs_write+0x1c2/0x260 ksys_write+0x5f/0xe0 do_syscall_64+0x33/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7f34f2c3df33 RSP: 002b:00007ffe751df6e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f34f2c3df33 RDX: 0000000000000002 RSI: 0000561ccb06ec10 RDI: 0000000000000001 RBP: 0000561ccb06ec10 R08: 000000000000000a R09: 0000000000000001 R10: 0000561ccb157590 R11: 0000000000000246 R12: 0000000000000002 R13: 00007f34f2d0e6a0 R14: 0000000000000002 R15: 00007f34f2d0e8a0 INFO: task modprobe:1104 can't die for more than 120 seconds. task:modprobe state:D stack: 0 pid: 1104 ppid: 916 flags:<etc> Call Trace: __schedule+0x2e4/0x900 schedule+0x46/0xb0 __kernfs_remove.part.0+0x228/0x2b0 ? finish_wait+0x80/0x80 kernfs_remove_by_name_ns+0x50/0x90 remove_files+0x2b/0x60 sysfs_remove_group+0x38/0x80 sysfs_remove_groups+0x29/0x40 device_remove_attrs+0x4a/0x80 device_del+0x183/0x3e0 ? mutex_lock+0xe/0x30 del_gendisk+0x27a/0x2d0 zram_remove+0x8a/0xb0 [zram] ? hot_remove_store+0xf0/0xf0 [zram] zram_remove_cb+0xd/0x10 [zram] idr_for_each+0x5e/0xd0 destroy_devices+0x39/0x6f [zram] __do_sys_delete_module+0x190/0x2a0 do_syscall_64+0x33/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7f32adf727d7 RSP: 002b:00007ffc08bb38a8 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0 RAX: ffffffffffffffda RBX: 000055eea23cbb10 RCX: 00007f32adf727d7 RDX: 0000000000000000 RSI: 0000000000000800 RDI: 000055eea23cbb78 RBP: 000055eea23cbb10 R08: 0000000000000000 R09: 0000000000000000 R10: 00007f32adfe5ac0 R11: 0000000000000206 R12: 000055eea23cbb78 R13: 0000000000000000 R14: 0000000000000000 R15: 000055eea23cbc20 [0] https://lkml.kernel.org/r/20210401235925.GR4332@42.do-not-panic.com Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/block/zram/zram_drv.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)