linux-kernel - Re: [syzbot] [hfs?] WARNING in hfs_write

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: <CAHk-=wi8XyAUF9_z6-oa4Ava6PVZeE-=TVNcFK1puQHpOtqLLw@mail.gmail.com>
Date:   Wed, 4 Jan 2023 11:06:28 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     syzbot <syzbot+7bb7cd3595533513a9e7@...kaller.appspotmail.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        christian.brauner@...ntu.com,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        jlayton@...nel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        Matthew Wilcox <willy@...radead.org>,
        ZhangPeng <zhangpeng362@...wei.com>
Subject: Re: [syzbot] [hfs?] WARNING in hfs_write_inode

On Wed, Jan 4, 2023 at 6:43 AM Arnd Bergmann <arnd@...db.de> wrote:
>
> My patch was a mechanical conversion from '/* panic? */'
> to 'WARN_ON()' to work around a compiler warning,
> and the previous code had been in there since the
> 2004 HFS rewrite by Roman Zippel.

Yeah. Looking at the code, the warning does make sense, in that the
code then does a

   hfs_bnode_write(...)

with the updated data, using the size of "struct hfs_cat_file", so
checking that the entry has that length does seem somewhat sensible.

At the same time, the HFS_IS_RSRC(inode) case in the same function
never had that "panic ?" thing, and obviously the two cases that *do*
have the check never actually did anything (since 2004, as you point
out - you have to go back to before the git days to even see any
development in this area).

> I know nothing about what this function actually does,
> so my best answer is that we could revert my patch
> and use pr_debug() instead of WARN_ON() for all of these.

Looks like this is syzbot just mounting a garbage image (or is it
actually some real hfs thing?)

I'm not sure a pr_debug() would even be appropriate. I think "return
-EIO" (but with a hfs_find_exit()) is likely the right answer. We've
done that in the past (see commit 8d824e69d9f3: "hfs: fix OOB Read in
__hfs_brec_find").

I suspect this code is basically all dead. From what I can tell, hfs
only gets updates for

 (a) syzbot reports

 (b) vfs interface changes

and the last real changes seem to have been by Ernesto A. Fernández
back in 2018.

Hmm. Looking at that code, we have another bug in there, introduced by
an earlier fix for a similar issue: commit 8d824e69d9f3 ("hfs: fix OOB
Read in __hfs_brec_find") added

+       if (HFS_I(main_inode)->cat_key.CName.len > HFS_NAMELEN)
+               return -EIO;

but it's after hfs_find_init(), so it should actually have done a
hfs_find_exit() to not leak memory.

So we should probably fix that too.

Something like this ENTIRELY UNTESTED patch?

Do we have anybody who looks at hfs?

                      Linus

View attachment "patch.diff" of type "text/x-patch" (2026 bytes)