linux-kernel - Re: [syzbot] [fs?] KASAN: slab-use-after-free Read in test_bdev_super

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230805085703.GA30229@lst.de>
Date:   Sat, 5 Aug 2023 10:57:03 +0200
From:   Christoph Hellwig <hch@....de>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Christoph Hellwig <hch@....de>,
        syzbot <syzbot+2faac0423fdc9692822b@...kaller.appspotmail.com>,
        jack@...e.cz, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        viro@...iv.linux.org.uk
Subject: Re: [syzbot] [fs?] KASAN: slab-use-after-free Read in
 test_bdev_super_fc

On Fri, Aug 04, 2023 at 05:29:37PM +0200, Christian Brauner wrote:
> On Fri, Aug 04, 2023 at 04:49:23PM +0200, Christian Brauner wrote:
> > On Fri, Aug 04, 2023 at 04:43:43PM +0200, Christoph Hellwig wrote:
> > > On Fri, Aug 04, 2023 at 04:36:49PM +0200, Christian Brauner wrote:
> > > > FFS
> > > 
> > > Good spot, this explains the missing dropping of s_umount.
> > > 
> > > But I don't think it's doing the right thing for MTD mount romfs,
> > > we'll need something like this:
> > 
> > I'll fold a fix into Jan's patch.
> 
> Folding:

Btw, we really need to think about reworking how super block freeing
works.  The calling conventions of ->kill_sb are really horrible right
now:

 - every instance of ->kill_sb is supposed to call generic_shutdown_super,
   but the instances can do work before and after it
 - we have a few generic helpers wrapping generic_shutdown_super, but
   they aren't easily combinable for file systems using say MTD and
   block backends.

Pair that with ->put_super also called from generic_shutdown_super
I think we have a major mess.

Here is my rough and not very well thought out idea (having a lot of
backlog and beeing on the way to a family celebreation):

 1) make ->kill_sb optional and default to generic_shutdown_super if
    not provided
 2) add a new ->free_fs_info method that is called at the end of
    generic_shutdown_super to free sb->s_fs_info
    (maybe thing if we should also call this on a failed mount
     for fc_fs_info, but I'm not quite sure about that) and then
     migrate everything that just frees resources over to that
 3) figure out what work really needs to be before
    generic_shutdown_super, and if there is something add a new
    method for it
 4) if we added the new method in 3 figure out if it can also
    take over the job from ->put_super
 5) PROFIT!!! (well, actually remove ->kill_sb).


> 
> diff --git a/fs/romfs/super.c b/fs/romfs/super.c
> index c59b230d55b4..2b9f3e3c052a 100644
> --- a/fs/romfs/super.c
> +++ b/fs/romfs/super.c
> @@ -583,16 +583,20 @@ static int romfs_init_fs_context(struct fs_context *fc)
>   */
>  static void romfs_kill_sb(struct super_block *sb)
>  {
> +	generic_shutdown_super(sb);
> +
>  #ifdef CONFIG_ROMFS_ON_MTD
>  	if (sb->s_mtd) {
> -		kill_mtd_super(sb);
> -		return;
> +		put_mtd_device(sb->s_mtd);
> +		sb->s_mtd = NULL;
>  	}
>  #endif
>  #ifdef CONFIG_ROMFS_ON_BLOCK
>  	if (sb->s_bdev) {
> -		kill_block_super(sb);
> -		return;
> +		sb->s_bdev->bd_super = NULL;
> +		sync_blockdev(sb->s_bdev);
> +		blkdev_put(sb->s_bdev, sb->s_type);
>  	}
>  #endif
>  }
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 27c6597aa1be..0b6cc8a03b54 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -485,12 +485,17 @@ static void cramfs_kill_sb(struct super_block *sb)
>  {
>         struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
> 
> +       generic_shutdown_super(sb);
> +
>         if (IS_ENABLED(CONFIG_CRAMFS_MTD) && sb->s_mtd) {
>                 if (sbi && sbi->mtd_point_size)
>                         mtd_unpoint(sb->s_mtd, 0, sbi->mtd_point_size);
> -               kill_mtd_super(sb);
> +               put_mtd_device(sb->s_mtd);
> +               sb->s_mtd = NULL;
>         } else if (IS_ENABLED(CONFIG_CRAMFS_BLOCKDEV) && sb->s_bdev) {
> -               kill_block_super(sb);
> +               sb->s_bdev->bd_super = NULL;
> +               sync_blockdev(sb->s_bdev);
> +               blkdev_put(sb->s_bdev, sb->s_type);
>         }
>         kfree(sbi);
>  }
> 
---end quoted text---