* [PATCH v4] hfsplus: validate thread record before delete key rebuild
@ 2026-07-07 23:41 Kyle Zeng
2026-07-08 18:14 ` Viacheslav Dubeyko
0 siblings, 1 reply; 3+ messages in thread
From: Kyle Zeng @ 2026-07-07 23:41 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Viacheslav Dubeyko, Kyle Zeng
hfsplus_delete_cat() is called with str == NULL when the last open
reference to an unlinked HFS+ hardlink backing inode is closed. In that
case, the function finds the catalog thread by CNID and rebuilds the
catalog key from thread.nodeName.
That reconstruction path reads thread.nodeName.length directly from the
catalog B-tree into fd.search_key and then copies length * 2 bytes into
fd.search_key->cat.name.unicode. It does not first check that the found
record is a thread record, that the record size matches the thread name
length, or that nodeName.length fits in the fixed HFS+ catalog key
buffer.
A corrupted image can therefore provide an oversized thread name length
and make hfs_bnode_read() write past the catalog search-key allocation.
Read the CNID record through hfsplus_brec_read_cat(), which performs the
hfs_brec_find() lookup via hfs_brec_read() before validating the catalog
record. Reject non-thread records before building the delete key from
the validated thread name.
Make hfsplus_brec_read_cat() validate that a thread nodeName length both
fits HFSPLUS_MAX_STRLEN and exactly matches fd->entrylength, then share
the HFS+ thread-record-type helper between hfsplus_find_cat() and
hfsplus_delete_cat().
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Assisted-by: Codex:gpt-5.5
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
fs/hfsplus/bfind.c | 8 ++++++--
fs/hfsplus/catalog.c | 30 ++++++++++++------------------
fs/hfsplus/hfsplus_fs.h | 12 ++++++++++++
3 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/fs/hfsplus/bfind.c b/fs/hfsplus/bfind.c
index 9a55fa6d5294..d5e41e1c70b3 100644
--- a/fs/hfsplus/bfind.c
+++ b/fs/hfsplus/bfind.c
@@ -322,8 +322,12 @@ int hfsplus_brec_read_cat(struct hfs_find_data *fd, hfsplus_cat_entry *entry)
pr_err("thread record too short (got %u)\n", fd->entrylength);
return -EIO;
}
- expected_size = hfsplus_cat_thread_size(&entry->thread);
- break;
+ if (!is_hfsplus_cat_thread_name_length_valid(&entry->thread,
+ fd->entrylength)) {
+ pr_err("catalog thread name length corrupted\n");
+ return -EIO;
+ }
+ return 0;
default:
pr_err("unknown catalog record type %d\n",
be16_to_cpu(entry->type));
diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index 776ce36cf076..1a4b0beae135 100644
--- a/fs/hfsplus/catalog.c
+++ b/fs/hfsplus/catalog.c
@@ -204,16 +204,11 @@ int hfsplus_find_cat(struct super_block *sb, u32 cnid,
return err;
type = be16_to_cpu(tmp.type);
- if (type != HFSPLUS_FOLDER_THREAD && type != HFSPLUS_FILE_THREAD) {
+ if (!is_hfs_thread_record_type(type)) {
pr_err("found bad thread record in catalog\n");
return -EIO;
}
- if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
- pr_err("catalog name length corrupted\n");
- return -EIO;
- }
-
hfsplus_cat_build_key_uni(fd->search_key,
be32_to_cpu(tmp.thread.parentID),
&tmp.thread.nodeName);
@@ -350,23 +345,22 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, const struct qstr *str)
goto out;
if (!str) {
- int len;
+ hfsplus_cat_entry entry = {0};
hfsplus_cat_build_key_with_cnid(sb, fd.search_key, cnid);
- err = hfs_brec_find(&fd, hfs_find_rec_by_key);
+ err = hfsplus_brec_read_cat(&fd, &entry);
if (err)
goto out;
- off = fd.entryoffset +
- offsetof(struct hfsplus_cat_thread, nodeName);
- fd.search_key->cat.parent = cpu_to_be32(dir->i_ino);
- hfs_bnode_read(fd.bnode,
- &fd.search_key->cat.name.length, off, 2);
- len = be16_to_cpu(fd.search_key->cat.name.length) * 2;
- hfs_bnode_read(fd.bnode,
- &fd.search_key->cat.name.unicode,
- off + 2, len);
- fd.search_key->key_len = cpu_to_be16(6 + len);
+ type = be16_to_cpu(entry.type);
+ if (!is_hfs_thread_record_type(type)) {
+ pr_err("found bad thread record in catalog\n");
+ err = -EIO;
+ goto out;
+ }
+
+ hfsplus_cat_build_key_uni(fd.search_key, dir->i_ino,
+ &entry.thread.nodeName);
} else {
err = hfsplus_cat_build_key(sb, fd.search_key, dir->i_ino, str);
if (unlikely(err))
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index ec04b82ad927..01e5f1246ff6 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -521,6 +521,18 @@ static inline u32 hfsplus_cat_thread_size(const struct hfsplus_cat_thread *threa
be16_to_cpu(thread->nodeName.length) * sizeof(hfsplus_unichr);
}
+static inline bool is_hfs_thread_record_type(u16 type)
+{
+ return type == HFSPLUS_FOLDER_THREAD || type == HFSPLUS_FILE_THREAD;
+}
+
+static inline bool is_hfsplus_cat_thread_name_length_valid(
+ const struct hfsplus_cat_thread *thread, u32 entrylength)
+{
+ return be16_to_cpu(thread->nodeName.length) <= HFSPLUS_MAX_STRLEN &&
+ entrylength == hfsplus_cat_thread_size(thread);
+}
+
int hfsplus_brec_read_cat(struct hfs_find_data *fd, hfsplus_cat_entry *entry);
/*
--
2.54.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] hfsplus: validate thread record before delete key rebuild
2026-07-07 23:41 [PATCH v4] hfsplus: validate thread record before delete key rebuild Kyle Zeng
@ 2026-07-08 18:14 ` Viacheslav Dubeyko
2026-07-09 0:58 ` Kyle Zeng
0 siblings, 1 reply; 3+ messages in thread
From: Viacheslav Dubeyko @ 2026-07-08 18:14 UTC (permalink / raw)
To: Kyle Zeng, linux-fsdevel
On Tue, 2026-07-07 at 16:41 -0700, Kyle Zeng wrote:
> hfsplus_delete_cat() is called with str == NULL when the last open
> reference to an unlinked HFS+ hardlink backing inode is closed. In
> that
> case, the function finds the catalog thread by CNID and rebuilds the
> catalog key from thread.nodeName.
>
> That reconstruction path reads thread.nodeName.length directly from
> the
> catalog B-tree into fd.search_key and then copies length * 2 bytes
> into
> fd.search_key->cat.name.unicode. It does not first check that the
> found
> record is a thread record, that the record size matches the thread
> name
> length, or that nodeName.length fits in the fixed HFS+ catalog key
> buffer.
>
> A corrupted image can therefore provide an oversized thread name
> length
> and make hfs_bnode_read() write past the catalog search-key
> allocation.
>
> Read the CNID record through hfsplus_brec_read_cat(), which performs
> the
> hfs_brec_find() lookup via hfs_brec_read() before validating the
> catalog
> record. Reject non-thread records before building the delete key from
> the validated thread name.
>
> Make hfsplus_brec_read_cat() validate that a thread nodeName length
> both
> fits HFSPLUS_MAX_STRLEN and exactly matches fd->entrylength, then
> share
> the HFS+ thread-record-type helper between hfsplus_find_cat() and
> hfsplus_delete_cat().
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> ---
> fs/hfsplus/bfind.c | 8 ++++++--
> fs/hfsplus/catalog.c | 30 ++++++++++++------------------
> fs/hfsplus/hfsplus_fs.h | 12 ++++++++++++
> 3 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/fs/hfsplus/bfind.c b/fs/hfsplus/bfind.c
> index 9a55fa6d5294..d5e41e1c70b3 100644
> --- a/fs/hfsplus/bfind.c
> +++ b/fs/hfsplus/bfind.c
> @@ -322,8 +322,12 @@ int hfsplus_brec_read_cat(struct hfs_find_data
> *fd, hfsplus_cat_entry *entry)
> pr_err("thread record too short (got %u)\n",
> fd->entrylength);
> return -EIO;
> }
> - expected_size = hfsplus_cat_thread_size(&entry-
> >thread);
> - break;
> + if (!is_hfsplus_cat_thread_name_length_valid(&entry-
> >thread,
> + fd-
> >entrylength)) {
> + pr_err("catalog thread name length
> corrupted\n");
> + return -EIO;
> + }
> + return 0;
Do we really need to change this function at all? Because, current
check already works pretty well. This check covers everything already:
if (fd->entrylength != expected_size) {
pr_err("catalog record size mismatch (type %d, got %u,
expected %u)\n",
be16_to_cpu(entry->type), fd->entrylength,
expected_size);
return -EIO;
}
> default:
> pr_err("unknown catalog record type %d\n",
> be16_to_cpu(entry->type));
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 776ce36cf076..1a4b0beae135 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -204,16 +204,11 @@ int hfsplus_find_cat(struct super_block *sb,
> u32 cnid,
> return err;
>
> type = be16_to_cpu(tmp.type);
> - if (type != HFSPLUS_FOLDER_THREAD && type !=
> HFSPLUS_FILE_THREAD) {
> + if (!is_hfs_thread_record_type(type)) {
> pr_err("found bad thread record in catalog\n");
> return -EIO;
> }
>
> - if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
> - pr_err("catalog name length corrupted\n");
> - return -EIO;
> - }
If hfsplus_brec_read_cat() covers this check already, then, probably,
we don't need in new function and we don't need this check. If we still
need to keep this check, then we cannot simply remove it but we need to
call the is_cat_thread_name_length_valid() here. So, do we really need
to introduce the is_cat_thread_name_length_valid() finally?
> -
> hfsplus_cat_build_key_uni(fd->search_key,
> be32_to_cpu(tmp.thread.parentID),
> &tmp.thread.nodeName);
> @@ -350,23 +345,22 @@ int hfsplus_delete_cat(u32 cnid, struct inode
> *dir, const struct qstr *str)
> goto out;
>
> if (!str) {
> - int len;
> + hfsplus_cat_entry entry = {0};
>
> hfsplus_cat_build_key_with_cnid(sb, fd.search_key,
> cnid);
> - err = hfs_brec_find(&fd, hfs_find_rec_by_key);
> + err = hfsplus_brec_read_cat(&fd, &entry);
> if (err)
> goto out;
>
> - off = fd.entryoffset +
> - offsetof(struct hfsplus_cat_thread,
> nodeName);
> - fd.search_key->cat.parent = cpu_to_be32(dir->i_ino);
> - hfs_bnode_read(fd.bnode,
> - &fd.search_key->cat.name.length, off, 2);
> - len = be16_to_cpu(fd.search_key->cat.name.length) *
> 2;
> - hfs_bnode_read(fd.bnode,
> - &fd.search_key->cat.name.unicode,
> - off + 2, len);
> - fd.search_key->key_len = cpu_to_be16(6 + len);
> + type = be16_to_cpu(entry.type);
> + if (!is_hfs_thread_record_type(type)) {
> + pr_err("found bad thread record in
> catalog\n");
> + err = -EIO;
> + goto out;
> + }
> +
> + hfsplus_cat_build_key_uni(fd.search_key, dir->i_ino,
> + &entry.thread.nodeName);
> } else {
> err = hfsplus_cat_build_key(sb, fd.search_key, dir-
> >i_ino, str);
> if (unlikely(err))
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index ec04b82ad927..01e5f1246ff6 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -521,6 +521,18 @@ static inline u32 hfsplus_cat_thread_size(const
> struct hfsplus_cat_thread *threa
> be16_to_cpu(thread->nodeName.length) *
> sizeof(hfsplus_unichr);
> }
>
> +static inline bool is_hfs_thread_record_type(u16 type)
> +{
> + return type == HFSPLUS_FOLDER_THREAD || type ==
> HFSPLUS_FILE_THREAD;
> +}
> +
> +static inline bool is_hfsplus_cat_thread_name_length_valid(
> + const struct hfsplus_cat_thread *thread, u32
> entrylength)
I don't like this style. :) Potentially, we can rework it:
static inline
bool is_cat_thread_name_length_valid(struct hfsplus_cat_thread *thread,
u32 entrylength);
Technically speaking, it will be better to provide struct hfs_find_data
*fd and hfsplus_cat_entry *entry here.
> +{
> + return be16_to_cpu(thread->nodeName.length) <=
> HFSPLUS_MAX_STRLEN
Do we need to have this check now? I assume that the second check could
cover everything. What do you think?
Thanks,
Slava.
> &&
> + entrylength == hfsplus_cat_thread_size(thread);
> +}
> +
> int hfsplus_brec_read_cat(struct hfs_find_data *fd,
> hfsplus_cat_entry *entry);
>
> /*
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] hfsplus: validate thread record before delete key rebuild
2026-07-08 18:14 ` Viacheslav Dubeyko
@ 2026-07-09 0:58 ` Kyle Zeng
0 siblings, 0 replies; 3+ messages in thread
From: Kyle Zeng @ 2026-07-09 0:58 UTC (permalink / raw)
To: Viacheslav Dubeyko; +Cc: linux-fsdevel
Considering that we want to reuse the check in
`hfsplus_brec_read_cat`, we can just switch to using
`hfsplus_brec_read_cat`, avoid the changes in `hfsplus_brec_read_cat`
and avoid adding the `is_hfsplus_cat_thread_name_length_valid`
function?
will send a v5 in a bit.
Best,
Kyle
On Wed, Jul 8, 2026 at 11:14 AM Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>
> On Tue, 2026-07-07 at 16:41 -0700, Kyle Zeng wrote:
> > hfsplus_delete_cat() is called with str == NULL when the last open
> > reference to an unlinked HFS+ hardlink backing inode is closed. In
> > that
> > case, the function finds the catalog thread by CNID and rebuilds the
> > catalog key from thread.nodeName.
> >
> > That reconstruction path reads thread.nodeName.length directly from
> > the
> > catalog B-tree into fd.search_key and then copies length * 2 bytes
> > into
> > fd.search_key->cat.name.unicode. It does not first check that the
> > found
> > record is a thread record, that the record size matches the thread
> > name
> > length, or that nodeName.length fits in the fixed HFS+ catalog key
> > buffer.
> >
> > A corrupted image can therefore provide an oversized thread name
> > length
> > and make hfs_bnode_read() write past the catalog search-key
> > allocation.
> >
> > Read the CNID record through hfsplus_brec_read_cat(), which performs
> > the
> > hfs_brec_find() lookup via hfs_brec_read() before validating the
> > catalog
> > record. Reject non-thread records before building the delete key from
> > the validated thread name.
> >
> > Make hfsplus_brec_read_cat() validate that a thread nodeName length
> > both
> > fits HFSPLUS_MAX_STRLEN and exactly matches fd->entrylength, then
> > share
> > the HFS+ thread-record-type helper between hfsplus_find_cat() and
> > hfsplus_delete_cat().
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Assisted-by: Codex:gpt-5.5
> > Signed-off-by: Kyle Zeng <kylebot@openai.com>
> > ---
> > fs/hfsplus/bfind.c | 8 ++++++--
> > fs/hfsplus/catalog.c | 30 ++++++++++++------------------
> > fs/hfsplus/hfsplus_fs.h | 12 ++++++++++++
> > 3 files changed, 30 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/hfsplus/bfind.c b/fs/hfsplus/bfind.c
> > index 9a55fa6d5294..d5e41e1c70b3 100644
> > --- a/fs/hfsplus/bfind.c
> > +++ b/fs/hfsplus/bfind.c
> > @@ -322,8 +322,12 @@ int hfsplus_brec_read_cat(struct hfs_find_data
> > *fd, hfsplus_cat_entry *entry)
> > pr_err("thread record too short (got %u)\n",
> > fd->entrylength);
> > return -EIO;
> > }
> > - expected_size = hfsplus_cat_thread_size(&entry-
> > >thread);
> > - break;
> > + if (!is_hfsplus_cat_thread_name_length_valid(&entry-
> > >thread,
> > + fd-
> > >entrylength)) {
> > + pr_err("catalog thread name length
> > corrupted\n");
> > + return -EIO;
> > + }
> > + return 0;
>
> Do we really need to change this function at all? Because, current
> check already works pretty well. This check covers everything already:
>
>
> if (fd->entrylength != expected_size) {
> pr_err("catalog record size mismatch (type %d, got %u,
> expected %u)\n",
> be16_to_cpu(entry->type), fd->entrylength,
> expected_size);
> return -EIO;
> }
>
> > default:
> > pr_err("unknown catalog record type %d\n",
> > be16_to_cpu(entry->type));
> > diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> > index 776ce36cf076..1a4b0beae135 100644
> > --- a/fs/hfsplus/catalog.c
> > +++ b/fs/hfsplus/catalog.c
> > @@ -204,16 +204,11 @@ int hfsplus_find_cat(struct super_block *sb,
> > u32 cnid,
> > return err;
> >
> > type = be16_to_cpu(tmp.type);
> > - if (type != HFSPLUS_FOLDER_THREAD && type !=
> > HFSPLUS_FILE_THREAD) {
> > + if (!is_hfs_thread_record_type(type)) {
> > pr_err("found bad thread record in catalog\n");
> > return -EIO;
> > }
> >
> > - if (be16_to_cpu(tmp.thread.nodeName.length) > 255) {
> > - pr_err("catalog name length corrupted\n");
> > - return -EIO;
> > - }
>
> If hfsplus_brec_read_cat() covers this check already, then, probably,
> we don't need in new function and we don't need this check. If we still
> need to keep this check, then we cannot simply remove it but we need to
> call the is_cat_thread_name_length_valid() here. So, do we really need
> to introduce the is_cat_thread_name_length_valid() finally?
>
> > -
> > hfsplus_cat_build_key_uni(fd->search_key,
> > be32_to_cpu(tmp.thread.parentID),
> > &tmp.thread.nodeName);
> > @@ -350,23 +345,22 @@ int hfsplus_delete_cat(u32 cnid, struct inode
> > *dir, const struct qstr *str)
> > goto out;
> >
> > if (!str) {
> > - int len;
> > + hfsplus_cat_entry entry = {0};
> >
> > hfsplus_cat_build_key_with_cnid(sb, fd.search_key,
> > cnid);
> > - err = hfs_brec_find(&fd, hfs_find_rec_by_key);
> > + err = hfsplus_brec_read_cat(&fd, &entry);
> > if (err)
> > goto out;
> >
> > - off = fd.entryoffset +
> > - offsetof(struct hfsplus_cat_thread,
> > nodeName);
> > - fd.search_key->cat.parent = cpu_to_be32(dir->i_ino);
> > - hfs_bnode_read(fd.bnode,
> > - &fd.search_key->cat.name.length, off, 2);
> > - len = be16_to_cpu(fd.search_key->cat.name.length) *
> > 2;
> > - hfs_bnode_read(fd.bnode,
> > - &fd.search_key->cat.name.unicode,
> > - off + 2, len);
> > - fd.search_key->key_len = cpu_to_be16(6 + len);
> > + type = be16_to_cpu(entry.type);
> > + if (!is_hfs_thread_record_type(type)) {
> > + pr_err("found bad thread record in
> > catalog\n");
> > + err = -EIO;
> > + goto out;
> > + }
> > +
> > + hfsplus_cat_build_key_uni(fd.search_key, dir->i_ino,
> > + &entry.thread.nodeName);
> > } else {
> > err = hfsplus_cat_build_key(sb, fd.search_key, dir-
> > >i_ino, str);
> > if (unlikely(err))
> > diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> > index ec04b82ad927..01e5f1246ff6 100644
> > --- a/fs/hfsplus/hfsplus_fs.h
> > +++ b/fs/hfsplus/hfsplus_fs.h
> > @@ -521,6 +521,18 @@ static inline u32 hfsplus_cat_thread_size(const
> > struct hfsplus_cat_thread *threa
> > be16_to_cpu(thread->nodeName.length) *
> > sizeof(hfsplus_unichr);
> > }
> >
> > +static inline bool is_hfs_thread_record_type(u16 type)
> > +{
> > + return type == HFSPLUS_FOLDER_THREAD || type ==
> > HFSPLUS_FILE_THREAD;
> > +}
> > +
> > +static inline bool is_hfsplus_cat_thread_name_length_valid(
> > + const struct hfsplus_cat_thread *thread, u32
> > entrylength)
>
> I don't like this style. :) Potentially, we can rework it:
>
> static inline
> bool is_cat_thread_name_length_valid(struct hfsplus_cat_thread *thread,
> u32 entrylength);
>
> Technically speaking, it will be better to provide struct hfs_find_data
> *fd and hfsplus_cat_entry *entry here.
>
>
> > +{
> > + return be16_to_cpu(thread->nodeName.length) <=
> > HFSPLUS_MAX_STRLEN
>
> Do we need to have this check now? I assume that the second check could
> cover everything. What do you think?
>
> Thanks,
> Slava.
>
> > &&
> > + entrylength == hfsplus_cat_thread_size(thread);
> > +}
> > +
> > int hfsplus_brec_read_cat(struct hfs_find_data *fd,
> > hfsplus_cat_entry *entry);
> >
> > /*
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-07-09 0:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-07 23:41 [PATCH v4] hfsplus: validate thread record before delete key rebuild Kyle Zeng
2026-07-08 18:14 ` Viacheslav Dubeyko
2026-07-09 0:58 ` Kyle Zeng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox