linux-kernel - Re: [PATCH RFC 0/4] Discuss to add return value in hfs_bnode_read* and hfs_brec

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: <c9814854f3b91699fd682a93b8379e8690a06d83.camel@dubeyko.com>
Date: Tue, 26 Aug 2025 12:21:53 -0700
From: Viacheslav Dubeyko <slava@...eyko.com>
To: Chenzhi Yang <yang.chenzhi@...o.com>, glaubitz@...sik.fu-berlin.de, 
	frank.li@...o.com
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 0/4] Discuss to add return value in hfs_bnode_read*
 and hfs_brec_lenoff

On Tue, 2025-08-26 at 11:35 +0800, Chenzhi Yang wrote:
> From: Yang Chenzhi <yang.chenzhi@...o.com>
> 
> Hello,
> 
> This patchset addresses two issues:
> 
> 1. Unchecked offset/length leading to out-of-bounds memory access. 
>    syzbot has reported such a bug in hfs_bmap_alloc, and
> hfs_bmap_free
>    has a similar potential problem.  
> 
>    To mitigate this, I added offset/length validation in
> `hfs_brec_lenoff`.
> 
>    This ensures callers always receive valid parameters, and in case
> of
>    invalid values, an error code will be returned instead of risking
>    memory corruption.
> 
> 2. Use of uninitialized memory due to early return in hfs_bnode_read.
> 
>    Recent commits have introduced offset/length validation in
> hfs_bnode_read. 
>    However, when an invalid offset is detected, the function
> currently 
>    returns early without initializing the provided buffer.
> 
>    This leads to a scenario where hfs_bnode_read_u16 may call
> be16_to_cpu
>    on uninitialized data.
> 
> While there could be multiple ways to fix these issues, adding proper
> error return values to the affected functions seems the safest
> solution.
> 
> However, this approach would require substantial changes across the
> codebase. In this patch, I only modified a small example function to
> illustrate the idea and seek feedback from the community: 
> Should we move forward with this direction and extend it more
> broadly?
> 
> In addition, this patch allows xfstests generic/113 to pass, which
> previously failed.
> 
> Yang Chenzhi (4):
>   hfs: add hfs_off_and_len_is_valid helper
>   hfs: introduce __hfs_bnode_read* to fix KMSAN uninit-value
>   hfs: restruct hfs_bnode_read
>   hfs: restructure hfs_brec_lenoff into a returned-value version
> 
>  fs/hfs/bfind.c | 12 +++----
>  fs/hfs/bnode.c | 87 +++++++++++++++++++++++++++++++++++-------------
> --
>  fs/hfs/brec.c  | 12 +++++--
>  fs/hfs/btree.c | 21 +++++++++---
>  fs/hfs/btree.h | 22 ++++++++++++-
>  5 files changed, 115 insertions(+), 39 deletions(-)

Frankly speaking, I don't see the point to split this fix on several
patches. It's really hard to understand the logic and correctness of
the fix in the current state of patchset. Could you please resend the
fix as one patch? It's not the big modification and one patch will be
more easy to review. Also, as far as I can see, patches depend on each
other and one patch will be more safe than the patchset.

Thanks,
Slava.