Re: [PATCH] ext4: fix bug in extents parsing when number of entries in header is zero

[Luís Henriques]

On Fri, Aug 12, 2022 at 10:33:20AM +0800, Baokun Li wrote:
> 在 2022/8/5 22:00, Luís Henriques 写道:
...
> > This bug is easily reproducible using the filesystem image provided --
> > it's just a matter of mounting it and run:
> > 
> >      $ cat /mnt/foo/bar/xattr
> 
> Hi Luís,
> yeah, that's a good catch!
> > Anyway, I hope my analysis of the bug is correct -- the root cause seems
> > to be an extent header with an invalid value for in eh_entries, which will
> > later cause the BUG_ON().
> > 
> > Cheers,
> > --
> > Luís
> But there's a little bit of a deviation in your understanding of the
> problem,
> so the patch doesn't look good.
> The issue is caused by the contradiction between eh_entries and eh_depth.

Ah! This makes a lot of sense and I can confirm this is exactly what
happens in both bugzilla images.  Thanks a lot for your feedback!

> Therefore, we need to check the contradiction instead of adding a judgment
> to ext4_ext_binsearch_idx.
> So the right fix is to add a check to __ext4_ext_check like:
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index c148bb97b527..2dfd35f727cb 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -460,6 +460,10 @@ static int __ext4_ext_check(const char *function,
> unsigned int line,
>                 error_msg = "invalid eh_entries";
>                 goto corrupted;
>         }
> +       if (unlikely((eh->eh_entries == 0) && (depth > 0))) {
> +               error_msg = "contradictory eh_entries and eh_depth";
> +               goto corrupted;
> +       }
>         if (!ext4_valid_extent_entries(inode, eh, lblk, &pblk, depth)) {
>                 error_msg = "invalid extent entries";
>                 goto corrupted;
> 
> In this way, we can fix this issue and check for header exceptions before
> calling ext4_ext_binsearch_idx.

Awesome, I'll send out v2 with the suggested change.  It makes sense to
have this check and it should fix both bugs.

On the other hand, I still wonder wether the extra check in my original
patch is correct or not.  I spent a good amount of time trying to find out
if eh_entries can be 0 at that point (in ext4_ext_binsearch_idx()) and
couldn't find a situation where it could.  And running the fstests with
that check didn't show any problem.  But yeah, my understanding of the
whole code is far from perfect.

Cheers,
--
Luís