Message-ID: <ZJQZm+UGyJZZNTvN@dread.disaster.area>
Date:   Thu, 22 Jun 2023 19:51:23 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     syzbot <syzbot+510dcbdc6befa1e6b2f6@...kaller.appspotmail.com>,
        djwong@...nel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org,
        syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [xfs?] UBSAN: array-index-out-of-bounds in
 xfs_attr3_leaf_add_work

On Wed, Jun 21, 2023 at 01:05:21AM -0700, Eric Biggers wrote:
> On Mon, Jun 19, 2023 at 12:02:41PM +1000, 'Dave Chinner' via syzkaller-bugs wrote:
> > > XFS (loop0): Mounting V4 Filesystem 5e6273b8-2167-42bb-911b-418aa14a1261
> > > XFS (loop0): Ending clean mount
> > > xfs filesystem being mounted at /root/file0 supports timestamps until 2038-01-19 (0x7fffffff)
> > > ================================================================================
> > > UBSAN: array-index-out-of-bounds in fs/xfs/libxfs/xfs_attr_leaf.c:1560:3
> > > index 14 is out of range for type '__u8 [1]'
> > > CPU: 1 PID: 5021 Comm: syz-executor198 Not tainted 6.4.0-rc6-next-20230613-syzkaller #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/25/2023
> > > Call Trace:
> > >  <TASK>
> > >  __dump_stack lib/dump_stack.c:88 [inline]
> > >  dump_stack_lvl+0x136/0x150 lib/dump_stack.c:106
> > >  ubsan_epilogue lib/ubsan.c:217 [inline]
> > >  __ubsan_handle_out_of_bounds+0xd5/0x140 lib/ubsan.c:348
> > >  xfs_attr3_leaf_add_work+0x1528/0x1730 fs/xfs/libxfs/xfs_attr_leaf.c:1560
> > >  xfs_attr3_leaf_add+0x750/0x880 fs/xfs/libxfs/xfs_attr_leaf.c:1438
> > >  xfs_attr_leaf_try_add+0x1b7/0x660 fs/xfs/libxfs/xfs_attr.c:1242
> > >  xfs_attr_leaf_addname fs/xfs/libxfs/xfs_attr.c:444 [inline]
> > >  xfs_attr_set_iter+0x16c4/0x2f90 fs/xfs/libxfs/xfs_attr.c:721
> > >  xfs_xattri_finish_update+0x3c/0x140 fs/xfs/xfs_attr_item.c:332
> > 
> > The on disk format for this field is defined as:
> > 
> > typedef struct xfs_attr_leaf_name_local {
> >         __be16  valuelen;               /* number of bytes in value */
> >         __u8    namelen;                /* length of name bytes */
> >         __u8    nameval[1];             /* name/value bytes */
> > } xfs_attr_leaf_name_local_t
> > 
> > If someone wants to do change the on-disk format definition to use
> > "kernel proper" flex arrays in both the kernel code and user space,
> > update all the documentation and do all the validation work that
> > on-disk format changes require for all XFS disk structures that are
> > defined this way, then we'll fix this.
> > 
> > But as it stands, these structures have been defined this way for 25
> > years and the code accessing them has been around for just as long.
> > The code is not broken and it does not need fixing. We have way more
> > important things to be doing that fiddling with on disk format
> > definitions and long standing, working code just to shut up UBSAN
> > and/or syzbot.
> > 
> > WONTFIX, NOTABUG.
> 
> My understanding is that the main motivation for the conversions to flex arrays
> is kernel hardening, as it allows bounds checking to be enabled.

<sigh>

Do you think we don't know this?

We can't actually rely on the compiler checking here. We *must* to
do runtime verification of these on-disk format structures because
we are parsing dynamic structures, not fixed size arrays.  IOWs, we
already bounds check these arrays (in multiple ways!) before we do
the memcpy(), and have done so for many, many years.

*This code is safe* no matter what the compiler says.

Last time this came up in a FORTIFY_SOURCE context, Darrick proposed
to change this to use unsafe_memcpy() to switch off the compile time
checking because we *must* do runtime checking of the array lengths
provided in the structure itself.

Kees pretty much knocked that down by instead proposing some
"flex_cpy()" API he was working on that never went anywhere. So
kernel security people essentially said "no, we don't want you to
fix it that way, but then failed to provide the new infrastructure
they said we should use for this purpose.

Damned if we do, damned if we don't.

So until someone from the security side of the fence ponies up the
resources to fix this in a way that is acceptible to the security
people and they do all the testing and validation we require for
such an on-disk format change, the status quo is unlikely to change.

> You can probably get away with not fixing this for a little while longer, as
> that stuff is still a work in progress.  But I would suggest you be careful
> about potentially getting yourself into a position where XFS is blocking
> enabling security mitigations for the whole kernel...

<sigh>

I'm really getting tired of all these "you'll do what we say or
else" threats that have been made over the past few months.

As I said: if this is a priority, then the entities who have given
it priority need to provide resources to fix it, not demand that
others do the work they want done right now for free. If anyone
wants work done for free, then they'll just have to wait in line
until we've got time to address it.

-Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists