Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Mateusz Guzik]

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?

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Kees Cook]

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

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Mateusz Guzik]

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>

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Kees Cook]

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

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Mateusz Guzik]

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>

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Christian Brauner]

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?

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Al Viro]

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?

Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow

[Mateusz Guzik]

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>