Re: [PATCH 06/20] ext4: avoid use-after-free in ext4_ext_show_leaf()

[Baokun Li]

On 2024/7/25 3:16, Jan Kara wrote:
> On Wed 10-07-24 12:06:40, libaokun@xxxxxxxxxxxxxxx wrote:
> > From: Baokun Li <libaokun1@xxxxxxxxxx>
> >
> > In ext4_find_extent(), path may be freed by error or be reallocated, so
> > using a previously saved *ppath may have been freed and thus may trigger
> > use-after-free, as follows:
> >
> > ext4_split_extent
> >    path = *ppath;
> >    ext4_split_extent_at(ppath)
> >    path = ext4_find_extent(ppath)
> >    ext4_split_extent_at(ppath)
> >      // ext4_find_extent fails to free path
> >      // but zeroout succeeds
> >    ext4_ext_show_leaf(inode, path)
> >      eh = path[depth].p_hdr
> >      // path use-after-free !!!
> >
> > Similar to ext4_split_extent_at(), we use *ppath directly as an input to
> > ext4_ext_show_leaf(). Fix a spelling error by the way.
> >
> > Same problem in ext4_ext_handle_unwritten_extents(). Since 'path' is only
> > used in ext4_ext_show_leaf(), remove 'path' and use *ppath directly.
> >
> > This issue is triggered only when EXT_DEBUG is defined and therefore does
> > not affect functionality.
> >
> > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> Looks good. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> I'd just note that this shows that modifying ppath in the called function
> was not a great idea as it makes possible use-after-free issues due to
> cached values being used very hard to spot and very easy to introduce...
>
> 								Honza

Hi Honza,

Thank you very much for your review!

Yes, it was too confusing, which is why I dropped all ppaths after fixing
the problem. Judging and using the returned path every time would
make the code look a lot simpler.

Thanks,
Baokun
> > ---
> >   fs/ext4/extents.c | 9 ++++-----
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 3a70a0739af8..1660434fbfc7 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -3328,7 +3328,7 @@ static int ext4_split_extent_at(handle_t *handle,
> >   }
> >   
> >   /*
> > - * ext4_split_extents() splits an extent and mark extent which is covered
> > + * ext4_split_extent() splits an extent and mark extent which is covered
> >    * by @map as split_flags indicates
> >    *
> >    * It may result in splitting the extent into multiple extents (up to three)
> > @@ -3404,7 +3404,7 @@ static int ext4_split_extent(handle_t *handle,
> >   			goto out;
> >   	}
> >   
> > -	ext4_ext_show_leaf(inode, path);
> > +	ext4_ext_show_leaf(inode, *ppath);
> >   out:
> >   	return err ? err : allocated;
> >   }
> > @@ -3869,14 +3869,13 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
> >   			struct ext4_ext_path **ppath, int flags,
> >   			unsigned int allocated, ext4_fsblk_t newblock)
> >   {
> > -	struct ext4_ext_path __maybe_unused *path = *ppath;
> >   	int ret = 0;
> >   	int err = 0;
> >   
> >   	ext_debug(inode, "logical block %llu, max_blocks %u, flags 0x%x, allocated %u\n",
> >   		  (unsigned long long)map->m_lblk, map->m_len, flags,
> >   		  allocated);
> > -	ext4_ext_show_leaf(inode, path);
> > +	ext4_ext_show_leaf(inode, *ppath);
> >   
> >   	/*
> >   	 * When writing into unwritten space, we should not fail to
> > @@ -3973,7 +3972,7 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
> >   	if (allocated > map->m_len)
> >   		allocated = map->m_len;
> >   	map->m_len = allocated;
> > -	ext4_ext_show_leaf(inode, path);
> > +	ext4_ext_show_leaf(inode, *ppath);
> >   out2:
> >   	return err ? err : allocated;
> >   }
> > --
> > 2.39.2
--
With Best Regards,
Baokun Li