Commit c2a04b02 authored by Jamie Iles's avatar Jamie Iles Committed by Andreas Gruenbacher

gfs2: use-after-free in sysfs deregistration

syzkaller found the following splat with CONFIG_DEBUG_KOBJECT_RELEASE=y:

  Read of size 1 at addr ffff000028e896b8 by task kworker/1:2/228

  CPU: 1 PID: 228 Comm: kworker/1:2 Tainted: G S                5.9.0-rc8+ #101
  Hardware name: linux,dummy-virt (DT)
  Workqueue: events kobject_delayed_cleanup
  Call trace:
   dump_backtrace+0x0/0x4d8
   show_stack+0x34/0x48
   dump_stack+0x174/0x1f8
   print_address_description.constprop.0+0x5c/0x550
   kasan_report+0x13c/0x1c0
   __asan_report_load1_noabort+0x34/0x60
   memcmp+0xd0/0xd8
   gfs2_uevent+0xc4/0x188
   kobject_uevent_env+0x54c/0x1240
   kobject_uevent+0x2c/0x40
   __kobject_del+0x190/0x1d8
   kobject_delayed_cleanup+0x2bc/0x3b8
   process_one_work+0x96c/0x18c0
   worker_thread+0x3f0/0xc30
   kthread+0x390/0x498
   ret_from_fork+0x10/0x18

  Allocated by task 1110:
   kasan_save_stack+0x28/0x58
   __kasan_kmalloc.isra.0+0xc8/0xe8
   kasan_kmalloc+0x10/0x20
   kmem_cache_alloc_trace+0x1d8/0x2f0
   alloc_super+0x64/0x8c0
   sget_fc+0x110/0x620
   get_tree_bdev+0x190/0x648
   gfs2_get_tree+0x50/0x228
   vfs_get_tree+0x84/0x2e8
   path_mount+0x1134/0x1da8
   do_mount+0x124/0x138
   __arm64_sys_mount+0x164/0x238
   el0_svc_common.constprop.0+0x15c/0x598
   do_el0_svc+0x60/0x150
   el0_svc+0x34/0xb0
   el0_sync_handler+0xc8/0x5b4
   el0_sync+0x15c/0x180

  Freed by task 228:
   kasan_save_stack+0x28/0x58
   kasan_set_track+0x28/0x40
   kasan_set_free_info+0x24/0x48
   __kasan_slab_free+0x118/0x190
   kasan_slab_free+0x14/0x20
   slab_free_freelist_hook+0x6c/0x210
   kfree+0x13c/0x460

Use the same pattern as f2fs + ext4 where the kobject destruction must
complete before allowing the FS itself to be freed.  This means that we
need an explicit free_sbd in the callers.

