qnx4 dir name length can vary to be of maximum size
QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
'link info' entry is stored and the status byte is set.
So to avoid buffer overflow check di_fname length
fetched from (struct qnx4_inode_entry *)
before use in strlen to avoid buffer overflow.

panic context
[ 4849.636861] detected buffer overflow in strlen
[ 4849.636897] ------------[ cut here ]------------
[ 4849.636902] kernel BUG at lib/string.c:1165!
[ 4849.636917] invalid opcode: 0000 [#2] SMP PTI
..
[ 4849.637047] Call Trace:
[ 4849.637053]  <TASK>
[ 4849.637059]  ? show_trace_log_lvl+0x1d6/0x2ea
[ 4849.637075]  ? show_trace_log_lvl+0x1d6/0x2ea
[ 4849.637095]  ? qnx4_find_entry.cold+0xc/0x18 [qnx4]
[ 4849.637111]  ? show_regs.part.0+0x23/0x29
[ 4849.637123]  ? __die_body.cold+0x8/0xd
[ 4849.637135]  ? __die+0x2b/0x37
[ 4849.637147]  ? die+0x30/0x60
[ 4849.637161]  ? do_trap+0xbe/0x100
[ 4849.637171]  ? do_error_trap+0x6f/0xb0
[ 4849.637180]  ? fortify_panic+0x13/0x15
[ 4849.637192]  ? exc_invalid_op+0x53/0x70
[ 4849.637203]  ? fortify_panic+0x13/0x15
[ 4849.637215]  ? asm_exc_invalid_op+0x1b/0x20
[ 4849.637228]  ? fortify_panic+0x13/0x15
[ 4849.637240]  ? fortify_panic+0x13/0x15
[ 4849.637251]  qnx4_find_entry.cold+0xc/0x18 [qnx4]
[ 4849.637264]  qnx4_lookup+0x3c/0xa0 [qnx4]
[ 4849.637275]  __lookup_slow+0x85/0x150
[ 4849.637291]  walk_component+0x145/0x1c0
[ 4849.637304]  ? path_init+0x2c0/0x3f0
[ 4849.637316]  path_lookupat+0x6e/0x1c0
[ 4849.637330]  filename_lookup+0xcf/0x1d0
[ 4849.637341]  ? __check_object_size+0x1d/0x30
[ 4849.637354]  ? strncpy_from_user+0x44/0x150
[ 4849.637365]  ? getname_flags.part.0+0x4c/0x1b0
[ 4849.637375]  user_path_at_empty+0x3f/0x60
[ 4849.637383]  vfs_statx+0x7a/0x130
[ 4849.637393]  do_statx+0x45/0x80
..

Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com>
---
 fs/qnx4/namei.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..825b891a52b3 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name,
 	} else {
 		namelen = QNX4_SHORT_NAME_MAX;
 	}
+
+	/** qnx4 dir name length can vary, check the di_fname
+	 * fetched from (struct qnx4_inode_entry *) before use in
+	 * strlen to avoid panic due to buffer overflow"
+	 */
+	if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
+		return -ENAMETOOLONG;
 	thislen = strlen( de->di_fname );
 	if ( thislen > namelen )
 		thislen = namelen;
-- 
2.34.1

On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote:
> qnx4 dir name length can vary to be of maximum size
> QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> 'link info' entry is stored and the status byte is set.
> So to avoid buffer overflow check di_fname length
> fetched from (struct qnx4_inode_entry *)
> before use in strlen to avoid buffer overflow.
> 

Inspired by removals of reiserfs and sysv I decided to try to whack
qnx4.

This here is the only qnx4-specific change made to the fs in years.

Are you using the filesystem? Perhaps you just playing around fuzzing
and that's how you got there instead?

On Fri, Feb 21, 2025 at 03:51:23PM +0100, Mateusz Guzik wrote:
> On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote:
> > qnx4 dir name length can vary to be of maximum size
> > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> > 'link info' entry is stored and the status byte is set.
> > So to avoid buffer overflow check di_fname length
> > fetched from (struct qnx4_inode_entry *)
> > before use in strlen to avoid buffer overflow.
> > 
> 
> Inspired by removals of reiserfs and sysv I decided to try to whack
> qnx4.

I have no strong opinion here beyond just pointing out that it appears
that the qnx4 fs is still extant in the world. QNX itself is still alive
and well and using this filesystem based on what I can find.

-Kees

-- 
Kees Cook

On Fri, Feb 21, 2025 at 6:38 PM Kees Cook <kees@kernel.org> wrote:
>
> On Fri, Feb 21, 2025 at 03:51:23PM +0100, Mateusz Guzik wrote:
> > On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote:
> > > qnx4 dir name length can vary to be of maximum size
> > > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> > > 'link info' entry is stored and the status byte is set.
> > > So to avoid buffer overflow check di_fname length
> > > fetched from (struct qnx4_inode_entry *)
> > > before use in strlen to avoid buffer overflow.
> > >
> >
> > Inspired by removals of reiserfs and sysv I decided to try to whack
> > qnx4.
>
> I have no strong opinion here beyond just pointing out that it appears
> that the qnx4 fs is still extant in the world. QNX itself is still alive
> and well and using this filesystem based on what I can find.
>

I'm aware.

However, we reached a point where should someone need to access a
now-removed filesystem, they can spin up a VM with an older system to
do it (including with one of the myriad Linux distro releases).
Suppose support disappears tomorrow. You still have something like
debian which will have a kernel with the module for several years. But
suppose you are years down the road and all the Linux distros which
had it are past EOL and you still need it. For the purpose of reading
the sucker, you can still use them.

So I don't believe this will cut anyone off of transferring data out
of a filesystem which got whacked upstream. That's concerning old
filesystems in general.

