linux-kernel - Re: [PATCH] hfsplus: fix missing hfs_bnode_get() in hfs_bnode

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] [day] [month] [year] [list]

Message-ID: <966687093123e00c166afabc0a9de87e0ba844d5.camel@mpiricsoftware.com>
Date: Wed, 24 Dec 2025 17:30:30 +0530
From: Shardul Bankar <shardul.b@...ricsoftware.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>, "zippel@...ux-m68k.org"
	 <zippel@...ux-m68k.org>, "glaubitz@...sik.fu-berlin.de"
	 <glaubitz@...sik.fu-berlin.de>, "linux-fsdevel@...r.kernel.org"
	 <linux-fsdevel@...r.kernel.org>, "slava@...eyko.com" <slava@...eyko.com>, 
	"frank.li@...o.com"
	 <frank.li@...o.com>
Cc: "akpm@...l.org" <akpm@...l.org>, "janak@...ricsoftware.com"
	 <janak@...ricsoftware.com>, "linux-kernel@...r.kernel.org"
	 <linux-kernel@...r.kernel.org>, shardulsb08@...il.com
Subject: Re:  [PATCH] hfsplus: fix missing hfs_bnode_get() in
 hfs_bnode_create()

On Tue, 2025-12-16 at 20:28 +0000, Viacheslav Dubeyko wrote:
> 
> The fix in hfs_bmap_alloc() sounds reasonable to me. But I don't see
> the point
> of adding hfs_bnode_get() in hfs_bnode_create() for the case of
> erroneous
> situation [1]:
> 
>         if (node) {
>                 pr_crit("new node %u already hashed?\n", num);
>                 WARN_ON(1);
>                 return node;
>         }
> 
> It will be much better to return ERR_PTR(-EEXIST) here. Because, it
> is not
> situation of "doing business as usual". We should not continue to
> believe that
> "sun is shining for us", but we should stop the logic somehow.
> 
> Thanks,
> Slava.
> 
> [1] https://elixir.bootlin.com/linux/v6.18/source/fs/hfs/bnode.c#L518

Hi Slava,

Thanks, agreed.

I’ll keep the hfs_bmap_alloc() change to ensure node 0 is never
allocated.

And I agree that the “already hashed?” case in hfs_bnode_create()
should not try to continue by returning a pointer (even with an extra
hfs_bnode_get()). Callers like hfs_btree_inc_height() and
hfs_bnode_split() treat the returned node as a freshly allocated node
and immediately rewrite its header/records. If hfs_bnode_create()
returns an existing hashed node, that effectively overwrites live node
contents and amplifies corruption, which can then cascade into later
failures.

So I’ll rework v2 as a small series:
1/2: guard against allocating node 0 in hfs_bmap_alloc()
2/2: make the “already hashed?” path return ERR_PTR(-EEXIST) and
propagate the error

I’ll send the updated series shortly.

Thanks,
Shardul