linux-kernel - RE: [PATCH v3] hfs: remove BUG() from hfs_release_folio()/hfs_test_inode()/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: <da34b1af424c855519b3e926c7bc891a338c327c.camel@ibm.com>
Date: Fri, 25 Jul 2025 17:45:29 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "penguin-kernel@...ove.SAKURA.ne.jp" <penguin-kernel@...ove.SAKURA.ne.jp>,
        "willy@...radead.org" <willy@...radead.org>
CC: "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "frank.li@...o.com" <frank.li@...o.com>,
        "slava@...eyko.com"
	<slava@...eyko.com>,
        "linux-fsdevel@...r.kernel.org"
	<linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>
Subject: RE: [PATCH v3] hfs: remove BUG() from
 hfs_release_folio()/hfs_test_inode()/hfs_write_inode()

On Fri, 2025-07-25 at 08:20 +0900, Tetsuo Handa wrote:
> On 2025/07/25 7:05, Tetsuo Handa wrote:
> > > > But I can't be convinced that above change is sufficient, for if I do
> > > > 
> > > > +	            static u8 serial;
> > > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > > +                       inode->i_ino = (serial++) % 16;
> > > 
> > > I don't see the point in flags introduction. It makes logic very complicated.
> > 
> > The point of this change is to excecise inode->i_ino for all values between 0 and 15.
> > Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then,
> 
> Background: I assume that the value of rec->dir.DirID comes from the hfs filesystem image in the
> reproducer (i.e. memfd file associated with /dev/loop0 ). But since I don't know the offset to modify
> the value if I want the reproducer to pass rec->dir.DirID == 1...15 instead of rec->dir.DirID == 0,
> I am modifying inode->i_ino here when rec->dir.DirID == 0.
> 
> > 
> > > 
> > > > 
> > > > instead of
> > > > 
> > > > +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> > > > +                       make_bad_inode(inode);
> > > > 
> > > > , the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
> > > > because hfs_write_inode() handles only 2, 3 and 4.
> > > > 
> > > 
> > > How can we go into hfs_write_inode() if we created the bad inode for invalid
> > > inode ID? How is it possible?
> 
> Calling make_bad_inode() for some of values between 0...15 at hfs_read_inode() will prevent
> that inode from going into hfs_write_inode(). But regarding the values between 0...15 which
> were not calling make_bad_inode() at hfs_read_inode() will not prevent that inode from going
> into hfs_write_inode().
> 
> Since hfs_write_inode() calls BUG() for values 0...15 except 2...4, any values between 0...15
> except 2...4 which were not calling make_bad_inode() at hfs_read_inode() will hit BUG().
> 
> If we don't remove BUG(), the values which hfs_read_inode() does not need to call
> make_bad_inode() will be limited to only 2...4.
> 
> And since you say that hfs_read_inode() should call make_bad_inode() for 3...4, the only value
> hfs_read_inode() can accept (from the point of view of avoid hitting BUG() in hfs_write_inode())
> will be 2.
> 
> > 
> > are all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 invalid value for hfs_read_inode() ?
> > 
> > If all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 are invalid value for hfs_read_inode(),
> > and 3 and 4 are also invalid value for hfs_read_inode(), hfs_read_inode() would accept only 2.
> > Something is crazily wrong.
> > 
> > Can we really filter some of values between 0 and 15 at hfs_read_inode() ?
> > 
> 
> Can the attempt to filter some of values between 0 and 15 at hfs_read_inode() make sense,
> without the attempt to remove BUG() from hfs_write_inode() ?
> 
> I think that we need to remove BUG() from hfs_write_inode(), even if you try to filter
> at hfs_read_inode().

If we manage the inode IDs properly in hfs_read_inode(), then hfs_write_inode()
never will receive the invalid inode ID. I don't see the point to remove the
BUG() in hfs_write_inode().

Thanks,
Slava.