linux-kernel - Re: [PATCH 1/4] btrfs: fix 64bit division in btrfs_insert_striped_mirrored_raid

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] [day] [month] [year] [list]

Date:   Wed, 20 Sep 2023 17:34:03 +0200
From:   David Sterba <dsterba@...e.cz>
To:     Qu Wenruo <quwenruo.btrfs@....com>
Cc:     dsterba@...e.cz, Johannes Thumshirn <Johannes.Thumshirn@....com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, Qu Wenru <wqu@...e.com>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] btrfs: fix 64bit division in
 btrfs_insert_striped_mirrored_raid_extents

On Wed, Sep 20, 2023 at 07:20:49AM +0930, Qu Wenruo wrote:
> >>>>> What if the quotient does not fit in a signed 32-bit value?
> >>>>
> >>>> Then you've bought a lot of HDDs ;-)
> >>>>
> >>>> Jokes aside, yes this is theoretically correct. Dave can you fix
> >>>> max_stripes up to be u64 when applying?
> >>>
> >>> I think we can keep it int, or unsigned int if needed, we can't hit such
> >>> huge values for rw_devices. The 'theoretically' would fit for a machine
> >>> with infinite resources, otherwise the maximum number of devices I'd
> >>> expect is a few thousand.
> >>
> >> In fact, we already have an check in btrfs_validate_super(), if the
> >> num_devices is over 1<<31, we would reject the fs.
> >
> > No, it's just a warning in that case.
> 
> We can make it a proper reject.
> 
> >
> >> I think we should be safe to further reduce the threshold.
> >>
> >> U16_MAX sounds a valid and sane value to me.
> >> If no rejection I can send out a patch for this.
> >>
> >> And later change internal rw_devices/num_devices to u16.
> >
> > U16 does not make sense here, it's not a native int type on many
> > architectures and generates awkward assembly code. We use it in
> > justified cases where it's saving space in structures that are allocated
> > thousand times. The arbitrary limit 65536 is probably sane but not
> > much different than 1<<31, practically not hit and was useful to
> > note fuzzed superblocks.
> 
> OK, we can make it unsigned int (mostly u32) for fs_info::*_devices, but
> still do extra limits on things like device add to limit it to U16_MAX.
> 
> Would this be a better solution?
> At least it would still half the width while keep it native to most (if
> not all) archs.

I don't see much point changing it from u64, it copies the on-disk type,
we validate the value on input, then use it as an int type. There are
not even theoretical problems stemming from the type width. With the
validations in place we don't need to add any artificial limits to the
number of devices, like we don't add such limitations elsewhere if not
necessary.