linux-ext4 - Re: [PATCH 4.19] ext4: fix race condition between ext4_ioctl_setflags and ext4

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Jul 2022 14:25:18 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Baokun Li <libaokun1@...wei.com>
Cc:     stable@...r.kernel.org, linux-ext4@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, jack@...e.cz, ritesh.list@...il.com,
        lczerner@...hat.com, enwlinux@...il.com,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        yebin10@...wei.com, yukuai3@...wei.com,
        Hulk Robot <hulkci@...wei.com>
Subject: Re: [PATCH 4.19] ext4: fix race condition between
 ext4_ioctl_setflags and ext4_fiemap

On Tue, Jul 19, 2022 at 08:15:13PM +0800, Baokun Li wrote:
> 在 2022/7/19 19:26, Greg KH 写道:
> > On Sat, Jul 16, 2022 at 10:33:30AM +0800, Baokun Li wrote:
> > > 在 2022/7/15 22:10, Greg KH 写道:
> > > > On Fri, Jul 15, 2022 at 10:39:28AM +0800, Baokun Li wrote:
> > > > > This patch and problem analysis is based on v4.19 LTS.
> > > > > The d3b6f23f7167("ext4: move ext4_fiemap to use iomap framework") patch
> > > > > is incorporated in v5.7-rc1. This patch avoids this problem by switching
> > > > > to iomap in ext4_fiemap.
> > > > > 
> > > > > Hulk Robot reported a BUG on stable 4.19.252:
> > > > > ==================================================================
> > > > > kernel BUG at fs/ext4/extents_status.c:762!
> > > > > invalid opcode: 0000 [#1] SMP KASAN PTI
> > > > > CPU: 7 PID: 2845 Comm: syz-executor Not tainted 4.19.252 #46
> > > > > RIP: 0010:ext4_es_cache_extent+0x30e/0x370
> > > > > [...]
> > > > > Call Trace:
> > > > >    ext4_cache_extents+0x238/0x2f0
> > > > >    ext4_find_extent+0x785/0xa40
> > > > >    ext4_fiemap+0x36d/0xe90
> > > > >    do_vfs_ioctl+0x6af/0x1200
> > > > > [...]
> > > > > ==================================================================
> > > > > 
> > > > > Above issue may happen as follows:
> > > > > -------------------------------------
> > > > >              cpu1		    cpu2
> > > > > _____________________|_____________________
> > > > > do_vfs_ioctl
> > > > >    ext4_ioctl
> > > > >     ext4_ioctl_setflags
> > > > >      ext4_ind_migrate
> > > > >                           do_vfs_ioctl
> > > > >                            ioctl_fiemap
> > > > >                             ext4_fiemap
> > > > >                              ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)
> > > > >                              ext4_fill_fiemap_extents
> > > > >       down_write(&EXT4_I(inode)->i_data_sem);
> > > > >       ext4_ext_check_inode
> > > > >       ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS)
> > > > >       memset(ei->i_data, 0, sizeof(ei->i_data))
> > > > >       up_write(&EXT4_I(inode)->i_data_sem);
> > > > >                               down_read(&EXT4_I(inode)->i_data_sem);
> > > > >                               ext4_find_extent
> > > > >                                ext4_cache_extents
> > > > >                                 ext4_es_cache_extent
> > > > >                                  BUG_ON(end < lblk)
> > > > > 
> > > > > We can easily reproduce this problem with the syzkaller testcase:
> > > > > ```
> > > > > 02:37:07 executing program 3:
> > > > > r0 = openat(0xffffffffffffff9c, &(0x7f0000000040)='./file0\x00', 0x26e1, 0x0)
> > > > > ioctl$FS_IOC_FSSETXATTR(r0, 0x40086602, &(0x7f0000000080)={0x17e})
> > > > > mkdirat(0xffffffffffffff9c, &(0x7f00000000c0)='./file1\x00', 0x1ff)
> > > > > r1 = openat(0xffffffffffffff9c, &(0x7f0000000100)='./file1\x00', 0x0, 0x0)
> > > > > ioctl$FS_IOC_FIEMAP(r1, 0xc020660b, &(0x7f0000000180)={0x0, 0x1, 0x0, 0xef3, 0x6, []}) (async, rerun: 32)
> > > > > ioctl$FS_IOC_FSSETXATTR(r1, 0x40086602, &(0x7f0000000140)={0x17e}) (rerun: 32)
> > > > > ```
> > > > > 
> > > > > To solve this issue, we use __generic_block_fiemap() instead of
> > > > > generic_block_fiemap() and add inode_lock_shared to avoid race condition.
> > > > > 
> > > > > Reported-by: Hulk Robot <hulkci@...wei.com>
> > > > > Signed-off-by: Baokun Li <libaokun1@...wei.com>
> > > > > ---
> > > > >    fs/ext4/extents.c | 15 +++++++++++----
> > > > >    1 file changed, 11 insertions(+), 4 deletions(-)
> > > > What is the git commit id of this change in Linus's tree?
> > > > 
> > > > If it is not in Linus's tree, why not?
> > > > 
> > > > confused,
> > > > 
> > > > greg k-h
> > > > .
> > > This patch does not exist in the Linus' tree.
> > > 
> > > This problem persists until the patch d3b6f23f7167("ext4: move ext4_fiemap
> > > to use iomap framework") is incorporated in v5.7-rc1.
> > Then why not ask for that change to be added instead?
> > 
> > thanks,
> > 
> > greg k-h
> > .
> 
> If we want to switch to the iomap framework, we need to analyze and
> integrate about 60 patches.
> 
> The workload may be greater than that of solving this problem alone.

95% of the time we take a patch that is not in Linus's tree, it is buggy
and causes problems in the long run.  See what those 60 patches really
require and if this issue really does need all of that.

Or better yet, take the effort here and move off of 4.19 to a newer
kernel without this problem in it.  What is preventing you from doing
that today?  4.19 is not going to be around for forever, and will
probably not even be getting fixes for stuff like RETBLEED, so are you
_SURE_ you want to keep using it?

thanks,

greg k-h