Cc: Bob Peterson <rpeterso@redhat.com>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: default avatarJamie Iles <jamie@nuviainc.com>
[Also go to fail_free when init_names fails.]
Signed-off-by: default avatarAndreas Gruenbacher <agruenba@redhat.com>
parent 0e539ca1
...@@ -705,6 +705,7 @@ struct gfs2_sbd { ...@@ -705,6 +705,7 @@ struct gfs2_sbd {
struct super_block *sd_vfs; struct super_block *sd_vfs;
struct gfs2_pcpu_lkstats __percpu *sd_lkstats; struct gfs2_pcpu_lkstats __percpu *sd_lkstats;
struct kobject sd_kobj; struct kobject sd_kobj;
struct completion sd_kobj_unregister;
unsigned long sd_flags; /* SDF_... */ unsigned long sd_flags; /* SDF_... */
struct gfs2_sb_host sd_sb; struct gfs2_sb_host sd_sb;
......
...@@ -1062,26 +1062,14 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc) ...@@ -1062,26 +1062,14 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
} }
error = init_names(sdp, silent); error = init_names(sdp, silent);
if (error) { if (error)
/* In this case, we haven't initialized sysfs, so we have to goto fail_free;
manually free the sdp. */
free_sbd(sdp);
sb->s_fs_info = NULL;
return error;
}
snprintf(sdp->sd_fsname, sizeof(sdp->sd_fsname), "%s", sdp->sd_table_name); snprintf(sdp->sd_fsname, sizeof(sdp->sd_fsname), "%s", sdp->sd_table_name);
error = gfs2_sys_fs_add(sdp); error = gfs2_sys_fs_add(sdp);
/*
* If we hit an error here, gfs2_sys_fs_add will have called function
* kobject_put which causes the sysfs usage count to go to zero, which
* causes sysfs to call function gfs2_sbd_release, which frees sdp.
* Subsequent error paths here will call gfs2_sys_fs_del, which also
* kobject_put to free sdp.
*/
if (error) if (error)
return error; goto fail_free;
gfs2_create_debugfs_file(sdp); gfs2_create_debugfs_file(sdp);
...@@ -1179,9 +1167,9 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc) ...@@ -1179,9 +1167,9 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
gfs2_lm_unmount(sdp); gfs2_lm_unmount(sdp);
fail_debug: fail_debug:
gfs2_delete_debugfs_file(sdp); gfs2_delete_debugfs_file(sdp);
/* gfs2_sys_fs_del must be the last thing we do, since it causes
* sysfs to call function gfs2_sbd_release, which frees sdp. */
gfs2_sys_fs_del(sdp); gfs2_sys_fs_del(sdp);
fail_free:
free_sbd(sdp);
sb->s_fs_info = NULL; sb->s_fs_info = NULL;
return error; return error;
} }
......
...@@ -744,6 +744,7 @@ static void gfs2_put_super(struct super_block *sb) ...@@ -744,6 +744,7 @@ static void gfs2_put_super(struct super_block *sb)
/* At this point, we're through participating in the lockspace */ /* At this point, we're through participating in the lockspace */
gfs2_sys_fs_del(sdp); gfs2_sys_fs_del(sdp);
free_sbd(sdp);
} }
/** /**
......
...@@ -303,7 +303,7 @@ static void gfs2_sbd_release(struct kobject *kobj) ...@@ -303,7 +303,7 @@ static void gfs2_sbd_release(struct kobject *kobj)
{ {
struct gfs2_sbd *sdp = container_of(kobj, struct gfs2_sbd, sd_kobj); struct gfs2_sbd *sdp = container_of(kobj, struct gfs2_sbd, sd_kobj);
free_sbd(sdp); complete(&sdp->sd_kobj_unregister);
} }
static struct kobj_type gfs2_ktype = { static struct kobj_type gfs2_ktype = {
...@@ -655,6 +655,7 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp) ...@@ -655,6 +655,7 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
sprintf(ro, "RDONLY=%d", sb_rdonly(sb)); sprintf(ro, "RDONLY=%d", sb_rdonly(sb));
sprintf(spectator, "SPECTATOR=%d", sdp->sd_args.ar_spectator ? 1 : 0); sprintf(spectator, "SPECTATOR=%d", sdp->sd_args.ar_spectator ? 1 : 0);
init_completion(&sdp->sd_kobj_unregister);
sdp->sd_kobj.kset = gfs2_kset; sdp->sd_kobj.kset = gfs2_kset;
error = kobject_init_and_add(&sdp->sd_kobj, &gfs2_ktype, NULL, error = kobject_init_and_add(&sdp->sd_kobj, &gfs2_ktype, NULL,
"%s", sdp->sd_table_name); "%s", sdp->sd_table_name);
...@@ -685,6 +686,7 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp) ...@@ -685,6 +686,7 @@ int gfs2_sys_fs_add(struct gfs2_sbd *sdp)
fail_reg: fail_reg:
fs_err(sdp, "error %d adding sysfs files\n", error); fs_err(sdp, "error %d adding sysfs files\n", error);
kobject_put(&sdp->sd_kobj); kobject_put(&sdp->sd_kobj);
wait_for_completion(&sdp->sd_kobj_unregister);
sb->s_fs_info = NULL; sb->s_fs_info = NULL;
return error; return error;
} }
...@@ -695,6 +697,7 @@ void gfs2_sys_fs_del(struct gfs2_sbd *sdp) ...@@ -695,6 +697,7 @@ void gfs2_sys_fs_del(struct gfs2_sbd *sdp)
sysfs_remove_group(&sdp->sd_kobj, &tune_group); sysfs_remove_group(&sdp->sd_kobj, &tune_group);
sysfs_remove_group(&sdp->sd_kobj, &lock_module_group); sysfs_remove_group(&sdp->sd_kobj, &lock_module_group);
kobject_put(&sdp->sd_kobj); kobject_put(&sdp->sd_kobj);
wait_for_completion(&sdp->sd_kobj_unregister);
} }
static int gfs2_uevent(struct kset *kset, struct kobject *kobj, static int gfs2_uevent(struct kset *kset, struct kobject *kobj,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment