[syzbot] [xfs?] UBSAN: array-index-out-of-bounds in xfs_attr3_leaf_add_work

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 1f6ce8392d6f Add linux-next specific files for 20230613
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=14e629dd280000
kernel config: https://syzkaller.appspot.com/x/.config?x=d103d5f9125e9fe9
dashboard link: https://syzkaller.appspot.com/bug?extid=510dcbdc6befa1e6b2f6
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=139d8d2d280000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11b371f1280000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/2d9bf45aeae9/disk-1f6ce839.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/e0b03ef83e17/vmlinux-1f6ce839.xz
kernel image: https://storage.googleapis.com/syzbot-assets/b6c21a24174d/bzImage-1f6ce839.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/65eca6891c21/mount_0.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+510dcb...@syzkaller.appspotmail.com

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
xfs_attr_finish_item+0x6d/0x280 fs/xfs/xfs_attr_item.c:463
xfs_defer_finish_one fs/xfs/libxfs/xfs_defer.c:481 [inline]
xfs_defer_finish_noroll+0x93b/0x1f20 fs/xfs/libxfs/xfs_defer.c:565
__xfs_trans_commit+0x566/0xe20 fs/xfs/xfs_trans.c:972
xfs_attr_set+0x12e5/0x2220 fs/xfs/libxfs/xfs_attr.c:1083
xfs_attr_change fs/xfs/xfs_xattr.c:106 [inline]
xfs_xattr_set+0xf2/0x1c0 fs/xfs/xfs_xattr.c:151
__vfs_setxattr+0x173/0x1e0 fs/xattr.c:201
__vfs_setxattr_noperm+0x129/0x5f0 fs/xattr.c:235
__vfs_setxattr_locked+0x1d3/0x260 fs/xattr.c:296
vfs_setxattr+0x143/0x340 fs/xattr.c:322
do_setxattr+0x147/0x190 fs/xattr.c:630
setxattr+0x146/0x160 fs/xattr.c:653
path_setxattr+0x197/0x1c0 fs/xattr.c:672
__do_sys_setxattr fs/xattr.c:688 [inline]
__se_sys_setxattr fs/xattr.c:684 [inline]
__x64_sys_setxattr+0xc4/0x160 fs/xattr.c:684
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f9effd537f9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 51 14 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffc33918058 EFLAGS: 00000246 ORIG_RAX: 00000000000000bc
RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007f9effd537f9
RDX: 0000000020000680 RSI: 0000000020000200 RDI: 0000000020000000
RBP: 00007f9effd13090 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000058 R11: 0000000000000246 R12: 00007f9effd13120
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>
================================================================================


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Dave Chinner]

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.

-Dave.
--
Dave Chinner
da...@fromorbit.com

[Eric Biggers]

Hi Dave,

My understanding is that the main motivation for the conversions to flex arrays
is kernel hardening, as it allows bounds checking to be enabled.

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...

- Eric

[Dave Chinner]

<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.

[Eric Biggers]

On Thu, Jun 22, 2023 at 07:51:23PM +1000, 'Dave Chinner' via syzkaller-bugs wrote:
> >
> > 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?

You said the only reason to fix this would be to "shut up UBSAN and/or syzbot".
So it seemed you were not aware of the kernel hardening motivation.

>
> 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.

I think you've missed the point. The point is not about improving the bounds
checks for this specific field. Rather, it's about eliminating a "false
positive" so that automatic bounds checking of known-sized arrays can be enabled
universally as a hardening measure. Without code like this fixed, it will be
impossible to enable automatic bounds checking, at least in the affected files.

>
> > 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.

Well, good news: Gustavo Silva, Kees Cook, and others are going through the
kernel and doing the conversions to flex arrays.

I am trying to help you understand the problem, not force you to fix it. If you
do not want to fix it yourself, then you can simply consider this bug report as
a heads up.

I would just ask that you please try to be cooperative when you eventually do
get a patch from someone trying to fix it.

XFS does not need to be the "problem subsystem" every time.

- Eric

[syzbot]

syzbot suspects this issue was fixed by commit:

commit a49bbce58ea90b14d4cb1d00681023a8606955f2
Author: Darrick J. Wong <djw...@kernel.org>
Date: Mon Jul 10 16:12:20 2023 +0000

xfs: convert flex-array declarations in xfs attr leaf blocks

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12eef28a680000
start commit: f8566aa4f176 Merge tag 'x86-urgent-2023-07-01' of git://gi..
git tree: upstream
kernel config: https://syzkaller.appspot.com/x/.config?x=3f27fb02fc20d955
dashboard link: https://syzkaller.appspot.com/bug?extid=510dcbdc6befa1e6b2f6
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1652938f280000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14c10c40a80000

If the result looks correct, please mark the issue as fixed by replying with:

#syz fix: xfs: convert flex-array declarations in xfs attr leaf blocks

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[Kees Cook]

On Sat, Sep 30, 2023 at 10:57:28AM -0700, syzbot wrote:
> syzbot suspects this issue was fixed by commit:
>
> commit a49bbce58ea90b14d4cb1d00681023a8606955f2
> Author: Darrick J. Wong <djw...@kernel.org>
> Date: Mon Jul 10 16:12:20 2023 +0000
>
> xfs: convert flex-array declarations in xfs attr leaf blocks
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12eef28a680000
> start commit: f8566aa4f176 Merge tag 'x86-urgent-2023-07-01' of git://gi..
> git tree: upstream
> kernel config: https://syzkaller.appspot.com/x/.config?x=3f27fb02fc20d955
> dashboard link: https://syzkaller.appspot.com/bug?extid=510dcbdc6befa1e6b2f6
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1652938f280000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14c10c40a80000
>
> If the result looks correct, please mark the issue as fixed by replying with:

Yup, that tracks. :)

#syz fix: xfs: convert flex-array declarations in xfs attr leaf blocks

-Kees

>
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection

--
Kees Cook