| Message ID | 20240710040654.1714672-7-libaokun@huaweicloud.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: some bugfixes and cleanups for ext4 extents path | expand |
On Wed 10-07-24 12:06:40, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > 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@huawei.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> 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 > --- > 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 >
On 2024/7/25 3:16, Jan Kara wrote: > On Wed 10-07-24 12:06:40, libaokun@huaweicloud.com wrote: >> From: Baokun Li <libaokun1@huawei.com> >> >> 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@huawei.com> > Looks good. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > 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 >>
On Wed, Jul 10, 2024 at 12:06:40PM +0800, libaokun@huaweicloud.com wrote: > From: Baokun Li <libaokun1@huawei.com> > > 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@huawei.com> Looks good, feel free to add: Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> Regards, ojaswin > --- > 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 >
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; }