Re: [PATCH 05/20] ext4: fix slab-use-after-free in ext4_split_extent_at()

[Ojaswin Mujoo]

On Tue, Jul 30, 2024 at 04:57:35PM +0800, Baokun Li wrote:
> On 2024/7/27 18:36, Ojaswin Mujoo wrote:
> > On Wed, Jul 10, 2024 at 12:06:39PM +0800, libaokun@xxxxxxxxxxxxxxx wrote:
> > > From: Baokun Li <libaokun1@xxxxxxxxxx>
> > > 
> > > We hit the following use-after-free:
> > > 
> > > ==================================================================
> > > BUG: KASAN: slab-use-after-free in ext4_split_extent_at+0xba8/0xcc0
> > > Read of size 2 at addr ffff88810548ed08 by task kworker/u20:0/40
> > > CPU: 0 PID: 40 Comm: kworker/u20:0 Not tainted 6.9.0-dirty #724
> > > Call Trace:
> > >   <TASK>
> > >   kasan_report+0x93/0xc0
> > >   ext4_split_extent_at+0xba8/0xcc0
> > >   ext4_split_extent.isra.0+0x18f/0x500
> > >   ext4_split_convert_extents+0x275/0x750
> > >   ext4_ext_handle_unwritten_extents+0x73e/0x1580
> > >   ext4_ext_map_blocks+0xe20/0x2dc0
> > >   ext4_map_blocks+0x724/0x1700
> > >   ext4_do_writepages+0x12d6/0x2a70
> > > [...]
> > > 
> > > Allocated by task 40:
> > >   __kmalloc_noprof+0x1ac/0x480
> > >   ext4_find_extent+0xf3b/0x1e70
> > >   ext4_ext_map_blocks+0x188/0x2dc0
> > >   ext4_map_blocks+0x724/0x1700
> > >   ext4_do_writepages+0x12d6/0x2a70
> > > [...]
> > > 
> > > Freed by task 40:
> > >   kfree+0xf1/0x2b0
> > >   ext4_find_extent+0xa71/0x1e70
> > >   ext4_ext_insert_extent+0xa22/0x3260
> > >   ext4_split_extent_at+0x3ef/0xcc0
> > >   ext4_split_extent.isra.0+0x18f/0x500
> > >   ext4_split_convert_extents+0x275/0x750
> > >   ext4_ext_handle_unwritten_extents+0x73e/0x1580
> > >   ext4_ext_map_blocks+0xe20/0x2dc0
> > >   ext4_map_blocks+0x724/0x1700
> > >   ext4_do_writepages+0x12d6/0x2a70
> > > [...]
> > > ==================================================================
> > > 
> > > The flow of issue triggering is as follows:
> > > 
> > > ext4_split_extent_at
> > >    path = *ppath
> > >    ext4_ext_insert_extent(ppath)
> > >      ext4_ext_create_new_leaf(ppath)
> > >        ext4_find_extent(orig_path)
> > >          path = *orig_path
> > >          read_extent_tree_block
> > >            // return -ENOMEM or -EIO
> > >          ext4_free_ext_path(path)
> > >            kfree(path)
> > >          *orig_path = NULL
> > >    a. If err is -ENOMEM:
> > >    ext4_ext_dirty(path + path->p_depth)
> > >    // path use-after-free !!!
> > >    b. If err is -EIO and we have EXT_DEBUG defined:
> > >    ext4_ext_show_leaf(path)
> > >      eh = path[depth].p_hdr
> > >      // path also use-after-free !!!
> > > 
> > > So when trying to zeroout or fix the extent length, call ext4_find_extent()
> > > to update the path.
> > > 
> > > In addition we use *ppath directly as an ext4_ext_show_leaf() input to
> > > avoid possible use-after-free when EXT_DEBUG is defined, and to avoid
> > > unnecessary path updates.
> > > 
> > > Fixes: dfe5080939ea ("ext4: drop EXT4_EX_NOFREE_ON_ERR from rest of extents handling code")
> > > Cc: stable@xxxxxxxxxx
> > > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> > > ---
> > >   fs/ext4/extents.c | 21 ++++++++++++++++++++-
> > >   1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > > index 6e5b5baf3aa6..3a70a0739af8 100644
> > > --- a/fs/ext4/extents.c
> > > +++ b/fs/ext4/extents.c
> > > @@ -3252,6 +3252,25 @@ static int ext4_split_extent_at(handle_t *handle,
> > >    if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
> > >      goto out;
> > > + /*
> > > +  * Update path is required because previous ext4_ext_insert_extent()
> > > +  * may have freed or reallocated the path. Using EXT4_EX_NOFAIL
> > > +  * guarantees that ext4_find_extent() will not return -ENOMEM,
> > > +  * otherwise -ENOMEM will cause a retry in do_writepages(), and a
> > > +  * WARN_ON may be triggered in ext4_da_update_reserve_space() due to
> > > +  * an incorrect ee_len causing the i_reserved_data_blocks exception.
> > > +  */
> > > + path = ext4_find_extent(inode, ee_block, ppath,
> > > +       flags | EXT4_EX_NOFAIL);
> > > + if (IS_ERR(path)) {
> > > +   EXT4_ERROR_INODE(inode, "Failed split extent on %u, err %ld",
> > > +        split, PTR_ERR(path));
> > > +   return PTR_ERR(path);
> > > + }
> > > + depth = ext_depth(inode);
> > > + ex = path[depth].p_ext;
> > > + *ppath = path;
> > > +
> > Hi Baokun, nice catch!
> > 
> > I was just wondering if it makes more sense to only update path if we
> > encounter an error:
> > 
> >    err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
> >    if (err != -ENOSPC && err != -EDQUOT && err != -ENOMEM)
> >      goto out;
> >    else if (err < 0) {
> >      ...
> >      path = ext4_find_extent(inode, ee_block, ppath, flags | EXT4_EX_NOFAIL);
> >    }
> > 
> > I believe that's the only time we'd end up with the use after free issue
> > and this way we can avoid the (maybe tiny) performance overhead of calling
> > the extra find extent. What are your thoughts?
> > 
> > regards,
> > ojaswin
> Hi Ojaswin,
> 
> We're already there now, updating the path only on recoverable,
> non-critical errors (-ENOSPC, -EDQUOT, and -ENOMEM).
> 
> Thanks,
> Baokun

Hey Baokun, yes you are right I somehow misread the code:

Feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>

Regards,
ojaswin

> > >    if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> > >      if (split_flag & (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
> > >        if (split_flag & EXT4_EXT_DATA_VALID1) {
> > > @@ -3304,7 +3323,7 @@ static int ext4_split_extent_at(handle_t *handle,
> > >    ext4_ext_dirty(handle, inode, path + path->p_depth);
> > >    return err;
> > >   out:
> > > - ext4_ext_show_leaf(inode, path);
> > > + ext4_ext_show_leaf(inode, *ppath);
> > >    return err;
> > >   }
> > > -- 
> > > 2.39.2
> > > 
> -- 
> With Best Regards,
> Baokun Li
>