Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231112095353.579855-1-debug.penguin32@gmail.com>
 <202311160612.C38BF44@keescook> <202311160631.5ACFB84B7C@keescook>
 <2910678.e9J7NaK4W3@oscar> <202311161022.6B34F00641@keescook>
In-Reply-To: <202311161022.6B34F00641@keescook>
From:   Ronald Monthero <debug.penguin32@gmail.com>
Date:   Sat, 18 Nov 2023 18:38:37 +1000
Message-ID: <CALk6UxqugQ_8piS6oNr4XdwRTo9dROJ6U-iU_OHUmQC_J_-VWw@mail.gmail.com>
Subject: Re: [PATCH] qnx4: fix to avoid panic due to buffer overflow
To:     Kees Cook <keescook@chromium.org>
Cc:     Anders Larsen <al@alarsen.net>, gustavoars@kernel.org,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Thank you Kees and Anders, Cheers

BR,
Ronald

On Fri, Nov 17, 2023 at 4:26=E2=80=AFAM Kees Cook <keescook@chromium.org> w=
rote:
>
> 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) =3D=3D QNX4_FILE_LINK) {
> > >                 lnk =3D (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 the=
re.
> > >
> > > 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 =3D=3D NULL) {
> > >             printk(KERN_WARNING "qnx4: matching unassigned buffer !
> > \n");
> > >             return 0;
> > >     }
> > > -   de =3D (struct qnx4_inode_entry *) (bh->b_data + *offset);
> > > +   de =3D (union qnx4_dir_entry *) (bh->b_data + *offset);
> > >     *offset +=3D QNX4_DIR_ENTRY_SIZE;
> > > -   if ((de->di_status & QNX4_FILE_LINK) !=3D 0) {
> > > -           namelen =3D QNX4_NAME_MAX;
> > > -   } else {
> > > -           namelen =3D QNX4_SHORT_NAME_MAX;
> > > -   }
> > > -   thislen =3D strlen( de->di_fname );
> > > -   if ( thislen > namelen )
> > > -           thislen =3D namelen;
> > > -   if (len !=3D thislen) {
> > > +
> > > +   switch (de->inode.di_status) {
> > > +   case QNX4_FILE_LINK:
> > > +           entry_fname =3D de->link.dl_fname;
> > > +           entry_max_len =3D sizeof(de->link.dl_fname);
> > > +           break;
> > > +   case QNX4_FILE_USED:
> > > +           entry_fname =3D de->inode.di_fname;
> > > +           entry_max_len =3D sizeof(de->inode.di_fname);
> > > +           break;
> > > +   default:
> > >             return 0;
> > >     }
> >
> > The switch won't work since the _status field is a bit-field, so we sho=
uld
> > 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) =3D=3D 0) {
> > > -           if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) !
> > =3D 0) {
> > > -                   return 1;
> > > -           }
> > > -   }
> > > +
> > > +   /* Directory entry may not be %NUL-terminated. */
> > > +   entry_len =3D strnlen(entry_fname, entry_max_len);
> > > +
> > > +   if (len !=3D entry_len)
> > > +           return 0;
> > > +
> > > +   if (strncmp(name, entry_fname, len) =3D=3D 0)
> > > +           return 1;
> > > +
> > >     return 0;
> > >  }
> > >
> > > diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_f=
s.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) =3D=3D
> > > +          offsetof(struct qnx4_link_info, dl_status));
> > > +
> > >  struct qnx4_xblk {
> > >     __le32          xblk_next_xblk;
> > >     __le32          xblk_prev_xblk;
> >
> >
> >
> >
>
> --
> Kees Cook