Message-ID: <20ca1d6dc2b4216246b14233373a2e06e29ca0dc.camel@ibm.com>
Date: Thu, 28 Aug 2025 19:46:17 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "glaubitz@...sik.fu-berlin.de" <glaubitz@...sik.fu-berlin.de>,
        "yang.chenzhi@...o.com" <yang.chenzhi@...o.com>,
        "slava@...eyko.com"
	<slava@...eyko.com>,
        "frank.li@...o.com" <frank.li@...o.com>
CC: "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH v2] hfs: add return values to hfs_brec_lenoff and
 hfs_bnode_read to improve robustness

On Thu, 2025-08-28 at 12:35 +0000, 杨晨志 wrote:
> 在 2025/8/28 4:04, Viacheslav Dubeyko 写道:
> > Frankly speaking, I don't think that reworking this method for
> > returning the error code is necessary. Currently, we return the length
> > value (u16) and we can return U16_MAX as for off as for len for the
> > case of incorrect offset or erroneous logic. We can treat U16_MAX as
> > error condition and we can check off and len for this value. Usually,
> > HFS b-tree node has 512 bytes in size and as offset as length cannot be
> > equal to U16_MAX (or bigger). And we don't need to change the input and
> > output arguments if we will check for U16_MAX value.
> 
> Using U16_MAX as the error return value is reasonable. This change also 
> applies to hfsplus, since the maximum node_size in hfsplus is 32768 
> (0x8000), which is less than U16_MAX. Adopting this approach helps avoid 
> extensive interface changes. I agree with your point.
> 
> > > diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> > > index e8cd1a31f247..b0bbaf016b8d 100644
> > > --- a/fs/hfs/bnode.c
> > > +++ b/fs/hfs/bnode.c
> > > @@ -57,26 +57,16 @@ int check_and_correct_requested_length(struct
> > > hfs_bnode *node, int off, int len)
> > >   	return len;
> > >   }
> > >   
> > > -void hfs_bnode_read(struct hfs_bnode *node, void *buf, int off, int
> > > len)
> > > +int __hfs_bnode_read(struct hfs_bnode *node, void *buf, u16 off, u16
> > > len)
> > 
> > I don't follow why do we need to introduce __hfs_bnode_read(). One
> > method for all is enough. And I think we still can use only
> > hfs_bnode_read(). Because, we can initialize the buffer by 0x00 or 0xFF
> > in the case we cannot read if offset or length are invalid. Usually,
> > every method checks (or should) check the returning value of
> > hfs_bnode_read().
> > 
> > >   {
> > >   	struct page *page;
> > >   	int pagenum;
> > >   	int bytes_read;
> > >   	int bytes_to_read;
> > >   
> > > -	if (!is_bnode_offset_valid(node, off))
> > > -		return;
> > > -
> > > -	if (len == 0) {
> > > -		pr_err("requested zero length: "
> > > -		       "NODE: id %u, type %#x, height %u, "
> > > -		       "node_size %u, offset %d, len %d\n",
> > > -		       node->this, node->type, node->height,
> > > -		       node->tree->node_size, off, len);
> > > -		return;
> > > -	}
> > > -
> > > -	len = check_and_correct_requested_length(node, off, len);
> > > +	/* len = 0 is invalid: prevent use of an uninitalized
> > > buffer*/
> > > +	if (!len || !hfs_off_and_len_is_valid(node, off, len))
> > > +		return -EINVAL;
> > >   
> > >   	off += node->page_offset;
> > >   	pagenum = off >> PAGE_SHIFT;
> > > @@ -93,6 +83,47 @@ void hfs_bnode_read(struct hfs_bnode *node, void
> > > *buf, int off, int len)
> > >   		pagenum++;
> > >   		off = 0; /* page offset only applies to the first
> > > page */
> > >   	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __hfs_bnode_read_u16(struct hfs_bnode *node, u16* buf,
> > > u16 off)
> > 
> > I don't see the point to introduce another version of method because we
> > can return U16_MAX as invalid value.
> > 
> > > +{
> > > +	__be16 data;
> > > +	int res;
> > > +
> > > +	res = __hfs_bnode_read(node, (void*)(&data), off, 2);
> > > +	if (res)
> > > +		return res;
> > > +	*buf = be16_to_cpu(data);
> > > +	return 0;
> > > +}
> > > +
> > > +
> > > +static int __hfs_bnode_read_u8(struct hfs_bnode *node, u8* buf, u16
> > > off)
> > 
> > And we can return U8_MAX as invalid value here too.
> 
> My original plan was to add a return value to hfs_bnode_read(). However, 
> since modifying this interface would require changes across dozens of 
> call sites—which seems too extensive—and introducing a return value 
> might necessitate additional error handling in certain functions (e.g., 
> hfs_bnode_split()), I wasn’t entirely sure how to proceed.
> 
> Therefore, I created __hfs_bnode_read() -- a function that behaves 
> identically to hfs_bnode_read but includes a return value—as a way to 
> experiment and facilitate discussion. I don’t think we ultimately need 
> an additional function; its purpose is strictly to help evaluate, within 
> this RFC patch, whether adding a return value to hfs_bnode_read is 
> desirable and whether the approach taken in __hfs_bnode_read() would be 
> suitable.
> 
> I agree that we should check return values at every call site—there are 
> too many issues in HFS caused by missing checks. However, I’m a bit 
> confused by the suggestion to validate the hfs_bnode_read buffer 
> directly. If the buffer is a structured type (such as btree_key), 
> detecting errors becomes more challenging. In such cases, we may need to 
> rely on methods like memcmp() or memchr_inv(). From that perspective, 
> adding a return value to hfs_bnode_read seems an easier way.
> 
> If you have a lightweight method to quickly verify the validity of a 
> read without introducing a return value, please let me know. I can 
> follow up with a new patch to address it.

My initial idea was to check the fields of the record. For example, like this
[1]:

hfs_bnode_read(fd.bnode, &entry, fd.entryoffset, fd.entrylength);
if (entry.type != HFS_CDR_THD) {
     pr_err("bad catalog folder thread\n");
     err = -EIO;
     goto out;
}

However, unfortunately, we also have such patterns [2]:

hfs_bnode_read(node, &node_desc, 0, sizeof(node_desc));
node_desc.next = cpu_to_be32(node->next);
node_desc.num_recs = cpu_to_be16(node->num_recs);
hfs_bnode_write(node, &node_desc, 0, sizeof(node_desc));

Of course, technically speaking, it is possible try to check the record's fields
but we could miss something, it is complicated and it could be more compute-
intensive. So, it is much simpler simply to check the returned error. The
question here: how to minimize the required changes? So, returning error code
from hfs_bnode_read() could be the simplest solution.

However, I don;t completely agree with hfs_off_and_len_is_valid() logic:

+static inline
+bool hfs_off_and_len_is_valid(struct hfs_bnode *node, u16 off, u16 len)
+{
+	bool ret = true;
+	if (off > node->tree->node_size ||
+			off + len > node->tree->node_size)
+		ret = false;
+
+	if (!ret) {
+		pr_err("requested invalid offset: "
+		       "NODE: id %u, type %#x, height %u, "
+		       "node_size %u, offset %u, length %u\n",
+		       node->this, node->type, node->height,
+		       node->tree->node_size, off, len);
+	}
+
+	return ret;
+}

If offset is out of node size, then, of course, we cannot read anything. But if
the offset is inside node and (offset + length) is bigger than node size, then
we can simply correct the length and read anyway. The question here: has buffer
enough allocated memory to receive the read data? But this check is should be
done by caller. Potentially, it is possible to receive a granularity size of
requested metadata structure and to compare with the requested length.
Otherwise, we cannot make any other reasonable conclusion.

As far as I can see, we have around 25 calls of hfs_bnode_read() and mostly
functions are ready to return error. So, we can carefully rework this logic.
The hfs_brec_lenoff(), hfs_bnode_read_u16() can return U16_MAX and
hfs_bnode_read_u8() can return U8_MAX. The hfs_bnode_read_key() is under
question yet. Should we return error code here or calling memset() will be
enough? I think hfs_bnode_dump() doesn't require significant rework because it
is mostly debugging function and it should dump as much as possible nevertheless
of detected errors.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/hfs/dir.c#L82
[2] https://elixir.bootlin.com/linux/v6.17-rc3/source/fs/hfs/brec.c#L327

> > 

Powered by blists - more mailing lists