As for qnx4 in particular, should you git log on it, you will find the
support is read-only. And should you read between the commits (if you
will) I am not at all convinced even that was high quality to begin
with -- looks like an abandoned WIP.

General tune is not holding the codebase hostage to obsolete (and
probably not at all operational) components. If in doubt, prune it.

This reminded me of a funny remark made by someone else concerning
removal of drivers for stale hardware. It was in the lines of "if you
ask if anyone is still using it, someone will respond they have an old
machine in their garage which they were totally going to boot up this
weekend. if you just remove it, nobody will ever notice".

If it was not for the aforementioned bugfix, I would be sending a
removal instead.
-- 
Mateusz Guzik <mjguzik gmail.com>

On Sat, Feb 22, 2025 at 01:12:47PM +0100, Mateusz Guzik wrote:

> General tune is not holding the codebase hostage to obsolete (and
> probably not at all operational) components. If in doubt, prune it.

What exactly is being held hostage, though?  Do we have an API change
that gets blocked just by the old filesystems and if so, which one
it is?

On Sun, Feb 23, 2025 at 1:19 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Feb 22, 2025 at 01:12:47PM +0100, Mateusz Guzik wrote:
>
> > General tune is not holding the codebase hostage to obsolete (and
> > probably not at all operational) components. If in doubt, prune it.
>
> What exactly is being held hostage, though?  Do we have an API change
> that gets blocked just by the old filesystems and if so, which one
> it is?

I was speaking in general. Some code uses obsolete APIs, other puts
weird restrictions, some other has to be at least reviewed.

General point being mere existence of code is an extra maintenance burden.

If the code is beneficial to have, then so be it. But if the code at
hand is not even used by anyone (and it is unclear if it even works),
what's the point keeping it?

-- 
Mateusz Guzik <mjguzik gmail.com>

On Sat, Feb 22, 2025 at 01:12:47PM +0100, Mateusz Guzik wrote:
> If it was not for the aforementioned bugfix, I would be sending a
> removal instead.

Less code is fewer bugs. I'm for it. :)

-- 
Kees Cook

On Sat, Feb 22, 2025 at 4:17 PM Kees Cook <kees@kernel.org> wrote:
>
> On Sat, Feb 22, 2025 at 01:12:47PM +0100, Mateusz Guzik wrote:
> > If it was not for the aforementioned bugfix, I would be sending a
> > removal instead.
>
> Less code is fewer bugs. I'm for it. :)
>

Removed code is debugged code.

-- 
Mateusz Guzik <mjguzik gmail.com>

On Sat, Feb 22, 2025 at 05:36:11PM +0100, Mateusz Guzik wrote:
> On Sat, Feb 22, 2025 at 4:17 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Sat, Feb 22, 2025 at 01:12:47PM +0100, Mateusz Guzik wrote:
> > > If it was not for the aforementioned bugfix, I would be sending a
> > > removal instead.
> >
> > Less code is fewer bugs. I'm for it. :)
> >
> 
> Removed code is debugged code.

We have both qnx4 and qnx6. Can anyone with authority speak as to the
usage of qnx4?

On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote:
> qnx4 dir name length can vary to be of maximum size
> QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> 'link info' entry is stored and the status byte is set.
> So to avoid buffer overflow check di_fname length
> fetched from (struct qnx4_inode_entry *)
> before use in strlen to avoid buffer overflow.
> 
> panic context
> [ 4849.636861] detected buffer overflow in strlen
> [ 4849.636897] ------------[ cut here ]------------
> [ 4849.636902] kernel BUG at lib/string.c:1165!
> [ 4849.636917] invalid opcode: 0000 [#2] SMP PTI
> ..
> [ 4849.637047] Call Trace:
> [ 4849.637053]  <TASK>
> [ 4849.637059]  ? show_trace_log_lvl+0x1d6/0x2ea
> [ 4849.637075]  ? show_trace_log_lvl+0x1d6/0x2ea
> [ 4849.637095]  ? qnx4_find_entry.cold+0xc/0x18 [qnx4]
> [ 4849.637111]  ? show_regs.part.0+0x23/0x29
> [ 4849.637123]  ? __die_body.cold+0x8/0xd
> [ 4849.637135]  ? __die+0x2b/0x37
> [ 4849.637147]  ? die+0x30/0x60
> [ 4849.637161]  ? do_trap+0xbe/0x100
> [ 4849.637171]  ? do_error_trap+0x6f/0xb0
> [ 4849.637180]  ? fortify_panic+0x13/0x15
> [ 4849.637192]  ? exc_invalid_op+0x53/0x70
> [ 4849.637203]  ? fortify_panic+0x13/0x15
> [ 4849.637215]  ? asm_exc_invalid_op+0x1b/0x20
> [ 4849.637228]  ? fortify_panic+0x13/0x15
> [ 4849.637240]  ? fortify_panic+0x13/0x15
> [ 4849.637251]  qnx4_find_entry.cold+0xc/0x18 [qnx4]
> [ 4849.637264]  qnx4_lookup+0x3c/0xa0 [qnx4]
> [ 4849.637275]  __lookup_slow+0x85/0x150
> [ 4849.637291]  walk_component+0x145/0x1c0
> [ 4849.637304]  ? path_init+0x2c0/0x3f0
> [ 4849.637316]  path_lookupat+0x6e/0x1c0
> [ 4849.637330]  filename_lookup+0xcf/0x1d0
> [ 4849.637341]  ? __check_object_size+0x1d/0x30
> [ 4849.637354]  ? strncpy_from_user+0x44/0x150
> [ 4849.637365]  ? getname_flags.part.0+0x4c/0x1b0
> [ 4849.637375]  user_path_at_empty+0x3f/0x60
> [ 4849.637383]  vfs_statx+0x7a/0x130
> [ 4849.637393]  do_statx+0x45/0x80
> ..
> 
> Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com>
> ---
>  fs/qnx4/namei.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> index 8d72221735d7..825b891a52b3 100644
> --- a/fs/qnx4/namei.c
> +++ b/fs/qnx4/namei.c
> @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name,
>  	} else {
>  		namelen = QNX4_SHORT_NAME_MAX;
>  	}
> +
> +	/** qnx4 dir name length can vary, check the di_fname
> +	 * fetched from (struct qnx4_inode_entry *) before use in
> +	 * strlen to avoid panic due to buffer overflow"
> +	 */

Style nit: this comment should start with just "/*" alone, like:

	/*
	 * qnx4 dir name ...

> +	if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
> +		return -ENAMETOOLONG;
>  	thislen = strlen( de->di_fname );

de->di_fname is:

struct qnx4_inode_entry {
        char            di_fname[QNX4_SHORT_NAME_MAX];
	...

#define QNX4_SHORT_NAME_MAX     16
#define QNX4_NAME_MAX           48

It's always going to have a max of QNX4_SHORT_NAME_MAX. Is any of this
code correct if namelen ends up being QNX4_NAME_MAX? It'll be reading
past the end of di_fname.

Is bh->b_data actually struct qnx4_inode_entry ?

-Kees

>  	if ( thislen > namelen )
>  		thislen = namelen;
> -- 
> 2.34.1
> 

-- 
Kees Cook

On Thu, Nov 16, 2023 at 06:29:59AM -0800, Kees Cook wrote:
> On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote:
> > qnx4 dir name length can vary to be of maximum size
> > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> > 'link info' entry is stored and the status byte is set.
> > So to avoid buffer overflow check di_fname length
> > fetched from (struct qnx4_inode_entry *)
> > before use in strlen to avoid buffer overflow.
> > 
> > panic context
> > [ 4849.636861] detected buffer overflow in strlen
> > [ 4849.636897] ------------[ cut here ]------------
> > [ 4849.636902] kernel BUG at lib/string.c:1165!
> > [ 4849.636917] invalid opcode: 0000 [#2] SMP PTI
> > ..
> > [ 4849.637047] Call Trace:
> > [ 4849.637053]  <TASK>
> > [ 4849.637059]  ? show_trace_log_lvl+0x1d6/0x2ea
> > [ 4849.637075]  ? show_trace_log_lvl+0x1d6/0x2ea
> > [ 4849.637095]  ? qnx4_find_entry.cold+0xc/0x18 [qnx4]
> > [ 4849.637111]  ? show_regs.part.0+0x23/0x29
> > [ 4849.637123]  ? __die_body.cold+0x8/0xd
> > [ 4849.637135]  ? __die+0x2b/0x37
> > [ 4849.637147]  ? die+0x30/0x60
> > [ 4849.637161]  ? do_trap+0xbe/0x100
> > [ 4849.637171]  ? do_error_trap+0x6f/0xb0
> > [ 4849.637180]  ? fortify_panic+0x13/0x15
> > [ 4849.637192]  ? exc_invalid_op+0x53/0x70
> > [ 4849.637203]  ? fortify_panic+0x13/0x15
> > [ 4849.637215]  ? asm_exc_invalid_op+0x1b/0x20
> > [ 4849.637228]  ? fortify_panic+0x13/0x15
> > [ 4849.637240]  ? fortify_panic+0x13/0x15
> > [ 4849.637251]  qnx4_find_entry.cold+0xc/0x18 [qnx4]
> > [ 4849.637264]  qnx4_lookup+0x3c/0xa0 [qnx4]
> > [ 4849.637275]  __lookup_slow+0x85/0x150
> > [ 4849.637291]  walk_component+0x145/0x1c0
> > [ 4849.637304]  ? path_init+0x2c0/0x3f0
> > [ 4849.637316]  path_lookupat+0x6e/0x1c0
> > [ 4849.637330]  filename_lookup+0xcf/0x1d0
> > [ 4849.637341]  ? __check_object_size+0x1d/0x30
> > [ 4849.637354]  ? strncpy_from_user+0x44/0x150
> > [ 4849.637365]  ? getname_flags.part.0+0x4c/0x1b0
> > [ 4849.637375]  user_path_at_empty+0x3f/0x60
> > [ 4849.637383]  vfs_statx+0x7a/0x130
> > [ 4849.637393]  do_statx+0x45/0x80
> > ..
> > 
> > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com>
> > ---
> >  fs/qnx4/namei.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> > index 8d72221735d7..825b891a52b3 100644
> > --- a/fs/qnx4/namei.c
> > +++ b/fs/qnx4/namei.c
> > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name,
> >  	} else {
> >  		namelen = QNX4_SHORT_NAME_MAX;
> >  	}
> > +
> > +	/** qnx4 dir name length can vary, check the di_fname
> > +	 * fetched from (struct qnx4_inode_entry *) before use in
> > +	 * strlen to avoid panic due to buffer overflow"
> > +	 */
> 
> Style nit: this comment should start with just "/*" alone, like:
> 
> 	/*
> 	 * qnx4 dir name ...
> 
> > +	if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
> > +		return -ENAMETOOLONG;
> >  	thislen = strlen( de->di_fname );
> 
> de->di_fname is:
> 
> struct qnx4_inode_entry {
>         char            di_fname[QNX4_SHORT_NAME_MAX];
> 	...
> 
> #define QNX4_SHORT_NAME_MAX     16
> #define QNX4_NAME_MAX           48
> 
> It's always going to have a max of QNX4_SHORT_NAME_MAX. Is any of this
> code correct if namelen ends up being QNX4_NAME_MAX? It'll be reading
> past the end of di_fname.
> 
> Is bh->b_data actually struct qnx4_inode_entry ?

Ah-ha, it looks like it's _not_:

        if (!(bh = qnx4_find_entry(len, dir, name, &de, &ino)))
                goto out;
        /* The entry is linked, let's get the real info */
        if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
                lnk = (struct qnx4_link_info *) de;

