Re: [PATCH] ext4: fix use-after-free in dx_release()
On Wed, May 08, 2019 at 01:09:47AM -0600, Andreas Dilger wrote: > On May 8, 2019, at 12:13 AM, Sahitya Tummala wrote: > > > > The buffer_head (frames[0].bh) and it's corresping page can be > > potentially free'd once brelse() is done inside the for loop > > but before the for loop exits in dx_release(). It can be free'd > > in another context, when the page cache is flushed via > > drop_caches_sysctl_handler(). This results into below data abort > > when accessing info->indirect_levels in dx_release(). > > > > Unable to handle kernel paging request at virtual address ffc17ac3e01e > > Call trace: > > dx_release+0x70/0x90 > > ext4_htree_fill_tree+0x2d4/0x300 > > ext4_readdir+0x244/0x6f8 > > iterate_dir+0xbc/0x160 > > SyS_getdents64+0x94/0x174 > > > > Signed-off-by: Sahitya Tummala > > The patch looks reasonable, but there is a danger that it may be > "optimized" back to the pre-patch form again. It probably makes > sense to include a comment like: > > /* save local copy, "info" may be freed after brelse() */ Thanks for reviewing it. Sure, I will add the comment. > > Looks fine otherwise. > > Reviewed-by: Andreas Dilger > > > --- > > fs/ext4/namei.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > > index 4181c9c..7e6c298 100644 > > --- a/fs/ext4/namei.c > > +++ b/fs/ext4/namei.c > > @@ -871,12 +871,14 @@ static void dx_release(struct dx_frame *frames) > > { > > struct dx_root_info *info; > > int i; > > + unsigned int indirect_levels; > > > > if (frames[0].bh == NULL) > > return; > > > > info = &((struct dx_root *)frames[0].bh->b_data)->info; > > - for (i = 0; i <= info->indirect_levels; i++) { > > + indirect_levels = info->indirect_levels; > > + for (i = 0; i <= indirect_levels; i++) { > > if (frames[i].bh == NULL) > > break; > > brelse(frames[i].bh); > > -- > > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, > > Inc. > > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux > > Foundation Collaborative Project. > > > > > Cheers, Andreas > > > > > -- -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Re: [PATCH] ext4: fix use-after-free in dx_release()
On May 8, 2019, at 12:13 AM, Sahitya Tummala wrote: > > The buffer_head (frames[0].bh) and it's corresping page can be > potentially free'd once brelse() is done inside the for loop > but before the for loop exits in dx_release(). It can be free'd > in another context, when the page cache is flushed via > drop_caches_sysctl_handler(). This results into below data abort > when accessing info->indirect_levels in dx_release(). > > Unable to handle kernel paging request at virtual address ffc17ac3e01e > Call trace: > dx_release+0x70/0x90 > ext4_htree_fill_tree+0x2d4/0x300 > ext4_readdir+0x244/0x6f8 > iterate_dir+0xbc/0x160 > SyS_getdents64+0x94/0x174 > > Signed-off-by: Sahitya Tummala The patch looks reasonable, but there is a danger that it may be "optimized" back to the pre-patch form again. It probably makes sense to include a comment like: /* save local copy, "info" may be freed after brelse() */ Looks fine otherwise. Reviewed-by: Andreas Dilger > --- > fs/ext4/namei.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 4181c9c..7e6c298 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -871,12 +871,14 @@ static void dx_release(struct dx_frame *frames) > { > struct dx_root_info *info; > int i; > + unsigned int indirect_levels; > > if (frames[0].bh == NULL) > return; > > info = &((struct dx_root *)frames[0].bh->b_data)->info; > - for (i = 0; i <= info->indirect_levels; i++) { > + indirect_levels = info->indirect_levels; > + for (i = 0; i <= indirect_levels; i++) { > if (frames[i].bh == NULL) > break; > brelse(frames[i].bh); > -- > Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux > Foundation Collaborative Project. > Cheers, Andreas signature.asc Description: Message signed with OpenPGP
[PATCH] ext4: fix use-after-free in dx_release()
The buffer_head (frames[0].bh) and it's corresping page can be potentially free'd once brelse() is done inside the for loop but before the for loop exits in dx_release(). It can be free'd in another context, when the page cache is flushed via drop_caches_sysctl_handler(). This results into below data abort when accessing info->indirect_levels in dx_release(). Unable to handle kernel paging request at virtual address ffc17ac3e01e Call trace: dx_release+0x70/0x90 ext4_htree_fill_tree+0x2d4/0x300 ext4_readdir+0x244/0x6f8 iterate_dir+0xbc/0x160 SyS_getdents64+0x94/0x174 Signed-off-by: Sahitya Tummala --- fs/ext4/namei.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 4181c9c..7e6c298 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -871,12 +871,14 @@ static void dx_release(struct dx_frame *frames) { struct dx_root_info *info; int i; + unsigned int indirect_levels; if (frames[0].bh == NULL) return; info = &((struct dx_root *)frames[0].bh->b_data)->info; - for (i = 0; i <= info->indirect_levels; i++) { + indirect_levels = info->indirect_levels; + for (i = 0; i <= indirect_levels; i++) { if (frames[i].bh == NULL) break; brelse(frames[i].bh); -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.