__hfs_ext_write_extent() does not report all failures while updating
the extents B-tree.

When inserting a new extent record, the return value of
hfs_brec_insert() is ignored and HFS_FLG_EXT_DIRTY and HFS_FLG_EXT_NEW
are cleared even if the insertion fails.

When updating an existing extent record, hfs_bnode_write() returns
void, so its caller cannot detect a rejected write. Validate the extent
record size and node range before calling hfs_bnode_write().

Propagate errors returned by hfs_brec_insert() and return -EIO for an
invalid existing extent record. Only clear the extent dirty flags after
a successful operation.

Fault injection confirmed both failure paths. Insertion errors are
propagated to the caller, and invalid existing-record writes are
rejected before hfs_bnode_write() without clearing the dirty state.

Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>

Changes in v2:
- Validate the existing extent record size and node range before
  calling hfs_bnode_write(), following review feedback.
- Return -EIO without clearing HFS_FLG_EXT_DIRTY when validation
  fails.
- Fault-injection tested the existing-record failure path. Before the
  change, hfs_bnode_write() rejected an invalid offset internally but
  __hfs_ext_write_extent() continued and cleared the dirty flag. With
  v2, the invalid write is rejected before hfs_bnode_write().

---
 fs/hfs/extent.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index f066a99a863b..13426503fbb3 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -121,12 +121,21 @@ static int __hfs_ext_write_extent(struct inode *inode, struct hfs_find_data *fd)
 		res = hfs_bmap_reserve(fd->tree, fd->tree->depth + 1);
 		if (res)
 			return res;
-		hfs_brec_insert(fd, HFS_I(inode)->cached_extents, sizeof(hfs_extent_rec));
+		res = hfs_brec_insert(fd, HFS_I(inode)->cached_extents,
+				      sizeof(hfs_extent_rec));
+		if (res)
+			return res;
 		HFS_I(inode)->flags &= ~(HFS_FLG_EXT_DIRTY|HFS_FLG_EXT_NEW);
 	} else {
 		if (res)
 			return res;
-		hfs_bnode_write(fd->bnode, HFS_I(inode)->cached_extents, fd->entryoffset, fd->entrylength);
+		if (fd->entrylength != sizeof(hfs_extent_rec) ||
+		    fd->entryoffset < 0 ||
+		    (u64)fd->entryoffset + fd->entrylength >
+			    fd->tree->node_size)
+			return -EIO;
+		hfs_bnode_write(fd->bnode, HFS_I(inode)->cached_extents,
+				fd->entryoffset, fd->entrylength);
 		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
 	}
 	return 0;
-- 
2.55.0

On Tue, 2026-09-22 at 20:40 -0300, Davy Felipe wrote:
> __hfs_ext_write_extent() does not report all failures while updating
> the extents B-tree.
> 
> When inserting a new extent record, the return value of
> hfs_brec_insert() is ignored and HFS_FLG_EXT_DIRTY and
> HFS_FLG_EXT_NEW
> are cleared even if the insertion fails.
> 
> When updating an existing extent record, hfs_bnode_write() returns
> void, so its caller cannot detect a rejected write. Validate the
> extent
> record size and node range before calling hfs_bnode_write().
> 
> Propagate errors returned by hfs_brec_insert() and return -EIO for an
> invalid existing extent record. Only clear the extent dirty flags
> after
> a successful operation.
> 
> Fault injection confirmed both failure paths. Insertion errors are
> propagated to the caller, and invalid existing-record writes are
> rejected before hfs_bnode_write() without clearing the dirty state.
> 
> Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>
> 
> Changes in v2:
> - Validate the existing extent record size and node range before
>   calling hfs_bnode_write(), following review feedback.
> - Return -EIO without clearing HFS_FLG_EXT_DIRTY when validation
>   fails.
> - Fault-injection tested the existing-record failure path. Before the
>   change, hfs_bnode_write() rejected an invalid offset internally but
>   __hfs_ext_write_extent() continued and cleared the dirty flag. With
>   v2, the invalid write is rejected before hfs_bnode_write().
> 
> ---
>  fs/hfs/extent.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
> index f066a99a863b..13426503fbb3 100644
> --- a/fs/hfs/extent.c
> +++ b/fs/hfs/extent.c
> @@ -121,12 +121,21 @@ static int __hfs_ext_write_extent(struct inode
> *inode, struct hfs_find_data *fd)
>  		res = hfs_bmap_reserve(fd->tree, fd->tree->depth +
> 1);
>  		if (res)
>  			return res;
> -		hfs_brec_insert(fd, HFS_I(inode)->cached_extents,
> sizeof(hfs_extent_rec));
> +		res = hfs_brec_insert(fd, HFS_I(inode)-
> >cached_extents,
> +				      sizeof(hfs_extent_rec));
> +		if (res)
> +			return res;
>  		HFS_I(inode)->flags &=
> ~(HFS_FLG_EXT_DIRTY|HFS_FLG_EXT_NEW);
>  	} else {
>  		if (res)
>  			return res;
> -		hfs_bnode_write(fd->bnode, HFS_I(inode)-
> >cached_extents, fd->entryoffset, fd->entrylength);
> +		if (fd->entrylength != sizeof(hfs_extent_rec) ||
> +		    fd->entryoffset < 0 ||
> +		    (u64)fd->entryoffset + fd->entrylength >
> +			    fd->tree->node_size)

I think it will be better to introduce a small check function that can
be reused then. And code will be cleaner here. What do you think?

> +			return -EIO;
> +		hfs_bnode_write(fd->bnode, HFS_I(inode)-
> >cached_extents,
> +				fd->entryoffset, fd->entrylength);

I see that you are trying not to go into huge modification. But,
frankly speaking, I believe we need the refactoring of
hfs_bnode_write() calling. This function should return error code and
we need to process this error code in other methods. Maybe, future
refactoring work for you? ;)

