| 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
| ||
|
Message-ID: <20240814192642.GZ25962@twin.jikos.cz>
Date: Wed, 14 Aug 2024 21:26:42 +0200
From: David Sterba <dsterba@...e.cz>
To: Thorsten Blum <thorsten.blum@...lux.com>
Cc: Nathan Chancellor <nathan@...nel.org>,
Johannes Thumshirn <Johannes.Thumshirn@....com>,
"clm@...com" <clm@...com>,
"josef@...icpanda.com" <josef@...icpanda.com>,
"dsterba@...e.com" <dsterba@...e.com>,
"kees@...nel.org" <kees@...nel.org>,
"gustavoars@...nel.org" <gustavoars@...nel.org>,
"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: Re: [PATCH] btrfs: Annotate structs with __counted_by()
On Wed, Aug 14, 2024 at 09:01:42PM +0200, Thorsten Blum wrote:
> On 14. Aug 2024, at 20:02, Nathan Chancellor <nathan@...nel.org> wrote:
> > On Mon, Aug 12, 2024 at 02:03:44PM +0200, Thorsten Blum wrote:
> >> On 12. Aug 2024, at 12:54, Johannes Thumshirn <Johannes.Thumshirn@....com> wrote:
> >>> On 12.08.24 12:37, Thorsten Blum wrote:
> >>>> Add the __counted_by compiler attribute to the flexible array member
> >>>> stripes to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> >>>> CONFIG_FORTIFY_SOURCE.
> >>>>
> >>>> Signed-off-by: Thorsten Blum <thorsten.blum@...lux.com>
> >>>> ---
> >>>> fs/btrfs/volumes.h | 4 ++--
> >>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> >>>> index 37a09ebb34dd..f28fa318036b 100644
> >>>> --- a/fs/btrfs/volumes.h
> >>>> +++ b/fs/btrfs/volumes.h
> >>>> @@ -551,7 +551,7 @@ struct btrfs_io_context {
> >>>> * stripes[data_stripes + 1]: The Q stripe (only for RAID6).
> >>>> */
> >>>> u64 full_stripe_logical;
> >>>> - struct btrfs_io_stripe stripes[];
> >>>> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes);
> >>>> };
> >>>>
> >>>> struct btrfs_device_info {
> >>>> @@ -591,7 +591,7 @@ struct btrfs_chunk_map {
> >>>> int io_width;
> >>>> int num_stripes;
> >>>> int sub_stripes;
> >>>> - struct btrfs_io_stripe stripes[];
> >>>> + struct btrfs_io_stripe stripes[] __counted_by(num_stripes);
> >>>> };
> >>>>
> >>>> #define btrfs_chunk_map_size(n) (sizeof(struct btrfs_chunk_map) + \
> >>>
> >>> Looks good to me,
> >>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@....com>
> >>>
> >>> Out of curiosity, have you encountered any issues with this patch applied?
> >>
> >> I only compile-tested it.
> >
> > This change is now in next-20240814 and I see a UBSAN warning at runtime
> > as a result because the assignment of ->num_stripes happens after
> > accessing ->stripes[] (which breaks one of the requirements for using
> > __counted_by [1]), meaning that UBSAN thinks this is a zero sized array
> > due to bioc being allocated with kzalloc().
> >
> > [ 24.992264] ------------[ cut here ]------------
> > [ 25.009196] UBSAN: array-index-out-of-bounds in fs/btrfs/volumes.c:6602:11
> > [ 25.021963] index 1 is out of range for type 'struct btrfs_io_stripe[] __counted_by(num_stripes)' (aka 'struct btrfs_io_stripe[]')
> > [ 25.036463] CPU: 28 UID: 0 PID: 1171 Comm: systemd-random- Not tainted 6.11.0-rc3-next-20240814 #1
> > [ 25.048172] Hardware name: ADLINK Ampere Altra Developer Platform/Ampere Altra Developer Platform, BIOS TianoCore 2.04.100.11 (SYS: 2.06.20220308) 11/06/2
> > [ 25.064754] Call trace:
> > [ 25.069965] dump_backtrace+0x114/0x19c
> > [ 25.076564] show_stack+0x28/0x3c
> > [ 25.082642] dump_stack_lvl+0x48/0x94
> > [ 25.089068] __ubsan_handle_out_of_bounds+0x10c/0x184
> > [ 25.096883] btrfs_map_block+0x540/0xb3c
> > [ 25.103570] btrfs_submit_bio+0xf8/0x654
> > [ 25.110256] write_one_eb+0x290/0x444
> > [ 25.116682] btree_write_cache_pages+0x44c/0x5a8
> > [ 25.124063] btree_writepages+0x2c/0x8c
> > [ 25.130662] do_writepages+0x10c/0x34c
> > [ 25.137175] filemap_fdatawrite_wbc+0x84/0xb0
> > [ 25.144295] filemap_fdatawrite_range+0x74/0xac
> > [ 25.151589] btrfs_write_marked_extents+0xa0/0x140
> > [ 25.159143] btrfs_sync_log+0x298/0xa98
> > [ 25.165743] btrfs_sync_file+0x440/0x608
> > [ 25.172429] __arm64_sys_fsync+0x90/0xd4
> > [ 25.179115] invoke_syscall+0x8c/0x11c
> > [ 25.185628] el0_svc_common
> > [ 25.191185] do_el0_svc+0x2c/0x3c
> > [ 25.197264] el0_svc+0x48/0xf0
> > [ 25.203083] el0t_64_sync_handler+0x98/0x108
> > [ 25.210118] el0t_64_sync+0x19c/0x1a0
> > [ 25.216552] ---[ end trace ]---
> >
> > The fix might be as simple as something like
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 4a259bdaa21c..0cabc2ebde71 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -6561,6 +6561,7 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> > }
> > bioc->map_type = map->type;
> >
> > + bioc->num_stripes = io_geom.num_stripes;
> > /*
> > * For RAID56 full map, we need to make sure the stripes[] follows the
> > * rule that data stripes are all ordered, then followed with P and Q
> > @@ -6621,7 +6622,6 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> > }
> >
> > *bioc_ret = bioc;
> > - bioc->num_stripes = io_geom.num_stripes;
> > bioc->max_errors = io_geom.max_errors;
> > bioc->mirror_num = io_geom.mirror_num;
> >
> >
> > but I am not sure of the implications of this change on quick glance
> > with regards to error handling and such.
> >
> > [1]: https://people.kernel.org/gustavoars/how-to-use-the-new-counted_by-attribute-in-c-and-linux
> >
> > Cheers,
> > Nathan
>
> My patch should probably be reverted as I somehow missed quite a few
> things and need more time to rework this properly.
>
> Sorry about that and thanks for reporting this!
Patch removed from for-next.
Powered by blists - more mailing lists