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

[Anders Larsen <al@alarsen.net>]

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