On 2023-11-13 10:25 Ronald Monthero wrote:
> On Mon, Nov 13, 2023 at 2:16 AM Anders Larsen <a...@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.pengui...@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
