linux-ext4 - Re: [PATCH] ext4: fix divide error in ext4_mb_regular

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zrw4/N7a/XrjOgtS@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Wed, 14 Aug 2024 10:26:47 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Edward Adam Davis <eadavis@...com>
Cc: syzbot+1ad8bac5af24d01e2cbd@...kaller.appspotmail.com,
        adilger.kernel@...ger.ca, jack@...e.cz, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        tytso@....edu
Subject: Re: [PATCH] ext4: fix divide error in ext4_mb_regular_allocator

On Wed, Aug 14, 2024 at 10:12:00AM +0800, Edward Adam Davis wrote:
> Before determining that the goal length is a multiple of the stripe size,
> check CR_GOAL_LEN_FAST and CR_BEST_AVAIL_LEN first.
> 
> Fixes: 1f6bc02f1848 ("ext4: fallback to complex scan if aligned scan doesn't work")
> Reported-and-tested-by: syzbot+1ad8bac5af24d01e2cbd@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1ad8bac5af24d01e2cbd
> Signed-off-by: Edward Adam Davis <eadavis@...com>
> ---
>  fs/ext4/mballoc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 9dda9cd68ab2..451f92cde461 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2928,13 +2928,12 @@ ext4_mb_regular_allocator(struct ext4_allocation_context *ac)
>       if (cr == CR_POWER2_ALIGNED)
>         ext4_mb_simple_scan_group(ac, &e4b);
>       else {
> -       bool is_stripe_aligned = sbi->s_stripe &&
> +       bool is_stripe_aligned = (cr == CR_GOAL_LEN_FAST ||
> +         cr == CR_BEST_AVAIL_LEN) && sbi->s_stripe &&
>           !(ac->ac_g_ex.fe_len %
>             EXT4_B2C(sbi, sbi->s_stripe));

Hi Edward, 

Thanks for the patch. So I didn't get a chance to look at syszcaller
report but assuming that EXT4_B2C(sbi, sbi->s_stripe) is becoming 0,
I'm not understanding how is this patch fixing the bug?

It just seems to short circuit the actual bug but we might still hit it
right? 

As for EXT4_B2C(stripe) becoming zero, I have 2 observations:

1. We should definitely be using EXT4_NUM_B2C() here to make sure we
   don't get 0 if stripe is less than cluster size.

2. That being saidIm not sure if it's even possible for this to become zero
   because we do check that stripe size is a multiple of cluster size in
   ext4_fill_super, else we disable it:

  /*
   * It's hard to get stripe aligned blocks if stripe is not aligned with
   * cluster, just disable stripe and alert user to simpfy code and avoid
   * stripe aligned allocation which will rarely successes.
   */
  if (sbi->s_stripe > 0 && sbi->s_cluster_ratio > 1 &&
      sbi->s_stripe % sbi->s_cluster_ratio != 0) {
    ext4_msg(sb, KERN_WARNING,
       "stripe (%lu) is not aligned with cluster size (%u), "
       "stripe is disabled",
       sbi->s_stripe, sbi->s_cluster_ratio);
    sbi->s_stripe = 0;
  }

I'll try to look into this a bit more and get back. Do let me know
if I missed anything.

Regards,
Ojaswin

>  
> -       if ((cr == CR_GOAL_LEN_FAST ||
> -            cr == CR_BEST_AVAIL_LEN) &&
> -           is_stripe_aligned)
> +       if (is_stripe_aligned)
>           ext4_mb_scan_aligned(ac, &e4b);
>  
>         if (ac->ac_status == AC_STATUS_CONTINUE)
> -- 
> 2.43.0
>