Thanks,
Slava.

>  		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
>  	}
>  	return 0;

Hi Slava,

Thanks for the feedback.

Yes, I agree. A small reusable check helper would make the code cleaner
and avoid duplicating the range validation.

Regarding hfs_bnode_write(), I also agree that its current void
interface makes proper error handling difficult. Converting it to
return an error code and auditing its callers looks like the right
direction for a follow-up refactoring.

For this patch, I will keep the change small, introduce the reusable
check helper, and send a v3.

I would be happy to work on the hfs_bnode_write() refactoring as a
follow-up as well.

Thanks,
Davy Felipe

On Wed, 23 Sep 2026, Viacheslav Dubeyko wrote:

> On Tue, 2026-09-22 at 20:40 -0300, Davy Felipe wrote:
>> __hfs_ext_write_extent() does not report all failures while updating
>> the extents B-tree.
>>
>> When inserting a new extent record, the return value of
>> hfs_brec_insert() is ignored and HFS_FLG_EXT_DIRTY and
>> HFS_FLG_EXT_NEW
>> are cleared even if the insertion fails.
>>
>> When updating an existing extent record, hfs_bnode_write() returns
>> void, so its caller cannot detect a rejected write. Validate the
>> extent
>> record size and node range before calling hfs_bnode_write().
>>
>> Propagate errors returned by hfs_brec_insert() and return -EIO for an
>> invalid existing extent record. Only clear the extent dirty flags
>> after
>> a successful operation.
>>
>> Fault injection confirmed both failure paths. Insertion errors are
>> propagated to the caller, and invalid existing-record writes are
>> rejected before hfs_bnode_write() without clearing the dirty state.
>>
>> Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>
>>
>> Changes in v2:
>> - Validate the existing extent record size and node range before
>>   calling hfs_bnode_write(), following review feedback.
>> - Return -EIO without clearing HFS_FLG_EXT_DIRTY when validation
>>   fails.
>> - Fault-injection tested the existing-record failure path. Before the
>>   change, hfs_bnode_write() rejected an invalid offset internally but
>>   __hfs_ext_write_extent() continued and cleared the dirty flag. With
>>   v2, the invalid write is rejected before hfs_bnode_write().
>>
>> ---
>>  fs/hfs/extent.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
>> index f066a99a863b..13426503fbb3 100644
>> --- a/fs/hfs/extent.c
>> +++ b/fs/hfs/extent.c
>> @@ -121,12 +121,21 @@ static int __hfs_ext_write_extent(struct inode
>> *inode, struct hfs_find_data *fd)
>>  		res = hfs_bmap_reserve(fd->tree, fd->tree->depth +
>> 1);
>>  		if (res)
>>  			return res;
>> -		hfs_brec_insert(fd, HFS_I(inode)->cached_extents,
>> sizeof(hfs_extent_rec));
>> +		res = hfs_brec_insert(fd, HFS_I(inode)-
>>> cached_extents,
>> +				      sizeof(hfs_extent_rec));
>> +		if (res)
>> +			return res;
>>  		HFS_I(inode)->flags &=
>> ~(HFS_FLG_EXT_DIRTY|HFS_FLG_EXT_NEW);
>>  	} else {
>>  		if (res)
>>  			return res;
>> -		hfs_bnode_write(fd->bnode, HFS_I(inode)-
>>> cached_extents, fd->entryoffset, fd->entrylength);
>> +		if (fd->entrylength != sizeof(hfs_extent_rec) ||
>> +		    fd->entryoffset < 0 ||
>> +		    (u64)fd->entryoffset + fd->entrylength >
>> +			    fd->tree->node_size)
>
> I think it will be better to introduce a small check function that can
> be reused then. And code will be cleaner here. What do you think?
>
>> +			return -EIO;
>> +		hfs_bnode_write(fd->bnode, HFS_I(inode)-
>>> cached_extents,
>> +				fd->entryoffset, fd->entrylength);
>
> I see that you are trying not to go into huge modification. But,
> frankly speaking, I believe we need the refactoring of
> hfs_bnode_write() calling. This function should return error code and
> we need to process this error code in other methods. Maybe, future
> refactoring work for you? ;)
>
> Thanks,
> Slava.
>
>>  		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
>>  	}
>>  	return 0;
>