It seems that entries may be either struct qnx4_inode_entry or struct
qnx4_link_info but it's not captured in a union.

This needs to be fixed by not lying to the compiler about what is there.

How about this?


diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..3cd20065bcfa 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -26,31 +26,39 @@
 static int qnx4_match(int len, const char *name,
 		      struct buffer_head *bh, unsigned long *offset)
 {
-	struct qnx4_inode_entry *de;
-	int namelen, thislen;
+	union qnx4_dir_entry *de;
+	char *entry_fname;
+	int entry_len, entry_max_len;
 
 	if (bh == NULL) {
 		printk(KERN_WARNING "qnx4: matching unassigned buffer !\n");
 		return 0;
 	}
-	de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
+	de = (union qnx4_dir_entry *) (bh->b_data + *offset);
 	*offset += QNX4_DIR_ENTRY_SIZE;
-	if ((de->di_status & QNX4_FILE_LINK) != 0) {
-		namelen = QNX4_NAME_MAX;
-	} else {
-		namelen = QNX4_SHORT_NAME_MAX;
-	}
-	thislen = strlen( de->di_fname );
-	if ( thislen > namelen )
-		thislen = namelen;
-	if (len != thislen) {
+
+	switch (de->inode.di_status) {
+	case QNX4_FILE_LINK:
+		entry_fname = de->link.dl_fname;
+		entry_max_len = sizeof(de->link.dl_fname);
+		break;
+	case QNX4_FILE_USED:
+		entry_fname = de->inode.di_fname;
+		entry_max_len = sizeof(de->inode.di_fname);
+		break;
+	default:
 		return 0;
 	}
-	if (strncmp(name, de->di_fname, len) == 0) {
-		if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) {
-			return 1;
-		}
-	}
+
+	/* Directory entry may not be %NUL-terminated. */
+	entry_len = strnlen(entry_fname, entry_max_len);
+
+	if (len != entry_len)
+		return 0;
+
+	if (strncmp(name, entry_fname, len) == 0)
+		return 1;
+
 	return 0;
 }
 
diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h
index 31487325d265..e033dbe1e009 100644
--- a/include/uapi/linux/qnx4_fs.h
+++ b/include/uapi/linux/qnx4_fs.h
@@ -68,6 +68,13 @@ struct qnx4_link_info {
 	__u8		dl_status;
 };
 
