Re: [PATCH] hfs: validate bitmap record offset in hfs_bmap_alloc

[Viacheslav Dubeyko <vdubeyko@redhat.com>]

On Sat, 2026-04-18 at 13:39 +0000, Tristan Madani wrote:
> hfs_bmap_alloc() retrieves the bitmap record from the header node
> using hfs_brec_lenoff() but does not validate the returned offset
> and length before using them to compute page pointers.  On a crafted
> HFS image with corrupted B-tree data, the offset can exceed the node
> size, causing an out-of-bounds page access.
> 
> Additionally, when the bitmap has node 0 bit incorrectly unset,
> hfs_bmap_alloc() calls hfs_bnode_create(tree, 0) for the already-
> hashed header node, triggering a WARN_ON and returning the existing
> node without incrementing its reference count.
> 
> Port the fixes already applied to HFS+ (commits d8a73cc46c84 and
> 738d5a51864e) to the HFS side:
> 
> 1. Move is_bnode_offset_valid() and check_and_correct_requested_length()
>    from bnode.c to btree.h so they can be used by btree.c.
> 
> 2. Validate the record offset and length in hfs_bmap_alloc() before
>    computing page pointers, preventing out-of-bounds access.
> 
> 3. Return ERR_PTR(-EEXIST) from hfs_bnode_create() when the node is
>    already hashed, properly signaling filesystem corruption to callers.

I assume that we have the second version of the patch again. Am I correct?

> Reported-by: syzbot+a19ca73b21fe8bc69101@syzkaller.appspotmail.com
> Tested-by: syzbot+a19ca73b21fe8bc69101@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=a19ca73b21fe8bc69101
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
> ---
>  fs/hfs/bnode.c | 44 +-------------------------------------------
>  fs/hfs/btree.c |  6 ++++++
>  fs/hfs/btree.h | 42 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> index 13d58c51fc46b..26c2e65ab5935 100644
> --- a/fs/hfs/bnode.c
> +++ b/fs/hfs/bnode.c
> @@ -15,48 +15,6 @@
>  
>  #include "btree.h"
>  
> -static inline
> -bool is_bnode_offset_valid(struct hfs_bnode *node, u32 off)
> -{
> -	bool is_valid = off < node->tree->node_size;
> -
> -	if (!is_valid) {
> -		pr_err("requested invalid offset: "
> -		       "NODE: id %u, type %#x, height %u, "
> -		       "node_size %u, offset %u\n",
> -		       node->this, node->type, node->height,
> -		       node->tree->node_size, off);
> -	}
> -
> -	return is_valid;
> -}
> -
> -static inline
> -u32 check_and_correct_requested_length(struct hfs_bnode *node, u32 off, u32 len)
> -{
> -	unsigned int node_size;
> -
> -	if (!is_bnode_offset_valid(node, off))
> -		return 0;
> -
> -	node_size = node->tree->node_size;
> -
> -	if ((off + len) > node_size) {
> -		u32 new_len = node_size - off;
> -
> -		pr_err("requested length has been corrected: "
> -		       "NODE: id %u, type %#x, height %u, "
> -		       "node_size %u, offset %u, "
> -		       "requested_len %u, corrected_len %u\n",
> -		       node->this, node->type, node->height,
> -		       node->tree->node_size, off, len, new_len);
> -
> -		return new_len;
> -	}
> -
> -	return len;
> -}
> -
>  void hfs_bnode_read(struct hfs_bnode *node, void *buf, u32 off, u32 len)
>  {
>  	struct page *page;
> @@ -518,7 +476,7 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
>  	if (node) {
>  		pr_crit("new node %u already hashed?\n", num);
>  		WARN_ON(1);
> -		return node;
> +		return ERR_PTR(-EEXIST);
>  	}
>  	node = __hfs_bnode_create(tree, num);
>  	if (!node)
> diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
> index 2eb37a2f64e86..e8bc24c8baf1a 100644
> --- a/fs/hfs/btree.c
> +++ b/fs/hfs/btree.c
> @@ -304,6 +304,12 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
>  	len = hfs_brec_lenoff(node, 2, &off16);
>  	off = off16;
>  
> +	if (!is_bnode_offset_valid(node, off)) {
> +		hfs_bnode_put(node);
> +		return ERR_PTR(-EIO);
> +	}
> +	len = check_and_correct_requested_length(node, off, len);
> +
>  	off += node->page_offset;
>  	pagep = node->page + (off >> PAGE_SHIFT);
>  	data = kmap_local_page(*pagep);
> diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
> index 99be858b24465..6032b14b1639d 100644
> --- a/fs/hfs/btree.h
> +++ b/fs/hfs/btree.h
> @@ -85,6 +85,48 @@ struct hfs_find_data {
>  };
>  
>  
> +static inline
> +bool is_bnode_offset_valid(struct hfs_bnode *node, u32 off)
> +{
> +	bool is_valid = off < node->tree->node_size;
> +
> +	if (!is_valid) {
> +		pr_err("requested invalid offset: "
> +		       "NODE: id %u, type %#x, height %u, "
> +		       "node_size %u, offset %u\n",
> +		       node->this, node->type, node->height,
> +		       node->tree->node_size, off);
> +	}
> +
> +	return is_valid;
> +}
> +
> +static inline
> +u32 check_and_correct_requested_length(struct hfs_bnode *node, u32 off, u32 len)
> +{
> +	unsigned int node_size;
> +
> +	if (!is_bnode_offset_valid(node, off))
> +		return 0;
> +
> +	node_size = node->tree->node_size;
> +
> +	if ((off + len) > node_size) {
> +		u32 new_len = node_size - off;
> +
> +		pr_err("requested length has been corrected: "
> +		       "NODE: id %u, type %#x, height %u, "
> +		       "node_size %u, offset %u, "
> +		       "requested_len %u, corrected_len %u\n",
> +		       node->this, node->type, node->height,
> +		       node->tree->node_size, off, len, new_len);
> +
> +		return new_len;
> +	}
> +
> +	return len;
> +}
> +
>  /* btree.c */
>  extern struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id,
>  					btree_keycmp keycmp);

This modification makes sense but I mean the porting of slightly different
functionality. Are you working on it? Or have you missed my point?

Thanks,
Slava.