hfs_brec_insert() may fail while inserting a new extent record, but
__hfs_ext_write_extent() currently ignores its return value and clears
HFS_FLG_EXT_DIRTY and HFS_FLG_EXT_NEW as if the insertion had
succeeded.

Propagate errors returned by hfs_brec_insert() and only clear the
extent flags after a successful insertion.

When updating an existing extent record, hfs_bnode_write() returns
void. Validate the extent record size and use a reusable B-tree node
range helper to reject invalid write parameters before calling
hfs_bnode_write(). This prevents an invalid update from being treated
as successful and avoids clearing HFS_FLG_EXT_DIRTY in that case.

Negative-path testing in QEMU confirmed that an insertion error is
propagated to the caller. Testing the existing-record path also
confirmed that invalid write parameters are rejected before
HFS_FLG_EXT_DIRTY is cleared.

Signed-off-by: Davy Felipe <davyfelipe34@gmail.com>

Sorry, I missed your suggestion about factoring the validation into a
reusable helper in v3. This revision addresses it.

Changes in v4:
- Factor B-tree node range validation into a reusable helper, as
  suggested by Viacheslav Dubeyko.
- Keep the extent-record size check local to the extent write path.
- Preserve the error propagation and validation behavior from v3.

---
 fs/hfs/btree.h  |  7 +++++++
 fs/hfs/extent.c | 12 ++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
index b4c3f2a31471..576412e6901e 100644
--- a/fs/hfs/btree.h
+++ b/fs/hfs/btree.h
@@ -84,6 +84,13 @@ struct hfs_find_data {
 	int entryoffset, entrylength;
 };
 
+static inline bool hfs_bnode_is_valid_range(struct hfs_bnode *node,
+					    int off, int len)
+{
+	return off >= 0 && len > 0 &&
+	       (u64)off + len <= node->tree->node_size;
+}
+
 
 /* btree.c */
 extern struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id,
diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index f066a99a863b..ece782205b47 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -121,12 +121,20 @@ static int __hfs_ext_write_extent(struct inode *inode, struct hfs_find_data *fd)
 		res = hfs_bmap_reserve(fd->tree, fd->tree->depth + 1);
 		if (res)
 			return res;
-		hfs_brec_insert(fd, HFS_I(inode)->cached_extents, sizeof(hfs_extent_rec));
+		res = hfs_brec_insert(fd, HFS_I(inode)->cached_extents,
+				      sizeof(hfs_extent_rec));
+		if (res)
+			return res;
 		HFS_I(inode)->flags &= ~(HFS_FLG_EXT_DIRTY|HFS_FLG_EXT_NEW);
 	} else {
 		if (res)
 			return res;
-		hfs_bnode_write(fd->bnode, HFS_I(inode)->cached_extents, fd->entryoffset, fd->entrylength);
+		if (fd->entrylength != sizeof(hfs_extent_rec) ||
+		    !hfs_bnode_is_valid_range(fd->bnode, fd->entryoffset,
+					      fd->entrylength))
+			return -EIO;
+		hfs_bnode_write(fd->bnode, HFS_I(inode)->cached_extents,
+				fd->entryoffset, fd->entrylength);
 		HFS_I(inode)->flags &= ~HFS_FLG_EXT_DIRTY;
 	}
 	return 0;
-- 
2.55.0