+union qnx4_dir_entry {
+	struct qnx4_inode_entry inode;
+	struct qnx4_link_info link;
+};
+_Static_assert(offsetof(struct qnx4_inode_entry, di_status) ==
+	       offsetof(struct qnx4_link_info, dl_status));
+
 struct qnx4_xblk {
 	__le32		xblk_next_xblk;
 	__le32		xblk_prev_xblk;


-- 
Kees Cook

On 2023-11-16 15:58 Kees Cook wrote:
> On Thu, Nov 16, 2023 at 06:29:59AM -0800, Kees Cook wrote:
> > On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote:
> > > qnx4 dir name length can vary to be of maximum size
> > > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> > > 'link info' entry is stored and the status byte is set.
> > > So to avoid buffer overflow check di_fname length
> > > fetched from (struct qnx4_inode_entry *)
> > > before use in strlen to avoid buffer overflow.
> > > 
> > > panic context
> > > [ 4849.636861] detected buffer overflow in strlen
> > > [ 4849.636897] ------------[ cut here ]------------
> > > [ 4849.636902] kernel BUG at lib/string.c:1165!
> > > [ 4849.636917] invalid opcode: 0000 [#2] SMP PTI
> > > ..
> > > [ 4849.637047] Call Trace:
> > > [ 4849.637053]  <TASK>
> > > [ 4849.637059]  ? show_trace_log_lvl+0x1d6/0x2ea
> > > [ 4849.637075]  ? show_trace_log_lvl+0x1d6/0x2ea
> > > [ 4849.637095]  ? qnx4_find_entry.cold+0xc/0x18 [qnx4]
> > > [ 4849.637111]  ? show_regs.part.0+0x23/0x29
> > > [ 4849.637123]  ? __die_body.cold+0x8/0xd
> > > [ 4849.637135]  ? __die+0x2b/0x37
> > > [ 4849.637147]  ? die+0x30/0x60
> > > [ 4849.637161]  ? do_trap+0xbe/0x100
> > > [ 4849.637171]  ? do_error_trap+0x6f/0xb0
> > > [ 4849.637180]  ? fortify_panic+0x13/0x15
> > > [ 4849.637192]  ? exc_invalid_op+0x53/0x70
> > > [ 4849.637203]  ? fortify_panic+0x13/0x15
> > > [ 4849.637215]  ? asm_exc_invalid_op+0x1b/0x20
> > > [ 4849.637228]  ? fortify_panic+0x13/0x15
> > > [ 4849.637240]  ? fortify_panic+0x13/0x15
> > > [ 4849.637251]  qnx4_find_entry.cold+0xc/0x18 [qnx4]
> > > [ 4849.637264]  qnx4_lookup+0x3c/0xa0 [qnx4]
> > > [ 4849.637275]  __lookup_slow+0x85/0x150
> > > [ 4849.637291]  walk_component+0x145/0x1c0
> > > [ 4849.637304]  ? path_init+0x2c0/0x3f0
> > > [ 4849.637316]  path_lookupat+0x6e/0x1c0
> > > [ 4849.637330]  filename_lookup+0xcf/0x1d0
> > > [ 4849.637341]  ? __check_object_size+0x1d/0x30
> > > [ 4849.637354]  ? strncpy_from_user+0x44/0x150
> > > [ 4849.637365]  ? getname_flags.part.0+0x4c/0x1b0
> > > [ 4849.637375]  user_path_at_empty+0x3f/0x60
> > > [ 4849.637383]  vfs_statx+0x7a/0x130
> > > [ 4849.637393]  do_statx+0x45/0x80
> > > ..
> > > 
> > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com>
> > > ---
> > > 
> > >  fs/qnx4/namei.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> > > index 8d72221735d7..825b891a52b3 100644
> > > --- a/fs/qnx4/namei.c
> > > +++ b/fs/qnx4/namei.c
> > > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name,
> > > 
> > >  	} else {
> > >  	
> > >  		namelen = QNX4_SHORT_NAME_MAX;
> > >  	
> > >  	}
> > > 
> > > +
> > > +	/** qnx4 dir name length can vary, check the di_fname
> > > +	 * fetched from (struct qnx4_inode_entry *) before use in
> > > +	 * strlen to avoid panic due to buffer overflow"
> > > +	 */
> > 
> > Style nit: this comment should start with just "/*" alone, like:
> > 	/*
> > 	
> > 	 * qnx4 dir name ...
> > 	 
> > > +	if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
> > > +		return -ENAMETOOLONG;
> > > 
> > >  	thislen = strlen( de->di_fname );
> > 
> > de->di_fname is:
> > 
> > struct qnx4_inode_entry {
> > 
> >         char            di_fname[QNX4_SHORT_NAME_MAX];
> > 	
> > 	...
> > 
> > #define QNX4_SHORT_NAME_MAX     16
> > #define QNX4_NAME_MAX           48
> > 
> > It's always going to have a max of QNX4_SHORT_NAME_MAX. Is any of this
> > code correct if namelen ends up being QNX4_NAME_MAX? It'll be reading
> > past the end of di_fname.
> > 
> > Is bh->b_data actually struct qnx4_inode_entry ?
> 
> Ah-ha, it looks like it's _not_:
> 
>         if (!(bh = qnx4_find_entry(len, dir, name, &de, &ino)))
>                 goto out;
>         /* The entry is linked, let's get the real info */
>         if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
>                 lnk = (struct qnx4_link_info *) de;
> 
> It seems that entries may be either struct qnx4_inode_entry or struct
> qnx4_link_info but it's not captured in a union.
> 
> This needs to be fixed by not lying to the compiler about what is there.
> 
> How about this?

The switch won't work since the _status field is a bit-field, so we should 
rather reuse the similar union-logic already present in fs/qnx4/dir.c

> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> index 8d72221735d7..3cd20065bcfa 100644
> --- a/fs/qnx4/namei.c
> +++ b/fs/qnx4/namei.c
> @@ -26,31 +26,39 @@
>  static int qnx4_match(int len, const char *name,
>  		      struct buffer_head *bh, unsigned long *offset)
>  {
> -	struct qnx4_inode_entry *de;
> -	int namelen, thislen;
> +	union qnx4_dir_entry *de;
> +	char *entry_fname;
> +	int entry_len, entry_max_len;
> 
>  	if (bh == NULL) {
>  		printk(KERN_WARNING "qnx4: matching unassigned buffer !
\n");
>  		return 0;
>  	}
> -	de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
> +	de = (union qnx4_dir_entry *) (bh->b_data + *offset);
>  	*offset += QNX4_DIR_ENTRY_SIZE;
> -	if ((de->di_status & QNX4_FILE_LINK) != 0) {
> -		namelen = QNX4_NAME_MAX;
> -	} else {
> -		namelen = QNX4_SHORT_NAME_MAX;
> -	}
> -	thislen = strlen( de->di_fname );
> -	if ( thislen > namelen )
> -		thislen = namelen;
> -	if (len != thislen) {
> +
> +	switch (de->inode.di_status) {
> +	case QNX4_FILE_LINK:
> +		entry_fname = de->link.dl_fname;
> +		entry_max_len = sizeof(de->link.dl_fname);
> +		break;
> +	case QNX4_FILE_USED:
> +		entry_fname = de->inode.di_fname;
> +		entry_max_len = sizeof(de->inode.di_fname);
> +		break;
> +	default:
>  		return 0;
>  	}
> -	if (strncmp(name, de->di_fname, len) == 0) {
> -		if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) !
= 0) {
> -			return 1;
> -		}
> -	}
> +
> +	/* Directory entry may not be %NUL-terminated. */
> +	entry_len = strnlen(entry_fname, entry_max_len);
> +
> +	if (len != entry_len)
> +		return 0;
> +
> +	if (strncmp(name, entry_fname, len) == 0)
> +		return 1;
> +
>  	return 0;
>  }
> 
> diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h
> index 31487325d265..e033dbe1e009 100644
> --- a/include/uapi/linux/qnx4_fs.h
> +++ b/include/uapi/linux/qnx4_fs.h
> @@ -68,6 +68,13 @@ struct qnx4_link_info {
>  	__u8		dl_status;
>  };
> 
> +union qnx4_dir_entry {
> +	struct qnx4_inode_entry inode;
> +	struct qnx4_link_info link;
> +};
> +_Static_assert(offsetof(struct qnx4_inode_entry, di_status) ==
> +	       offsetof(struct qnx4_link_info, dl_status));
> +
>  struct qnx4_xblk {
>  	__le32		xblk_next_xblk;
>  	__le32		xblk_prev_xblk;

On Thu, Nov 16, 2023 at 05:48:20PM +0100, Anders Larsen wrote:
> On 2023-11-16 15:58 Kees Cook wrote:
> >         if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
> >                 lnk = (struct qnx4_link_info *) de;
> > 
> > It seems that entries may be either struct qnx4_inode_entry or struct
> > qnx4_link_info but it's not captured in a union.
> > 
> > This needs to be fixed by not lying to the compiler about what is there.
> > 
> > How about this?
> 
> > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> > index 8d72221735d7..3cd20065bcfa 100644
> > --- a/fs/qnx4/namei.c
> > +++ b/fs/qnx4/namei.c
> > @@ -26,31 +26,39 @@
> >  static int qnx4_match(int len, const char *name,
> >  		      struct buffer_head *bh, unsigned long *offset)
> >  {
> > -	struct qnx4_inode_entry *de;
> > -	int namelen, thislen;
> > +	union qnx4_dir_entry *de;
> > +	char *entry_fname;
> > +	int entry_len, entry_max_len;
> > 
> >  	if (bh == NULL) {
> >  		printk(KERN_WARNING "qnx4: matching unassigned buffer !
> \n");
> >  		return 0;
> >  	}
> > -	de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
> > +	de = (union qnx4_dir_entry *) (bh->b_data + *offset);
> >  	*offset += QNX4_DIR_ENTRY_SIZE;
> > -	if ((de->di_status & QNX4_FILE_LINK) != 0) {
> > -		namelen = QNX4_NAME_MAX;
> > -	} else {
> > -		namelen = QNX4_SHORT_NAME_MAX;
> > -	}
> > -	thislen = strlen( de->di_fname );
> > -	if ( thislen > namelen )
> > -		thislen = namelen;
> > -	if (len != thislen) {
> > +
> > +	switch (de->inode.di_status) {
> > +	case QNX4_FILE_LINK:
> > +		entry_fname = de->link.dl_fname;
> > +		entry_max_len = sizeof(de->link.dl_fname);
> > +		break;
> > +	case QNX4_FILE_USED:
> > +		entry_fname = de->inode.di_fname;
> > +		entry_max_len = sizeof(de->inode.di_fname);
> > +		break;
> > +	default:
> >  		return 0;
> >  	}
> 
> The switch won't work since the _status field is a bit-field, so we should 
> rather reuse the similar union-logic already present in fs/qnx4/dir.c

Ah, okay, LINK and USED might both be there. And perfect, yes, it looks
like the union qnx4_directory_entry in fs/qnx4/dir.c would be perfect.

-Kees

> > -	if (strncmp(name, de->di_fname, len) == 0) {
> > -		if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) !
> = 0) {
> > -			return 1;
> > -		}
> > -	}
> > +
> > +	/* Directory entry may not be %NUL-terminated. */
> > +	entry_len = strnlen(entry_fname, entry_max_len);
> > +
> > +	if (len != entry_len)
> > +		return 0;
> > +
> > +	if (strncmp(name, entry_fname, len) == 0)
> > +		return 1;
> > +
> >  	return 0;
> >  }
> > 
> > diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h
> > index 31487325d265..e033dbe1e009 100644
> > --- a/include/uapi/linux/qnx4_fs.h
> > +++ b/include/uapi/linux/qnx4_fs.h
> > @@ -68,6 +68,13 @@ struct qnx4_link_info {
> >  	__u8		dl_status;
> >  };
> > 
> > +union qnx4_dir_entry {
> > +	struct qnx4_inode_entry inode;
> > +	struct qnx4_link_info link;
> > +};
> > +_Static_assert(offsetof(struct qnx4_inode_entry, di_status) ==
> > +	       offsetof(struct qnx4_link_info, dl_status));
> > +
> >  struct qnx4_xblk {
> >  	__le32		xblk_next_xblk;
> >  	__le32		xblk_prev_xblk;
> 
> 
> 
> 

-- 
Kees Cook

Thank you Kees and Anders, Cheers

BR,
Ronald

On Fri, Nov 17, 2023 at 4:26 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Nov 16, 2023 at 05:48:20PM +0100, Anders Larsen wrote:
> > On 2023-11-16 15:58 Kees Cook wrote:
> > >         if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
> > >                 lnk = (struct qnx4_link_info *) de;
> > >
> > > It seems that entries may be either struct qnx4_inode_entry or struct
> > > qnx4_link_info but it's not captured in a union.
> > >
> > > This needs to be fixed by not lying to the compiler about what is there.
> > >
> > > How about this?
> >
> > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> > > index 8d72221735d7..3cd20065bcfa 100644
> > > --- a/fs/qnx4/namei.c
> > > +++ b/fs/qnx4/namei.c
> > > @@ -26,31 +26,39 @@
> > >  static int qnx4_match(int len, const char *name,
> > >                   struct buffer_head *bh, unsigned long *offset)
> > >  {
> > > -   struct qnx4_inode_entry *de;
> > > -   int namelen, thislen;
> > > +   union qnx4_dir_entry *de;
> > > +   char *entry_fname;
> > > +   int entry_len, entry_max_len;
> > >
> > >     if (bh == NULL) {
> > >             printk(KERN_WARNING "qnx4: matching unassigned buffer !
> > \n");
> > >             return 0;
> > >     }
> > > -   de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
> > > +   de = (union qnx4_dir_entry *) (bh->b_data + *offset);
> > >     *offset += QNX4_DIR_ENTRY_SIZE;
> > > -   if ((de->di_status & QNX4_FILE_LINK) != 0) {
> > > -           namelen = QNX4_NAME_MAX;
> > > -   } else {
> > > -           namelen = QNX4_SHORT_NAME_MAX;
> > > -   }
> > > -   thislen = strlen( de->di_fname );
> > > -   if ( thislen > namelen )
> > > -           thislen = namelen;
> > > -   if (len != thislen) {
> > > +
> > > +   switch (de->inode.di_status) {
> > > +   case QNX4_FILE_LINK:
> > > +           entry_fname = de->link.dl_fname;
> > > +           entry_max_len = sizeof(de->link.dl_fname);
> > > +           break;
> > > +   case QNX4_FILE_USED:
> > > +           entry_fname = de->inode.di_fname;
> > > +           entry_max_len = sizeof(de->inode.di_fname);
> > > +           break;
> > > +   default:
> > >             return 0;
> > >     }
> >
> > The switch won't work since the _status field is a bit-field, so we should
> > rather reuse the similar union-logic already present in fs/qnx4/dir.c
>
> Ah, okay, LINK and USED might both be there. And perfect, yes, it looks
> like the union qnx4_directory_entry in fs/qnx4/dir.c would be perfect.
>
> -Kees
>
> > > -   if (strncmp(name, de->di_fname, len) == 0) {
> > > -           if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) !
> > = 0) {
> > > -                   return 1;
> > > -           }
> > > -   }
> > > +
> > > +   /* Directory entry may not be %NUL-terminated. */
> > > +   entry_len = strnlen(entry_fname, entry_max_len);
> > > +
> > > +   if (len != entry_len)
> > > +           return 0;
> > > +
> > > +   if (strncmp(name, entry_fname, len) == 0)
> > > +           return 1;
> > > +
> > >     return 0;
> > >  }
> > >
> > > diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h
> > > index 31487325d265..e033dbe1e009 100644
> > > --- a/include/uapi/linux/qnx4_fs.h
> > > +++ b/include/uapi/linux/qnx4_fs.h
> > > @@ -68,6 +68,13 @@ struct qnx4_link_info {
> > >     __u8            dl_status;
> > >  };
> > >
> > > +union qnx4_dir_entry {
> > > +   struct qnx4_inode_entry inode;
> > > +   struct qnx4_link_info link;
> > > +};
> > > +_Static_assert(offsetof(struct qnx4_inode_entry, di_status) ==
> > > +          offsetof(struct qnx4_link_info, dl_status));
> > > +
> > >  struct qnx4_xblk {
> > >     __le32          xblk_next_xblk;
> > >     __le32          xblk_prev_xblk;
> >
> >
> >
> >
>
> --
> Kees Cook

On 2023-11-12 10:53 Ronald Monthero wrote:
> qnx4 dir name length can vary to be of maximum size
> QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> 'link info' entry is stored and the status byte is set.
> So to avoid buffer overflow check di_fname length
> fetched from (struct qnx4_inode_entry *)
> before use in strlen to avoid buffer overflow.

[snip]

> 
> Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com>
> ---
>  fs/qnx4/namei.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> index 8d72221735d7..825b891a52b3 100644
> --- a/fs/qnx4/namei.c
> +++ b/fs/qnx4/namei.c
> @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name,
>  	} else {
>  		namelen = QNX4_SHORT_NAME_MAX;
>  	}
> +
> +	/** qnx4 dir name length can vary, check the di_fname
> +	 * fetched from (struct qnx4_inode_entry *) before use in
> +	 * strlen to avoid panic due to buffer overflow"
> +	 */
> +	if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
> +		return -ENAMETOOLONG;

sizeof(de->di_fname) equals QNX4_SHORT_NAME_MAX, so this test fails as soon as 
a filename is longer than that!

This quick fix (untested!) should do the trick (and avoids computing the length 
of the name twice):

diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..7694f86fbb2e 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -40,9 +40,7 @@ static int qnx4_match(int len, const char *name,
 	} else {
 		namelen = QNX4_SHORT_NAME_MAX;
 	}
-	thislen = strlen( de->di_fname );
-	if ( thislen > namelen )
-		thislen = namelen;
+	thislen = strnlen(de->di_fname, namelen);
 	if (len != thislen) {
 		return 0;
 	}

Cheers
Anders

On Mon, Nov 13, 2023 at 2:16 AM Anders Larsen <al@alarsen.net> wrote:
>
> On 2023-11-12 10:53 Ronald Monthero wrote:
> > qnx4 dir name length can vary to be of maximum size
> > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> > 'link info' entry is stored and the status byte is set.
> > So to avoid buffer overflow check di_fname length
> > fetched from (struct qnx4_inode_entry *)
> > before use in strlen to avoid buffer overflow.
>
> [snip]
>
> >
> > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com>
> > ---
> >  fs/qnx4/namei.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> > index 8d72221735d7..825b891a52b3 100644
> > --- a/fs/qnx4/namei.c
> > +++ b/fs/qnx4/namei.c
> > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name,
> >       } else {
> >               namelen = QNX4_SHORT_NAME_MAX;
> >       }
> > +
> > +     /** qnx4 dir name length can vary, check the di_fname
> > +      * fetched from (struct qnx4_inode_entry *) before use in
> > +      * strlen to avoid panic due to buffer overflow"
> > +      */
> > +     if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
> > +             return -ENAMETOOLONG;
>
> sizeof(de->di_fname) equals QNX4_SHORT_NAME_MAX, so this test fails as soon as
> a filename is longer than that!

I suppose de->di_fname can be QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX isn't it ?
It's set based on di_status, if di_status is set, then it will be
QNX4_NAME_MAX and otherwise QNX4_SHORT_NAME_MAX.
We capture that into namelen, prior to the  string length check  - if
(strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
as below:

        de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
        *offset += QNX4_DIR_ENTRY_SIZE;
        if ((de->di_status & QNX4_FILE_LINK) != 0) {
                namelen = QNX4_NAME_MAX;
        } else {
                namelen = QNX4_SHORT_NAME_MAX;
        }

BR,
Ron


> This quick fix (untested!) should do the trick (and avoids computing the length
> of the name twice):
>
> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> index 8d72221735d7..7694f86fbb2e 100644
> --- a/fs/qnx4/namei.c
> +++ b/fs/qnx4/namei.c
> @@ -40,9 +40,7 @@ static int qnx4_match(int len, const char *name,
>         } else {
>                 namelen = QNX4_SHORT_NAME_MAX;
>         }
> -       thislen = strlen( de->di_fname );
> -       if ( thislen > namelen )
> -               thislen = namelen;
> +       thislen = strnlen(de->di_fname, namelen);
>         if (len != thislen) {
>                 return 0;
>         }
>
> Cheers
> Anders
>
>

On 2023-11-13 10:25 Ronald Monthero wrote:
> On Mon, Nov 13, 2023 at 2:16 AM Anders Larsen <al@alarsen.net> wrote:
> > On 2023-11-12 10:53 Ronald Monthero wrote:
> > > qnx4 dir name length can vary to be of maximum size
> > > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether
> > > 'link info' entry is stored and the status byte is set.
> > > So to avoid buffer overflow check di_fname length
> > > fetched from (struct qnx4_inode_entry *)
> > > before use in strlen to avoid buffer overflow.
> > 
> > [snip]
> > 
> > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com>
> > > ---
> > > 
> > >  fs/qnx4/namei.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> > > index 8d72221735d7..825b891a52b3 100644
> > > --- a/fs/qnx4/namei.c
> > > +++ b/fs/qnx4/namei.c
> > > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name,
> > > 
> > >       } else {
> > >       
> > >               namelen = QNX4_SHORT_NAME_MAX;
> > >       
> > >       }
> > > 
> > > +
> > > +     /** qnx4 dir name length can vary, check the di_fname
> > > +      * fetched from (struct qnx4_inode_entry *) before use in
> > > +      * strlen to avoid panic due to buffer overflow"
> > > +      */
> > > +     if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
> > > +             return -ENAMETOOLONG;
> > 
> > sizeof(de->di_fname) equals QNX4_SHORT_NAME_MAX, so this test fails as
> > soon as a filename is longer than that!
> 
> I suppose de->di_fname can be QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX isn't it
> ? It's set based on di_status, if di_status is set, then it will be
> QNX4_NAME_MAX and otherwise QNX4_SHORT_NAME_MAX.
> We capture that into namelen, prior to the  string length check  - if
> (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname))
> as below:
> 
>         de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
>         *offset += QNX4_DIR_ENTRY_SIZE;
>         if ((de->di_status & QNX4_FILE_LINK) != 0) {
>                 namelen = QNX4_NAME_MAX;
>         } else {
>                 namelen = QNX4_SHORT_NAME_MAX;
>         }
> 
> BR,
> Ron

sizeof(de->di_fname) is evaluated as QNX4_SHORT_NAME_MAX already at compile 
time, see the definition of di_fname in uapi/linux/qnx4_fs.h

I agree that the code is confusing, as 'de' is declared as a pointer to a 
struct qnx4_inode_entry but in reality points to a struct qnx4_link_info iff 
QNX4_FILE_LINK is set in de->di_status.
(Note that the corresponding field dl_status in qnx4_link_info is at the same 
offset as di_status in qnx4_inode_entry - that's the disk layout.)

> > This quick fix (untested!) should do the trick (and avoids computing the
> > length of the name twice):
> > 
> > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> > index 8d72221735d7..7694f86fbb2e 100644
> > --- a/fs/qnx4/namei.c
> > +++ b/fs/qnx4/namei.c
> > @@ -40,9 +40,7 @@ static int qnx4_match(int len, const char *name,
> >         } else {
> >                 namelen = QNX4_SHORT_NAME_MAX;
> >         }
> > -       thislen = strlen( de->di_fname );
> > -       if ( thislen > namelen )
> > -               thislen = namelen;
> > +       thislen = strnlen(de->di_fname, namelen);
> >         if (len != thislen) {
> >                 return 0;
> >         }

Niek reported that this fix improved the situation, but he later got a crash, 
albeit at a different place (but still within the qnx4fs).

Cheers
Anders

On Tue, Nov 14, 2023 at 1:40 AM Anders Larsen <al@alarsen.net> wrote:
>
< Snipped>
>
> sizeof(de->di_fname) is evaluated as QNX4_SHORT_NAME_MAX already at compile
> time, see the definition of di_fname in uapi/linux/qnx4_fs.h
>
> I agree that the code is confusing, as 'de' is declared as a pointer to a
> struct qnx4_inode_entry but in reality points to a struct qnx4_link_info iff
> QNX4_FILE_LINK is set in de->di_status.
> (Note that the corresponding field dl_status in qnx4_link_info is at the same
> offset as di_status in qnx4_inode_entry - that's the disk layout.)
>

Thanks for the details, yes in  struct qnx4_inode_entry  its size
char di_fname[QNX4_SHORT_NAME_MAX];

< snipped>
>
> Niek reported that this fix improved the situation, but he later got a crash,
> albeit at a different place (but still within the qnx4fs).

Yes I saw that Niek has shared the second crash dump stack in above email thread
and also in [1] Bugzilla 218111.The dump stack of the crash looks to
be doing a similar
lookup call context, do_statx => vfs_statx => filename_lookup =>
qn4x_lookup => fortify_panic ( )
[1] https://bugzilla.kernel.org/show_bug.cgi?id=218111#c4

But  I also see a softlockup also in the dump stack, so something in
their environment
is causing softlock ups.  And that tallies with the symptoms of system
freeze that Niek
mentioned " I can mount and view the directories, but after several hours
my system froze up again."

watchdog: BUG: soft lockup - CPU#7 stuck for 26s! [pool-gvfsd-admi:31952]   <<<<

It's possible the softlockups were occurring on fews of the CPUs on the system
for a few hours before the crash occurred that caused a system slow down.

BR,
Ronald